Add warn auto-ban for repeated admin /warn commands#398
Conversation
Add per-event log table tracking admin /warn commands per (gid, user_id). Mirrors the Reports storage pattern with Add/CountWithin methods and opportunistic cleanup of rows older than 1 year. Tests cover both SQLite and Postgres engines. Related to #71
Adds the Warnings interface to the events package next to the Reports interface, generates the moq mock, and threads the warnings/warnThreshold/ warnWindow fields onto the admin struct so they can be wired in Task 4 and consumed in Task 5.
Adds --warn.threshold and --warn.window flags (env: WARN_THRESHOLD,
WARN_WINDOW) and a corresponding WarnSettings substruct on the top-level
Settings model. Maps options through optToSettings and registers
Warn.Threshold in zeroAwarePaths so 0 ("disabled") survives DB-backed
config loads. Adds startup validation that rejects threshold>0 with
non-positive window, extracted into a testable validateSettings helper.
Add WarnThreshold, WarnWindow, and Warnings fields to TelegramListener and pass them into the admin handler literal. Construct the warnings storage in main.go when --warn.threshold > 0 and populate the listener fields from settings.Warn. The admin handler still gates on warnThreshold == 0 || warnings == nil, so the wiring is inert until Task 5 extends DirectWarnReport to use it.
When warn threshold is configured, DirectWarnReport now records each warn and bans the user or channel once the count within the configured window reaches the threshold. The new executeWarnBan helper respects dry, training, and soft-ban modes, posts an admin-chat notification, and uses the existing banUserOrChannel primitive. With threshold 0 (default) the flow is unchanged.
Add a "Warn auto-ban" subsection to the Bot Behavior tab with warnThreshold and warnWindow inputs (and matching read-only rows for non-confdb mode). Add a self-contained e2e test that boots the binary in --confdb mode, exercises the new inputs, saves, and verifies the values persist across a page reload. Disable CAS in the shared e2e setup so the spam-check assertion is not flaky on slow networks.
Mark Task 8 checkboxes done. Full test suite passes with race detector, linter reports 0 issues, formatters applied, coverage on touched packages ranges 76.7%-87.2%. Default behavior (--warn.threshold=0) confirmed unchanged via early return in DirectWarnReport.
Add a "Warn-Driven Auto-Ban" subsection to README.md describing the sliding-window threshold semantics, ban path, repeat-ban behavior, and the deliberate omission of spam-sample updates. Add a CLAUDE.md subsection under Spam Detection Architecture capturing the non-obvious design decisions (storage cap vs configured window, repeat ban idempotency, no UpdateSpam call, zeroAwarePaths handling). Move the completed plan file to docs/plans/completed/. Related to #71
- skip auto-ban for anonymous admin posts: when SenderChat.ID equals the primary chat id (admin posts "as the group"), the From identity is the shared GroupAnonymousBot user. recording a warn against either ID is meaningless and could trigger BanChatSenderChatConfig against the group itself. - gid-scope the warnings cleanup query: previously cleanupOld used DELETE FROM warnings WHERE created_at < ?, which would prune every tenant's old rows on each Add. now scoped via "WHERE gid = ? AND created_at < ?", matching the Reports cleanup pattern. - extract trackWarnAndMaybeBan and resolveWarnTarget helpers from DirectWarnReport so the function stays focused on the warning-message side. removes the awkward early-return-inside-switch-default that duplicated the function tail. - replace executeWarnBan(userID, userName, channelID, count) signature with executeWarnBan(target warnTarget, count) - two adjacent int64 parameters separated only by a string were swap-prone. Adds tests: - TestAdmin_DirectWarnReport_AutoBan/anonymous_admin_post: covers the SenderChat == primChatID guard - TestWarnings_CleanupOldGIDIsolation: shared sqlite file with two different gids verifies one tenant's cleanup does not prune another's rows
Move Warnings, SpamLogger, and Reports interfaces (with their moq directives and the SpamLoggerFunc helper) from events.go to the file containing their sole consumer struct. Multi-consumer interfaces (TbAPI, Bot, Locator) stay in events.go. Aligns interface placement with the consumer-co-location rule already followed across the rest of the package.
TestManageSamples_DeleteSpam was racing with HTMX in headed mode: the test clicked the newly-swapped delete button before HTMX bound hx-post to it, causing the form to fall back to a native POST against /manage_samples (404). Add WaitForLoadState(NetworkIdle) between add and delete so HTMX has time to process the swapped content. Headless was unaffected because the race fell the other way without slowMo.
Deploying tg-spam with
|
| Latest commit: |
20a13df
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1d056633.tg-spam.pages.dev |
| Branch Preview URL: | https://warn-auto-ban.tg-spam.pages.dev |
There was a problem hiding this comment.
Pull request overview
Implements warn-driven auto-ban (issue #71) by persisting /warn events per (user, gid) and banning once a configurable threshold is reached within a sliding window, including full wiring through CLI → persisted settings → web UI, plus storage/events behavior and tests.
Changes:
- Add
warningsstorage table + retention cleanup + tests. - Wire new
warn.threshold/warn.windowsettings through CLI, config DB, and web UI (with e2e coverage). - Extend admin
/warnhandling to record/count warnings and auto-ban (respecting dry/training/soft-ban) with admin-chat notification.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e-ui/e2e_test.go | Adds e2e coverage for warn settings persistence and reduces CAS-related flakiness. |
| docs/plans/completed/20260427-warn-auto-ban.md | Adds completed implementation plan/notes for the feature. |
| app/webapi/config.go | Parses warnThreshold/warnWindow from settings form submissions. |
| app/webapi/config_test.go | Adds unit tests for warn settings form parsing behavior. |
| app/webapi/assets/settings.html | Adds Warn Auto-Ban inputs and read-only display fields. |
| app/storage/warnings.go | Introduces warnings table storage with Add/CountWithin + cleanup. |
| app/storage/warnings_test.go | Adds storage tests for warnings across SQLite/Postgres and gid isolation. |
| app/config/settings.go | Adds WarnSettings and persists it via Settings struct/db tags + zero-aware path. |
| app/config/settings_test.go | Extends settings round-trip/default-merge tests for Warn settings. |
| app/settings.go | Maps CLI options into persisted config.Settings.Warn. |
| app/settings_test.go | Adds tests for Warn option mapping and struct-tag defaults. |
| app/main.go | Adds CLI flags, settings validation, and warnings storage wiring into listener/admin. |
| app/main_test.go | Adds tests for the new cross-field settings validation rules. |
| app/events/admin.go | Implements warn tracking + auto-ban path and admin notifications. |
| app/events/admin_test.go | Adds comprehensive tests for warn auto-ban behavior and edge cases. |
| app/events/listener.go | Wires warn settings/storage into admin handler and relocates SpamLogger interface. |
| app/events/events.go | Removes relocated interfaces/generate directives (now defined in specific files). |
| app/events/reports.go | Relocates Reports interface + moq directive. |
| app/events/mocks/warnings.go | Adds generated mock for Warnings interface. |
| README.md | Documents warn-driven auto-ban flags/behavior and lists new CLI options. |
| CLAUDE.md | Adds architecture notes for warn auto-ban and settings semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // make a warning message and replay to origMsg.MessageID | ||
| warnTarget := "@" + origMsg.From.UserName | ||
| warnTargetName := "@" + origMsg.From.UserName | ||
| if origMsg.SenderChat != nil && origMsg.SenderChat.ID != 0 && origMsg.SenderChat.ID != a.primChatID { | ||
| chName := a.channelDisplayName(origMsg.SenderChat) | ||
| if origMsg.SenderChat.UserName != "" { | ||
| warnTarget = "@" + chName | ||
| warnTargetName = "@" + chName | ||
| } else { | ||
| warnTarget = chName | ||
| warnTargetName = chName | ||
| } | ||
| } | ||
| warnMsg := fmt.Sprintf("warning from %s\n\n%s %s", update.Message.From.UserName, | ||
| warnTarget, a.warnMsg) | ||
| warnTargetName, a.warnMsg) |
There was a problem hiding this comment.
DirectWarnReport builds warnTargetName from origMsg.From.UserName without checking origMsg.From for nil. For channel messages, Telegram may omit From and only set SenderChat, which would panic before the SenderChat branch runs. Please guard accesses to ReplyToMessage.From / origMsg.From and use SenderChat (or a safe fallback) when From is nil.
| // the feature is disabled (threshold == 0), warnings storage is unwired, or the | ||
| // target cannot be resolved (anonymous admin posts, missing From/SenderChat). | ||
| // returns nil unless the ban itself fails - storage failures are logged but not propagated | ||
| // because the warning message has already been posted (best-effort). | ||
| func (a *admin) trackWarnAndMaybeBan(origMsg *tbapi.Message) error { | ||
| if a.warnThreshold == 0 || a.warnings == nil { |
There was a problem hiding this comment.
Auto-ban disablement is checked with a.warnThreshold == 0. Negative thresholds can be persisted (e.g., crafted form POST), and if warnings storage is wired this would make count < threshold false and trigger bans immediately. Consider treating warnThreshold <= 0 as disabled (and/or validating threshold is non-negative at settings validation and form parsing).
| // the feature is disabled (threshold == 0), warnings storage is unwired, or the | |
| // target cannot be resolved (anonymous admin posts, missing From/SenderChat). | |
| // returns nil unless the ban itself fails - storage failures are logged but not propagated | |
| // because the warning message has already been posted (best-effort). | |
| func (a *admin) trackWarnAndMaybeBan(origMsg *tbapi.Message) error { | |
| if a.warnThreshold == 0 || a.warnings == nil { | |
| // the feature is disabled (threshold <= 0), warnings storage is unwired, or the | |
| // target cannot be resolved (anonymous admin posts, missing From/SenderChat). | |
| // returns nil unless the ban itself fails - storage failures are logged but not propagated | |
| // because the warning message has already been posted (best-effort). | |
| func (a *admin) trackWarnAndMaybeBan(origMsg *tbapi.Message) error { | |
| if a.warnThreshold <= 0 || a.warnings == nil { |
| if s.Warn.Threshold > 0 && s.Warn.Window <= 0 { | ||
| return fmt.Errorf("warn.threshold (%d) is set but warn.window (%v) is not positive", | ||
| s.Warn.Threshold, s.Warn.Window) | ||
| } | ||
| if s.Warn.Threshold > 0 && s.Warn.Window > storage.WarningsRetention { | ||
| return fmt.Errorf("warn.window (%v) exceeds storage retention (%v); older rows are pruned and would not be counted", | ||
| s.Warn.Window, storage.WarningsRetention) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
validateSettings doesn't reject negative warn thresholds. A negative value can be supplied via CLI or persisted via the web form, leaving settings in an invalid/undefined state (and could enable immediate bans if the handler treats non-zero as enabled). Add an invariant like warn.threshold >= 0 (or treat <= 0 as disabled consistently across validation and handler logic).
| // mirrors reactionsWindow (gate on r.Form key presence so unrelated saves preserve | ||
| // the existing window). | ||
| if val := r.FormValue("warnThreshold"); val != "" { | ||
| if n, err := strconv.Atoi(val); err == nil { |
There was a problem hiding this comment.
Form parsing accepts negative warnThreshold values (e.g., "-1") and will persist them. Since threshold semantics are defined as 0=disabled and N>0 enabled, please reject negatives here (ignore, clamp to 0, or surface a validation error) to avoid storing invalid settings.
| if n, err := strconv.Atoi(val); err == nil { | |
| if n, err := strconv.Atoi(val); err == nil { | |
| if n < 0 { | |
| n = 0 | |
| } |
| The feature is disabled by default. To enable it, set `--warn.threshold=, [$WARN_THRESHOLD]` to a positive number and adjust `--warn.window=, [$WARN_WINDOW]` (default: `720h`). When enabled: | ||
|
|
||
| 1. Each `/warn` issued by an admin is recorded in the `warnings` table together with the user/channel id and timestamp | ||
| 2. After recording the warning, the bot counts how many warnings the same user has received within the configured window | ||
| 3. If the count reaches `--warn.threshold=` (i.e. `count >= threshold`), the bot bans the user immediately, respecting `--training`, `--dry`, and `--soft-ban` modes |
There was a problem hiding this comment.
The flag/env-var examples in this README section appear to have stray = and commas (e.g. --warn.threshold=, [$WARN_THRESHOLD] and --warn.threshold=). This reads like a formatting typo and makes the enablement instructions ambiguous; please format them like the rest of the README (e.g., --warn.threshold / $WARN_THRESHOLD, and --warn.window / $WARN_WINDOW).
| The feature is disabled by default. To enable it, set `--warn.threshold=, [$WARN_THRESHOLD]` to a positive number and adjust `--warn.window=, [$WARN_WINDOW]` (default: `720h`). When enabled: | |
| 1. Each `/warn` issued by an admin is recorded in the `warnings` table together with the user/channel id and timestamp | |
| 2. After recording the warning, the bot counts how many warnings the same user has received within the configured window | |
| 3. If the count reaches `--warn.threshold=` (i.e. `count >= threshold`), the bot bans the user immediately, respecting `--training`, `--dry`, and `--soft-ban` modes | |
| The feature is disabled by default. To enable it, set `--warn.threshold` / `$WARN_THRESHOLD` to a positive number and adjust `--warn.window` / `$WARN_WINDOW` (default: `720h`). When enabled: | |
| 1. Each `/warn` issued by an admin is recorded in the `warnings` table together with the user/channel id and timestamp | |
| 2. After recording the warning, the bot counts how many warnings the same user has received within the configured window | |
| 3. If the count reaches `--warn.threshold` (i.e. `count >= threshold`), the bot bans the user immediately, respecting `--training`, `--dry`, and `--soft-ban` modes |
Address Copilot review on PR #398: - admin.go: gate warn auto-ban with `warnThreshold <= 0` instead of `== 0` to match the existing `Report.AutoBanThreshold > 0` convention. Without this, a persisted negative would silently enable ban-on-every-warn. - main.go: validateSettings rejects `warn.threshold < 0` at startup (fail-fast on CLI typos). - README: drop stray help-output formatting in flag references. Tests added for the new validation case and the handler's negative-threshold short-circuit.
implements #71 - track admin
/warnper (user, gid) and auto-ban once the count within a sliding window reaches a configurable threshold.new flags (both off by default):
--warn.threshold/WARN_THRESHOLD(int, default 0 = disabled)--warn.window/WARN_WINDOW(duration, default 720h)behavior: when threshold=0 (default),
/warnkeeps current behavior - delete offending message + admin reply, post a warning, no DB writes. When threshold>0, each /warn additionally records a row in a newwarningstable; if count within window reaches threshold, the user is banned via the same path as/spamand an admin-chat notification fires. Repeat/warnon an already-banned user re-fires the (idempotent) Telegram ban for audit visibility. No spam-sample update.settings: CLI →
config.WarnSettings(db-persisted post #294) → web UI form fields under Bot Behavior tab. e2e test covers the round-trip.storage: new
app/storage/warnings.gotable, idempotentCREATE IF NOT EXISTSfor sqlite and postgres. Opportunistic cleanup of rows older than 1y on eachAdd. No changes to existing tables.related: #71