Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 46 additions & 20 deletions src/commands/moderation/ban.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,30 @@
import type { User } from "grammy/types"
import { CommandsCollection } from "@/lib/managed-commands"
import { logger } from "@/logger"
import { Moderation } from "@/modules/moderation"
import { duration } from "@/utils/duration"
import { fmt } from "@/utils/format"
import { ephemeral } from "@/utils/messages"
import { getTelegramId } from "@/utils/telegram-id"
import { numberOrString, type Role } from "@/utils/types"
import { getUser } from "@/utils/users"
import { getUserFromIdOrUsername } from "@/utils/users"

export const ban = new CommandsCollection<Role>("Banning")
.createCommand({
trigger: "ban",
args: [{ key: "reason", optional: true, description: "Optional reason to ban the user" }],
args: [
{
key: "reasonOrUser",
optional: true,
type: numberOrString,
description:
"If the message is a reply, this argument is the reason. Otherwise, it's the username or user id of the user to mute",
},
{
key: "reason",
optional: true,
description: "Reason to mute the user (only if the first argument is the username or user id)",
Comment on lines +20 to +25
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix copy text in ban argument descriptions.

Lines 20 and 25 say "mute" in the ban command help text, which is confusing for users.

Suggested patch
-          "If the message is a reply, this argument is the reason. Otherwise, it's the username or user id of the user to mute",
+          "If the message is a reply, this argument is the reason. Otherwise, it's the username or user id of the user to ban",
...
-        description: "Reason to mute the user (only if the first argument is the username or user id)",
+        description: "Reason to ban the user (only if the first argument is the username or user id)",
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"If the message is a reply, this argument is the reason. Otherwise, it's the username or user id of the user to mute",
},
{
key: "reason",
optional: true,
description: "Reason to mute the user (only if the first argument is the username or user id)",
"If the message is a reply, this argument is the reason. Otherwise, it's the username or user id of the user to ban",
},
{
key: "reason",
optional: true,
description: "Reason to ban the user (only if the first argument is the username or user id)",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/moderation/ban.ts` around lines 20 - 25, Update the help text
for the ban command arguments to replace incorrect "mute" wording with "ban":
edit the first argument description (the one that mentions reply handling) and
the argument object with key "reason" in the ban command definition so they
state "ban" instead of "mute" (ensure the phrasing matches the ban semantics and
optional note about when reason applies).

},
],
description: "Permanently ban a user from a group",
scope: "group",
reply: "required",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Make non-reply /ban path reachable.

Line 30 still has reply: "required", so the non-reply targeting branch at Lines 47-63 is effectively blocked.

Suggested patch
-    reply: "required",
+    reply: "optional",
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
reply: "required",
reply: "optional",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/moderation/ban.ts` at line 30, The command definition currently
forces reply-only usage by setting reply: "required" which prevents the
non-reply target branch in the /ban handler from ever executing; update the
command option/property named reply in src/commands/moderation/ban.ts to be
optional (e.g., "optional" or remove the required flag) so the non-reply
targeting logic in the branch around the handler that checks for an explicit
target (the code handling the non-reply path at the branch around lines 47-63)
can be reached; ensure the command validation still handles both reply and
explicit-target cases in the ban handler function.

Expand All @@ -21,12 +34,34 @@ export const ban = new CommandsCollection<Role>("Banning")
allowGroupAdmins: true,
},
handler: async ({ args, context, repliedTo }) => {
if (!repliedTo.from) {
logger.error("ban: no repliedTo.from field (the msg was sent in a channel)")
return
}
let user: User | null = null
let reason: string | undefined

const res = await Moderation.ban(repliedTo.from, context.chat, context.from, null, [repliedTo], args.reason)
if (repliedTo) {
if (!repliedTo.from) {
logger.error("BAN: no repliedTo.from field (the msg was sent in a channel)")
return
}
user = repliedTo.from
reason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ") ?? undefined
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Normalize empty reason to undefined.

Line 46 uses .join(" ") ?? undefined; join never returns nullish, so this can pass "" instead of undefined.

Suggested patch
-        reason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ") ?? undefined
+        const mergedReason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ").trim()
+        reason = mergedReason || undefined
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
reason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ") ?? undefined
const mergedReason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ").trim()
reason = mergedReason || undefined
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/moderation/ban.ts` at line 46, The assignment for reason
currently does reason = [args.reasonOrUser, args.reason].filter(Boolean).join("
") ?? undefined but join never yields nullish so an empty string can slip
through; update the logic that computes reason (the variable named reason using
args.reasonOrUser and args.reason) to normalize empty or whitespace-only results
to undefined (e.g., build the joined string, trim it, and set reason = trimmed
|| undefined) so empty reasons become undefined.

} else {
if (!args.reasonOrUser) {
const msg = await context.reply(
fmt(({ b }) => b`You must specify a user to ban or reply to one of their messages`)
)
await ephemeral(msg)
return
}
user = await getUserFromIdOrUsername(args.reasonOrUser, context)
if (!user) {
const msg = await context.reply(fmt(({ n }) => n`Error: cannot find this user`))
logger.error({ user: args.reasonOrUser }, "BAN: cannot retrieve the user")
await ephemeral(msg)
return
}
reason = args.reason
}
const res = await Moderation.ban(user, context.chat, context.from, null, repliedTo ? [repliedTo] : [], reason)
if (res.isErr()) await ephemeral(context.reply(res.error.fmtError))
},
})
Expand All @@ -51,7 +86,7 @@ export const ban = new CommandsCollection<Role>("Banning")
},
handler: async ({ args, context, repliedTo }) => {
if (!repliedTo.from) {
logger.error("ban: no repliedTo.from field (the msg was sent in a channel)")
logger.error("TBAN: no repliedTo.from field (the msg was sent in a channel)")
return
}

Expand All @@ -77,18 +112,9 @@ export const ban = new CommandsCollection<Role>("Banning")
allowGroupAdmins: true,
},
handler: async ({ args, context }) => {
const userId: number | null =
typeof args.username === "string" ? await getTelegramId(args.username.replaceAll("@", "")) : args.username

if (!userId) {
logger.debug(`unban: no userId for username ${args.username}`)
await ephemeral(context.reply(fmt(({ b }) => b`@${context.from.username} user not found`)))
return
}

const user = await getUser(userId, context)
const user = await getUserFromIdOrUsername(args.username, context)
if (!user) {
logger.error({ userId }, "UNBAN: cannot retrieve the user")
logger.error({ user: args.username }, "UNBAN: cannot retrieve the user")
await ephemeral(context.reply(fmt(({ n }) => [n`Error: cannot find this user`])))
return
}
Expand Down
67 changes: 47 additions & 20 deletions src/commands/moderation/mute.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import type { User } from "grammy/types"
import { CommandsCollection } from "@/lib/managed-commands"
import { logger } from "@/logger"
import { Moderation } from "@/modules/moderation"
import { duration } from "@/utils/duration"
import { fmt } from "@/utils/format"
import { ephemeral } from "@/utils/messages"
import { getTelegramId } from "@/utils/telegram-id"
import { numberOrString, type Role } from "@/utils/types"
import { getUser } from "@/utils/users"
import { getUserFromIdOrUsername } from "@/utils/users"

export const mute = new CommandsCollection<Role>("Muting")
.createCommand({
Expand All @@ -30,7 +30,7 @@ export const mute = new CommandsCollection<Role>("Muting")
},
handler: async ({ args, context, repliedTo }) => {
if (!repliedTo.from) {
logger.error("tmute: no repliedTo.from field (the msg was sent in a channel)")
logger.error("TMUTE: no repliedTo.from field (the msg was sent in a channel)")
return
}

Expand All @@ -47,22 +47,58 @@ export const mute = new CommandsCollection<Role>("Muting")
})
.createCommand({
trigger: "mute",
args: [{ key: "reason", optional: true, description: "Optional reason to mute the user" }],
args: [
{
key: "reasonOrUser",
optional: true,
type: numberOrString,
description:
"If the message is a reply, this argument is the reason. Otherwise, it's the username or user id of the user to mute",
},
{
key: "reason",
optional: true,
description: "Reason to mute the user (only if the first argument is the username or user id)",
},
],
description: "Permanently mute a user from a group",
scope: "group",
reply: "required",
reply: "optional",
permissions: {
allowedRoles: ["owner", "direttivo"],
excludedRoles: ["creator"],
allowGroupAdmins: true,
},
handler: async ({ args, context, repliedTo }) => {
if (!repliedTo.from) {
logger.error("mute: no repliedTo.from field (the msg was sent in a channel)")
return
let user: User | null = null
let reason: string | undefined

if (repliedTo) {
if (!repliedTo.from) {
logger.error("MUTE: no repliedTo.from field (the msg was sent in a channel)")
return
}
user = repliedTo.from
reason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ") ?? undefined
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Avoid sending empty-string reasons.

Line 82 can produce "" (not undefined) because .join(" ") ?? undefined never yields nullish.

Suggested patch
-        reason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ") ?? undefined
+        const mergedReason = [args.reasonOrUser, args.reason].filter(Boolean).join(" ").trim()
+        reason = mergedReason || undefined
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/moderation/mute.ts` at line 82, The current assignment to reason
can end up as an empty string (""), so update the expression that computes
reason in mute.ts (the variable named reason) to normalize empty results to
undefined; for example, build the joined string from [args.reasonOrUser,
args.reason], trim it and then use a fallback like || undefined (or explicitly
set to undefined when === "") so that empty-string reasons are not sent.

} else {
if (!args.reasonOrUser) {
const msg = await context.reply(
fmt(({ b }) => b`You must specify a user to mute or reply to one of their messages`)
)
await ephemeral(msg)
return
}
user = await getUserFromIdOrUsername(args.reasonOrUser, context)
if (!user) {
const msg = await context.reply(fmt(({ n }) => n`Error: cannot find this user`))
logger.error({ user: args.reasonOrUser }, "MUTE: cannot retrieve the user")
await ephemeral(msg)
return
}
reason = args.reason
}

const res = await Moderation.mute(repliedTo.from, context.chat, context.from, null, [repliedTo], args.reason)
const res = await Moderation.mute(user, context.chat, context.from, null, repliedTo ? [repliedTo] : [], reason)
if (res.isErr()) await ephemeral(context.reply(res.error.fmtError))
},
})
Expand All @@ -77,19 +113,10 @@ export const mute = new CommandsCollection<Role>("Muting")
allowGroupAdmins: true,
},
handler: async ({ args, context }) => {
const userId: number | null =
typeof args.username === "string" ? await getTelegramId(args.username.replaceAll("@", "")) : args.username
if (!userId) {
logger.debug(`unmute: no userId for username ${args.username}`)
const msg = await context.reply(fmt(({ b }) => b`@${context.from.username} user not found`))
await ephemeral(msg)
return
}

const user = await getUser(userId, context)
const user = await getUserFromIdOrUsername(args.username, context)
if (!user) {
const msg = await context.reply(fmt(({ n }) => n`Error: cannot find this user`))
logger.error({ userId }, "UNMUTE: cannot retrieve the user")
logger.error({ user: args.username }, "UNMUTE: cannot retrieve the user")
await ephemeral(msg)
return
}
Expand Down
15 changes: 15 additions & 0 deletions src/utils/users.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,28 @@
import type { Context } from "grammy"
import type { User } from "grammy/types"
import { logger } from "@/logger"
import { MessageUserStorage } from "@/middlewares/message-user-storage"
import { getTelegramId } from "./telegram-id"

export async function getUser<C extends Context>(userId: number, ctx: C | null): Promise<User | null> {
// TODO: check if this works correctly
const chatUser = ctx ? await ctx.getChatMember(userId).catch(() => null) : null
return chatUser?.user ?? MessageUserStorage.getInstance().getStoredUser(userId)
}

export async function getUserFromIdOrUsername<C extends Context>(
idOrUsername: number | string,
ctx: C | null
): Promise<User | null> {
const userId = typeof idOrUsername === "string" ? await getTelegramId(idOrUsername.replaceAll("@", "")) : idOrUsername
if (!userId) {
logger.debug(`unmute: no userId for username ${idOrUsername}`)
return null
}

return await getUser(userId, ctx)
}

/**
* Formats a user's username and ID for logging.
* @param user grammY User object
Expand Down
Loading