feat: decouple ManagedCommands from business logic#68
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughReplaces the centralized Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Managed as ManagedCommands
participant Hooks as Hooks System
participant Command as Command Handler
participant API as Backend/API
User->>Managed: send command text
Managed->>Managed: parse trigger, resolve command (collection-aware)
alt wrong scope
Managed->>Hooks: wrongScope(params)
Hooks->>User: scope error reply
else missing permissions
Managed->>Hooks: missingPermissions(params)
Hooks->>User: permission denied reply (ephemeral possible)
else allowed
Managed->>Hooks: beforeHandler(params)
Hooks-->>Managed: (pre-exec actions)
Managed->>Command: execute handler(context,args)
Command->>API: backend/db calls
API-->>Command: response
Command->>User: reply result
alt handler throws
Command-->>Managed: throw error
Managed->>Hooks: handlerError({ error, ... })
Hooks->>API: log/report error
end
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
src/commands/userid.ts (1)
13-13: Inconsistent@removal compared to other commands.Other commands in this PR (e.g.,
role.tslines 22, 52, 94 andmute.tsline 79) usereplaceAll("@", "")to strip the@symbol, while this usesreplace("@", ""). Although usernames typically have only one@prefix, usingreplaceAllmaintains consistency across the codebase.♻️ Suggested fix for consistency
- const username = args.username.replace("@", "") + const username = args.username.replaceAll("@", "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/userid.ts` at line 13, Update the username sanitization to use replaceAll instead of replace for consistency with other commands: locate the const username assignment in userid.ts (the line declaring const username = args.username.replace("@", "")) and change it to call replaceAll("@", "") so all occurrences of "@" are removed and behavior matches role.ts and mute.ts handling.src/commands/audit.ts (1)
26-45: Consider handling empty audit log case.When
listis empty, the response will only show the header (🧾 Audit Log: ${username}) followed by a separator line, which may be confusing to users. Consider adding an explicit "No audit records found" message.♻️ Suggested improvement for empty list
const list = await api.tg.auditLog.getById.query({ targetId: userId }) + if (list.length === 0) { + await context.reply(fmt(({ b }) => b`🧾 No audit records found for ${args.username}`)) + return + } await context.reply( fmt(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/audit.ts` around lines 26 - 45, The current code builds and replies with the audit list (using list from api.tg.auditLog.getById.query and formatting via fmt) but does not handle the empty-list case; update the logic in the handler around list (the variable produced by api.tg.auditLog.getById.query) to detect when list.length === 0 and send a clear reply (via context.reply / same fmt flow) that includes the header with args.username and an explicit "No audit records found" message (or similar) instead of the separators-only output, keeping existing formatting helpers (fmt, fmtDate) and structure for non-empty lists.src/commands/mute.ts (1)
77-97: Inconsistent error handling pattern inunmutehandler.The
unmutehandler mixes two different patterns for auto-deleting error messages:
- Lines 83, 91: Manual
wait(5000).then(async () => msg.delete())- Line 96:
void ephemeral(context.reply(...))The manual pattern doesn't handle deletion failures and is more verbose. Consider using
ephemeral()consistently for all error replies in this handler.♻️ Suggested fix for consistency
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`)) - void wait(5000).then(async () => msg.delete()) + void ephemeral(context.reply(fmt(({ b }) => b`@${context.from.username} user not found`))) return } const user = await getUser(userId, context) if (!user) { - const msg = await context.reply("Error: cannot find this user") logger.error({ userId }, "UNMUTE: cannot retrieve the user") - void wait(5000).then(async () => msg.delete()) + void ephemeral(context.reply("Error: cannot find this user")) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/mute.ts` around lines 77 - 97, The handler mixes manual auto-delete logic with ephemeral() calls; replace the manual wait(5000).then(...msg.delete()) flows with the ephemeral wrapper to standardize error replies: when userId is not found (currently creating msg then wait/delete) and when getUser returns null (currently same pattern), call void ephemeral(context.reply(<same message>)) instead, keeping the existing logger.debug/logger.error and the same message text; leave the Moderation.unmute branch using ephemeral as-is. Use the identifiers handler, userId, getTelegramId, getUser, logger.debug, logger.error, ephemeral, context.reply, and Moderation.unmute to locate the spots to change.src/commands/grants.ts (2)
99-103: Potential time inconsistency with separateconversation.now()calls.Lines 99-100 call
conversation.now()twice, which could return different values if there's any delay between calls. Consider capturing the timestamp once.♻️ Suggested fix
- const today = new Date(await conversation.now()) - const startDate = new Date(await conversation.now()) + const now = await conversation.now() + const today = new Date(now) + const startDate = new Date(now)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/grants.ts` around lines 99 - 103, Call conversation.now() only once and reuse that value to avoid inconsistent timestamps: await conversation.now() should be stored in a single variable (e.g. nowTs) and then used to construct today and startDate (replace the two separate calls to conversation.now()). Keep grantDuration, endDate (which uses startDate and grantDuration), and baseMsg/mainMsg logic the same but ensure they reference the single startDate derived from the single now value.
105-130: MissingawaitoneditMessageTextcalls.The
editMessageTextcalls at lines 107, 113, and 128 are not awaited. While these may work as fire-and-forget, the lack ofawaitmeans errors won't propagate and menu navigation could complete before the message is actually updated.♻️ Suggested fix
async function changeDuration(ctx: ConversationMenuContext<ConversationContext<"private">>, durationStr: string) { grantDuration = duration.zod.parse(durationStr) - ctx.editMessageText(baseMsg(), { reply_markup: ctx.msg?.reply_markup }) + await ctx.editMessageText(baseMsg(), { reply_markup: ctx.msg?.reply_markup }) ctx.menu.nav("grants-main") } async function changeStartDate(ctx: ConversationMenuContext<ConversationContext<"private">>, delta: number) { startDate.setDate(today.getDate() + delta) - ctx.editMessageText( + await ctx.editMessageText( fmt(({ skip, b }) => [skip`${baseMsg()}`, b`🕓 Changing start TIME`], { sep: "\n\n" }), { reply_markup: ctx.msg?.reply_markup } ) ctx.menu.nav("grants-start-time") } async function changeStartTime( ctx: ConversationMenuContext<ConversationContext<"private">>, hour: number, minutes: number ) { // TODO: check timezone match between bot and user startDate.setHours(hour) startDate.setMinutes(minutes) - ctx.editMessageText(baseMsg(), { reply_markup: ctx.msg?.reply_markup }) + await ctx.editMessageText(baseMsg(), { reply_markup: ctx.msg?.reply_markup }) ctx.menu.nav("grants-main") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/grants.ts` around lines 105 - 130, The editMessageText calls in changeDuration, changeStartDate, and changeStartTime are not awaited, so await each ctx.editMessageText(...) call to ensure errors propagate and the message update completes before navigating the menu (ctx.menu.nav). Update the three functions (changeDuration, changeStartDate, changeStartTime) to await ctx.editMessageText(...) and only call ctx.menu.nav("grants-main") or ctx.menu.nav("grants-start-time") after the awaited promise resolves.src/commands/index.ts (1)
38-47: Minor: Unhandled rejection on reply deletion.Line 46 calls
reply.delete()without error handling. If the deletion fails (e.g., message already deleted, permissions issue), it could cause an unhandled promise rejection.♻️ Suggested fix
// Inform the user of restricted access const reply = await context.reply("You are not allowed to execute this command") await context.deleteMessage() - void wait(3000).then(() => reply.delete()) + void wait(3000).then(() => reply.delete().catch(() => {})) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/index.ts` around lines 38 - 47, In missingPermissions handler, the reply.delete() call inside wait(3000).then(() => reply.delete()) can throw and cause an unhandled rejection; wrap the deletion in a promise-safe catch (or await it inside a try/catch) so failures are swallowed/logged. Update the code in missingPermissions (the reply creation and the wait(...).then(...) block) to call reply.delete().catch(...) or use try { await reply.delete() } catch (err) { logger.debug(err, "failed to delete reply") } to prevent unhandled promise rejections.src/commands/ban.ts (1)
81-97: Inconsistent error handling pattern inunbanhandler.Lines 84 and 92 use
wait(5000).then(() => msg.delete())while line 97 usesephemeral(). Consider usingephemeral()consistently for all error cases to align with the pattern inbanandtbanhandlers.♻️ Suggested refactor for consistency
if (!userId) { logger.debug(`unban: no userId for username ${args.username}`) - const msg = await context.reply(fmt(({ b }) => b`@${context.from.username} user not found`)) - void wait(5000).then(() => msg.delete()) + void ephemeral(context.reply(fmt(({ b }) => b`@${context.from.username} user not found`))) return } const user = await getUser(userId, context) if (!user) { - const msg = await context.reply("Error: cannot find this user") logger.error({ userId }, "UNBAN: cannot retrieve the user") - void wait(5000).then(() => msg.delete()) + void ephemeral(context.reply("Error: cannot find this user")) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/ban.ts` around lines 81 - 97, The unban handler mixes manual temporary message deletion (wait(5000).then(() => msg.delete())) with ephemeral() usage; make error reporting consistent by replacing the manual reply+wait+delete flows with ephemeral(...) calls: for the "user not found" and "cannot find this user" branches, call ephemeral(context.reply(...)) with the same text used currently (e.g., `@${context.from.username} user not found` and `Error: cannot find this user`) and remove the wait(...) and msg.delete() logic so all errors use ephemeral like the Moderation.unban error branch.src/lib/managed-commands/collection.ts (1)
24-30: Document the flush side effect inwithCollection.Calling
withCollection()flushes the passed collections, marking them as used. This means a collection cannot be shared between multiple parent collections. While this is likely intentional to prevent duplicate registrations, documenting this behavior in the JSDoc would help consumers avoid surprises.📝 Suggested documentation
+ /** + * Merges commands from other collections into this collection. + * + * **Note:** This flushes the passed collections, preventing them from being + * added to other collections. Each collection can only be consumed once. + * + * `@param` collections The collections to merge + * `@returns` `this` instance for chaining + */ withCollection(...collections: CommandsCollection<TRole>[]): CommandsCollection<TRole> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/managed-commands/collection.ts` around lines 24 - 30, The withCollection method implicitly calls flush() on each provided CommandsCollection which mutates those collections (setting flushed and preventing reuse), so update the JSDoc for withCollection to explicitly document this side effect: state that withCollection will call collection.flush() on every passed CommandsCollection<TRole>, that this marks them as flushed/used and makes them ineligible for reuse or sharing with other parents, and mention the thrown Error when this.flushed is true; reference the withCollection method, the flush() method, the flushed flag, and commands.push(...collections.flatMap(...)) so consumers understand the behavioral and mutability implications.src/commands/link-admin-dashboard.ts (1)
112-113: Consider handling potential deletion failure.The
asynckeyword is unnecessary here, and unlike theephemeral()utility used elsewhere, this doesn't catch deletion errors (e.g., message already deleted, bot lacks permissions).♻️ Suggested fix
- void wait(5000).then(async () => msg.delete()) + void wait(5000).then(() => msg.delete().catch(() => {}))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/link-admin-dashboard.ts` around lines 112 - 113, The anonymous async in the wait callback is unnecessary and msg.delete() can throw (message already deleted or missing permissions), so update the wait(...).then(...) callback that calls msg.delete() to remove the async keyword and handle deletion failures: call msg.delete() inside a try/catch (or append .catch()) and swallow or log expected errors rather than letting them bubble; refer to the wait(...).then(...) callback and the msg.delete() invocation when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/index.ts`:
- Around line 48-52: The handlerError closure currently sends raw error text to
users via context.reply(String(error)); change it to reply with a generic
user-facing message (e.g., "An internal error occurred, please try again later")
while retaining the full error details in the logger.error call for diagnostics;
optionally generate or log a correlation/error id to include in the user
message. Locate the handlerError function, update context.reply to a
non-sensitive generic string, and ensure logger.error({ error }, ...) continues
to capture the full error and any telemetry/id you add.
- Around line 60-87: Wrap the external calls inside permissionHandler
(specifically api.tg.permissions.checkGroup.query and
api.tg.permissions.getRoles.query, and the ctx.getChatMember usage) in try/catch
blocks so network/API errors don't crash the handler; on catch, log the error
with the project logger (or console.error) including context (userId, chatId,
function name) and return a safe default (deny access by returning false) or
treat the particular check as failed; also defensively handle missing/undefined
responses from those queries before accessing properties like .status or roles.
In `@src/lib/managed-commands/index.ts`:
- Around line 135-137: Run the code formatter to fix the Biome formatting error
around the generic declaration and implements clause; specifically, format
src/lib/managed-commands/index.ts (the declaration using TRole, C extends
ManagedCommandsFlavor<Context>, and the implements MiddlewareObj<C> clause) by
running `pnpm exec biome format` (or apply the same formatting change: reflow
the generic/type declaration and place `implements MiddlewareObj<C>` according
to project style) and commit the resulting changes so the CI Biome check passes.
- Line 382: Correct the typo in the inline comment before executing the handler
in src/lib/managed-commands/index.ts by changing "Fianlly execute the handler"
to "Finally execute the handler" so the comment reads correctly near the handler
execution point.
---
Nitpick comments:
In `@src/commands/audit.ts`:
- Around line 26-45: The current code builds and replies with the audit list
(using list from api.tg.auditLog.getById.query and formatting via fmt) but does
not handle the empty-list case; update the logic in the handler around list (the
variable produced by api.tg.auditLog.getById.query) to detect when list.length
=== 0 and send a clear reply (via context.reply / same fmt flow) that includes
the header with args.username and an explicit "No audit records found" message
(or similar) instead of the separators-only output, keeping existing formatting
helpers (fmt, fmtDate) and structure for non-empty lists.
In `@src/commands/ban.ts`:
- Around line 81-97: The unban handler mixes manual temporary message deletion
(wait(5000).then(() => msg.delete())) with ephemeral() usage; make error
reporting consistent by replacing the manual reply+wait+delete flows with
ephemeral(...) calls: for the "user not found" and "cannot find this user"
branches, call ephemeral(context.reply(...)) with the same text used currently
(e.g., `@${context.from.username} user not found` and `Error: cannot find this
user`) and remove the wait(...) and msg.delete() logic so all errors use
ephemeral like the Moderation.unban error branch.
In `@src/commands/grants.ts`:
- Around line 99-103: Call conversation.now() only once and reuse that value to
avoid inconsistent timestamps: await conversation.now() should be stored in a
single variable (e.g. nowTs) and then used to construct today and startDate
(replace the two separate calls to conversation.now()). Keep grantDuration,
endDate (which uses startDate and grantDuration), and baseMsg/mainMsg logic the
same but ensure they reference the single startDate derived from the single now
value.
- Around line 105-130: The editMessageText calls in changeDuration,
changeStartDate, and changeStartTime are not awaited, so await each
ctx.editMessageText(...) call to ensure errors propagate and the message update
completes before navigating the menu (ctx.menu.nav). Update the three functions
(changeDuration, changeStartDate, changeStartTime) to await
ctx.editMessageText(...) and only call ctx.menu.nav("grants-main") or
ctx.menu.nav("grants-start-time") after the awaited promise resolves.
In `@src/commands/index.ts`:
- Around line 38-47: In missingPermissions handler, the reply.delete() call
inside wait(3000).then(() => reply.delete()) can throw and cause an unhandled
rejection; wrap the deletion in a promise-safe catch (or await it inside a
try/catch) so failures are swallowed/logged. Update the code in
missingPermissions (the reply creation and the wait(...).then(...) block) to
call reply.delete().catch(...) or use try { await reply.delete() } catch (err) {
logger.debug(err, "failed to delete reply") } to prevent unhandled promise
rejections.
In `@src/commands/link-admin-dashboard.ts`:
- Around line 112-113: The anonymous async in the wait callback is unnecessary
and msg.delete() can throw (message already deleted or missing permissions), so
update the wait(...).then(...) callback that calls msg.delete() to remove the
async keyword and handle deletion failures: call msg.delete() inside a try/catch
(or append .catch()) and swallow or log expected errors rather than letting them
bubble; refer to the wait(...).then(...) callback and the msg.delete()
invocation when making the change.
In `@src/commands/mute.ts`:
- Around line 77-97: The handler mixes manual auto-delete logic with ephemeral()
calls; replace the manual wait(5000).then(...msg.delete()) flows with the
ephemeral wrapper to standardize error replies: when userId is not found
(currently creating msg then wait/delete) and when getUser returns null
(currently same pattern), call void ephemeral(context.reply(<same message>))
instead, keeping the existing logger.debug/logger.error and the same message
text; leave the Moderation.unmute branch using ephemeral as-is. Use the
identifiers handler, userId, getTelegramId, getUser, logger.debug, logger.error,
ephemeral, context.reply, and Moderation.unmute to locate the spots to change.
In `@src/commands/userid.ts`:
- Line 13: Update the username sanitization to use replaceAll instead of replace
for consistency with other commands: locate the const username assignment in
userid.ts (the line declaring const username = args.username.replace("@", ""))
and change it to call replaceAll("@", "") so all occurrences of "@" are removed
and behavior matches role.ts and mute.ts handling.
In `@src/lib/managed-commands/collection.ts`:
- Around line 24-30: The withCollection method implicitly calls flush() on each
provided CommandsCollection which mutates those collections (setting flushed and
preventing reuse), so update the JSDoc for withCollection to explicitly document
this side effect: state that withCollection will call collection.flush() on
every passed CommandsCollection<TRole>, that this marks them as flushed/used and
makes them ineligible for reuse or sharing with other parents, and mention the
thrown Error when this.flushed is true; reference the withCollection method, the
flush() method, the flushed flag, and commands.push(...collections.flatMap(...))
so consumers understand the behavioral and mutability implications.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6459fb0-46a7-4167-ac18-ebc376b33b8d
📒 Files selected for processing (24)
src/commands/_base.tssrc/commands/audit.tssrc/commands/ban.tssrc/commands/banall.tssrc/commands/del.tssrc/commands/grants.tssrc/commands/index.tssrc/commands/kick.tssrc/commands/link-admin-dashboard.tssrc/commands/mute.tssrc/commands/report.tssrc/commands/role.tssrc/commands/search.tssrc/commands/test/args.tssrc/commands/test/db.tssrc/commands/test/format.tssrc/commands/test/index.tssrc/commands/test/menu.tssrc/commands/userid.tssrc/lib/managed-commands/collection.tssrc/lib/managed-commands/command.tssrc/lib/managed-commands/index.tssrc/utils/messages.tssrc/utils/users.ts
💤 Files with no reviewable changes (1)
- src/commands/_base.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/commands/moderation/mute.ts (2)
48-51:⚠️ Potential issue | 🟡 MinorSame typo: "mutate" should be "mute".
Suggested fix
- args: [{ key: "reason", optional: true, description: "Optional reason to mutate the user" }], + args: [{ key: "reason", optional: true, description: "Optional reason to mute the user" }],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/moderation/mute.ts` around lines 48 - 51, The command metadata passed into .createCommand for the "mute" command contains a typo in the args description ("Optional reason to mutate the user"); update that string to read "Optional reason to mute the user" so the args.description for the mute command is correct; locate the .createCommand call where trigger: "mute" and modify the args[0].description accordingly.
14-24:⚠️ Potential issue | 🟡 MinorTypo: "mutate" should be "mute".
The descriptions contain "mutate" instead of "mute" (lines 20, 22).
Suggested fix
{ key: "duration", type: duration.zod, optional: false, - description: `How long to mutate the user. ${duration.formatDesc}`, + description: `How long to mute the user. ${duration.formatDesc}`, }, - { key: "reason", optional: true, description: "Optional reason to mutate the user" }, + { key: "reason", optional: true, description: "Optional reason to mute the user" }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/moderation/mute.ts` around lines 14 - 24, In the tmute command definition update the typo in the description strings where "mutate" is used; change the description for the duration arg (in the args entry with key "duration" and type duration.zod) from "How long to mutate the user." to "How long to mute the user." and update the optional reason arg description (args entry with key "reason") from "Optional reason to mutate the user" to "Optional reason to mute the user", and also update the command-level description from "Temporary mute a user from a group" if needed to ensure consistent wording.src/commands/moderation/banall.ts (1)
71-77:⚠️ Potential issue | 🟡 MinorIncorrect argument description.
The description for the
usernameargument says "user you want to update the role" but this is theunban_allcommand. This appears to be a copy-paste error.Suggested fix
args: [ { key: "username", type: numberOrString, - description: "The username or the user id of the user you want to update the role", + description: "The username or the user id of the user you want to unban from all groups", }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/moderation/banall.ts` around lines 71 - 77, The args entry for the "username" parameter in the unban_all command has an incorrect copy-paste description; update the description text for the args -> username item (in banall.ts / the unban_all command handler) to accurately reflect its purpose — e.g., "The username or user id of the user to unban from all servers" — so the description matches the command behavior and removes the "update the role" wording.
♻️ Duplicate comments (2)
src/commands/index.ts (2)
52-57:⚠️ Potential issue | 🟠 MajorPermission lookups should fail safely (deny + log), not throw.
Lines [53], [55], and [61] call Telegram/API services without guarding failures. A transient network/API error can crash permission evaluation instead of safely denying access.
Suggested fix
overrideGroupAdminCheck: async (userId, groupId, ctx) => { - const { status: groupRole } = await ctx.getChatMember(userId) - if (groupRole === "administrator" || groupRole === "creator") return true - const isDbAdmin = await api.tg.permissions.checkGroup.query({ userId, groupId }) - return isDbAdmin + try { + const { status: groupRole } = await ctx.getChatMember(userId) + if (groupRole === "administrator" || groupRole === "creator") return true + return await api.tg.permissions.checkGroup.query({ userId, groupId }) + } catch (error) { + logger.error({ error, userId, groupId }, "[ManagedCommands] overrideGroupAdminCheck failed") + return false + } }, }, getUserRoles: async (userId) => { // TODO: cache this to avoid hitting the db on every command - const { roles } = await api.tg.permissions.getRoles.query({ userId }) - return roles || [] + try { + const { roles } = await api.tg.permissions.getRoles.query({ userId }) + return roles || [] + } catch (error) { + logger.error({ error, userId }, "[ManagedCommands] getUserRoles failed") + return [] + } },Also applies to: 59-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/index.ts` around lines 52 - 57, The permission lookup in overrideGroupAdminCheck can throw when calling ctx.getChatMember or api.tg.permissions.checkGroup.query; wrap both calls in try/catch blocks so any error is caught, logged (via your logger or ctx.logger), and the function returns false (deny) instead of propagating the exception; specifically, protect the call to ctx.getChatMember(userId) and the subsequent api.tg.permissions.checkGroup.query({ userId, groupId }) call, logging the error with context (userId, groupId) and returning false on failure.
41-45:⚠️ Potential issue | 🟠 MajorDo not expose raw exception text to users.
At Line [44], replying with
String(error)can leak internal details. Keep full error only in logs and return a generic user message.Suggested fix
handlerError: async ({ context, command, error }) => { logger.error({ error }, `[ManagedCommands] Error in handler for command '/${command.trigger}'`) // TODO: we should figure out what to tell the user, maybe if we have some telemetry we can produce an error report id here? - await context.reply(`An error occurred: ${String(error)}`) + await context.reply("An internal error occurred while processing your command. Please try again later.") },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/index.ts` around lines 41 - 45, The handlerError in src/commands/index.ts currently replies to users with String(error), which may leak internal details; keep the full error in the logger (logger.error({ error }, ...)) but change the user-facing reply in handlerError to a generic message (e.g., "An unexpected error occurred; please try again later" or include a safe error/report id if you have telemetry), removing String(error) from the reply; update the handlerError block (the async function handling { context, command, error }) to send only the generic text to context.reply while retaining the detailed error in the log.
🧹 Nitpick comments (10)
src/lib/managed-commands/index.ts (1)
268-270: Use a collision-safe conversation ID for multi-trigger commands.
join("_")can collide for different trigger sets (e.g.,["a_b", "c"]vs["a", "b_c"]), risking conversation ID overlap.♻️ Suggested refactor
private static commandID(cmd: AnyCommand) { - return typeof cmd.trigger === "string" ? cmd.trigger : cmd.trigger.join("_") + const triggers = Array.isArray(cmd.trigger) ? cmd.trigger : [cmd.trigger] + return `cmd:${triggers.map(encodeURIComponent).join("|")}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/managed-commands/index.ts` around lines 268 - 270, commandID currently builds IDs using typeof cmd.trigger === "string" ? cmd.trigger : cmd.trigger.join("_"), which can produce collisions for multi-trigger arrays; update the commandID(AnyCommand) implementation to produce an unambiguous ID by encoding array triggers uniquely (for example, when cmd.trigger is an array use JSON.stringify(cmd.trigger) or a stable hash of the joined components) while preserving the original string trigger behavior for string triggers; ensure you reference the commandID function and the cmd.trigger value when making the change so all callers continue to receive the new collision-safe ID format.src/commands/management/grants.ts (3)
105-109: Missingawaitonctx.editMessageText.
editMessageTextreturns a Promise. While fire-and-forget may be intentional here, errors would be silently swallowed. Consider addingawaitor explicitly voiding the promise:async function changeDuration(ctx: ConversationMenuContext<ConversationContext<"private">>, durationStr: string) { grantDuration = duration.zod.parse(durationStr) - ctx.editMessageText(baseMsg(), { reply_markup: ctx.msg?.reply_markup }) + await ctx.editMessageText(baseMsg(), { reply_markup: ctx.msg?.reply_markup }) ctx.menu.nav("grants-main") }The same pattern appears in
changeStartDate(line 113) andchangeStartTime(line 128).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/management/grants.ts` around lines 105 - 109, In changeDuration, changeStartDate, and changeStartTime, ctx.editMessageText returns a Promise that is currently not awaited; update each handler (changeDuration, changeStartDate, changeStartTime) to either await ctx.editMessageText(...) or explicitly void the returned promise (e.g., void ctx.editMessageText(...)) so errors aren't silently swallowed and the edit completes before navigating with ctx.menu.nav("grants-main") or similar navigation.
150-154: MissingawaitondeleteMessage().Similar to above,
res.deleteMessage()is not awaited. If deletion fails, the error is silently lost. Consider:do { const res = await conversation.waitFor(":text") - res.deleteMessage() + void res.deleteMessage() text = res.msg.text } while (!duration.zod.safeParse(text).success)Using
voidmakes the fire-and-forget intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/management/grants.ts` around lines 150 - 154, The loop calls res.deleteMessage() without awaiting it, which can drop errors; update the block around conversation.waitFor(":text") so the deletion is awaited or explicitly fire-and-forget using void (e.g., await res.deleteMessage() or void res.deleteMessage()), keeping the rest of the logic (text = res.msg.text and duration.zod.safeParse(text)) intact to ensure failures are not silently lost.
161-161: Misleading underscore prefix on used variable.
_startTimeMenuis prefixed with an underscore (typically indicating unused), but it's actually used—conversation.menu()registers it for the menu system. Consider removing the underscore to avoid confusion:- const _startTimeMenu = conversation + const startTimeMenu = conversation .menu("grants-start-time", { parent: "grants-main" })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/management/grants.ts` at line 161, The variable _startTimeMenu is misleadingly prefixed with an underscore despite being used by the menu system; rename it to startTimeMenu (remove the leading underscore) and update all references where it is registered with conversation.menu() so the declaration and any uses match (e.g., update the const _startTimeMenu declaration and the call conversation.menu(_startTimeMenu) to use startTimeMenu).src/commands/moderation/mute.ts (2)
83-83: Unnecessaryasyncin arrow function.Similar to
link-admin-dashboard.ts, theasynckeyword is redundant here.Suggested fix
- void wait(5000).then(async () => msg.delete()) + void wait(5000).then(() => msg.delete())🤖 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 83, The arrow function passed to the Promise returned by wait() in mute command is marked async but does not use await; remove the redundant async from the callback in the expression using wait(5000).then(async () => msg.delete()) so it becomes a plain function that calls msg.delete(), updating the code around the wait(...) invocation in the mute handler (reference: wait and msg.delete).
91-91: Same unnecessaryasync.Suggested fix
- void wait(5000).then(async () => msg.delete()) + void wait(5000).then(() => msg.delete())🤖 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 91, The then callback passed to wait is declared async but does not use await, so remove the unnecessary async to avoid misleading async behavior; update the call site where wait(5000).then(async () => msg.delete()) is used (referencing the wait function and the msg variable) to a non-async callback such as then(() => msg.delete()), or if you intended to await msg.delete() make the surrounding function async and use await wait(5000); await msg.delete(); instead.src/commands/link-admin-dashboard.ts (2)
37-37: AnonymousCommandsCollectioninstantiation.Unlike other command files in this PR (e.g.,
ban.tsuses"Banning",mute.tsuses"Muting"), this creates an unnamed collection. Consider adding a descriptive name for consistency and debugging clarity:-export const linkAdminDashboard = new CommandsCollection<Role>().createCommand({ +export const linkAdminDashboard = new CommandsCollection<Role>("Link Admin Dashboard").createCommand({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/link-admin-dashboard.ts` at line 37, The CommandsCollection is instantiated without a descriptive name in the export linkAdminDashboard (new CommandsCollection<Role>()), so update the instantiation to pass a clear name string (e.g., "Linking" or "AdminDashboard") consistent with other files like ban.ts and mute.ts; locate the new CommandsCollection<Role>() call in linkAdminDashboard and change it to new CommandsCollection<Role>("YourChosenName") so the collection is named for logging and debugging.
112-112: Unnecessaryasyncin arrow function.The
asynckeyword is redundant here sincemsg.delete()already returns a Promise, and the outer promise is discarded viavoid.Suggested fix
- void wait(5000).then(async () => msg.delete()) + void wait(5000).then(() => msg.delete())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/link-admin-dashboard.ts` at line 112, The arrow function passed to then contains an unnecessary async; remove the async keyword and return the Promise directly so the call becomes void wait(5000).then(() => msg.delete()); update the anonymous arrow used with wait(...).then to drop async and ensure msg.delete() is returned (no additional await/try needed).src/commands/moderation/ban.ts (1)
77-98: Inconsistent error handling pattern.The
unbancommand mixes two patterns for ephemeral messages:
- Lines 84, 92: Manual
void wait(5000).then(() => msg.delete())- Line 97:
void ephemeral(context.reply(...))Consider using
ephemeral()consistently for all temporary error messages in this handler for uniformity with the rest of the file.🤖 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 77 - 98, The handler function for the unban command mixes manual temporary-message deletion with the ephemeral helper; replace the manual patterns so all temporary error replies use ephemeral(context.reply(...)) for consistency. Specifically, in the handler where you check for missing userId (currently creating msg and calling wait(...).then(() => msg.delete())) and where getUser returns null (same manual delete), change those to call ephemeral(context.reply(...)) with the same message text; leave the call to Moderation.unban and its existing ephemeral usage unchanged. Ensure imports/usages of ephemeral remain available and remove the now-unneeded wait-based delete calls and local msg variables.src/commands/management/audit.ts (1)
26-45: Consider handling empty audit logs.If
listis empty, the reply will display only the header and separator lines with no entries. Consider adding a message when no audit entries exist:Suggested improvement
const list = await api.tg.auditLog.getById.query({ targetId: userId }) + if (list.length === 0) { + await context.reply(fmt(({ b }) => b`🧾 Audit Log: ${args.username}\n\nNo entries found.`)) + return + } await context.reply(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/management/audit.ts` around lines 26 - 45, The reply builds an audit log view from the variable list returned by api.tg.auditLog.getById.query but doesn't handle the empty-array case; add a guard using list.length (or list?.length) before calling the fmt/context.reply flow and send a concise fallback message like "No audit entries for this user" (or include that line in the formatted output when list is empty) so users don't see only headers/separators; update the logic around the list variable and the fmt/context.reply call to return the fallback when list is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/index.ts`:
- Around line 25-26: The wrongScope hook's await context.deleteMessage() can
throw and currently aborts the hook; wrap the call in a try/catch (following the
same pattern used in beforeHandler) so deletion is best-effort: call
context.deleteMessage() inside try, ignore/log any error in catch (don’t
rethrow), and ensure the subsequent logging and remaining logic in wrongScope
still run; reference the wrongScope function and context.deleteMessage to locate
where to add the try/catch.
- Around line 37-40: The delayed cleanup of the temporary reply uses
wait(3000).then(() => reply.delete()) which can reject and produce an unhandled
promise; update that chain to swallow errors like the pattern used elsewhere by
appending .catch(() => {}) to the promise returned by wait(...) so that
reply.delete() failures are ignored; locate the code around the reply variable
and the wait(...) call in src/commands/index.ts and modify the promise chain to
include .catch(() => {}).
In `@src/commands/management/audit.ts`:
- Line 10: Update the description string for the audit command to fix the
grammar: replace the current description value "Get audit of an user" with a
corrected phrase such as "Get audit of a user" or "Get the audit log for a user"
in the command definition's description property in
src/commands/management/audit.ts (the description field of the audit command).
In `@src/lib/managed-commands/index.ts`:
- Around line 376-381: The permission checks are applying group-only filters
(allowedGroupsId/excludedGroupsId) even for private chats when a command has
scope "both"; update the logic inside isAllowedInGroups handling in
managed-commands/index.ts so that allowedGroupsId and excludedGroupsId are only
evaluated when the current chat is a group (e.g., check ctx.isGroup or
ctx.chatType === 'group') or when the command scope is explicitly group;
specifically modify the block that reads const { allowGroupAdmins,
allowedGroupsId, excludedGroupsId } = command.permissions to skip the
allowedGroupsId/includes(ctx.chatId) and excludedGroupsId.includes(ctx.chatId)
checks unless the chat is a group, ensuring private chats are not incorrectly
denied.
- Around line 339-341: The lookup only checks equality against a string trigger
(cmdArg) so commands whose trigger is an array (aliases) are never matched;
update the search in the block that uses this.getCommands() so it tests both
forms: if c.trigger is a string compare equality (case-insensitive) to cmdArg,
and if c.trigger is an array use c.trigger.includes(cmdArg) (or check c.aliases
if your Command type uses that property), returning the matched command; apply
the same matching logic wherever trigger matching is needed (e.g., in the
getCommands().find callback that references cmdArg).
In `@src/utils/duration.ts`:
- Line 25: The durationRegex currently lacks start/end anchors, allowing
substring matches (e.g., "abc5m"); update the regex referenced by durationRegex
and used in the .regex(...) validation call so it is anchored (add ^ at the
start and $ at the end) to enforce exact-match validation per the error message;
ensure the symbol durationRegex (and the .regex(durationRegex, "...") usage) is
updated accordingly so only strings matching the full "<number><unit>" pattern
pass.
---
Outside diff comments:
In `@src/commands/moderation/banall.ts`:
- Around line 71-77: The args entry for the "username" parameter in the
unban_all command has an incorrect copy-paste description; update the
description text for the args -> username item (in banall.ts / the unban_all
command handler) to accurately reflect its purpose — e.g., "The username or user
id of the user to unban from all servers" — so the description matches the
command behavior and removes the "update the role" wording.
In `@src/commands/moderation/mute.ts`:
- Around line 48-51: The command metadata passed into .createCommand for the
"mute" command contains a typo in the args description ("Optional reason to
mutate the user"); update that string to read "Optional reason to mute the user"
so the args.description for the mute command is correct; locate the
.createCommand call where trigger: "mute" and modify the args[0].description
accordingly.
- Around line 14-24: In the tmute command definition update the typo in the
description strings where "mutate" is used; change the description for the
duration arg (in the args entry with key "duration" and type duration.zod) from
"How long to mutate the user." to "How long to mute the user." and update the
optional reason arg description (args entry with key "reason") from "Optional
reason to mutate the user" to "Optional reason to mute the user", and also
update the command-level description from "Temporary mute a user from a group"
if needed to ensure consistent wording.
---
Duplicate comments:
In `@src/commands/index.ts`:
- Around line 52-57: The permission lookup in overrideGroupAdminCheck can throw
when calling ctx.getChatMember or api.tg.permissions.checkGroup.query; wrap both
calls in try/catch blocks so any error is caught, logged (via your logger or
ctx.logger), and the function returns false (deny) instead of propagating the
exception; specifically, protect the call to ctx.getChatMember(userId) and the
subsequent api.tg.permissions.checkGroup.query({ userId, groupId }) call,
logging the error with context (userId, groupId) and returning false on failure.
- Around line 41-45: The handlerError in src/commands/index.ts currently replies
to users with String(error), which may leak internal details; keep the full
error in the logger (logger.error({ error }, ...)) but change the user-facing
reply in handlerError to a generic message (e.g., "An unexpected error occurred;
please try again later" or include a safe error/report id if you have
telemetry), removing String(error) from the reply; update the handlerError block
(the async function handling { context, command, error }) to send only the
generic text to context.reply while retaining the detailed error in the log.
---
Nitpick comments:
In `@src/commands/link-admin-dashboard.ts`:
- Line 37: The CommandsCollection is instantiated without a descriptive name in
the export linkAdminDashboard (new CommandsCollection<Role>()), so update the
instantiation to pass a clear name string (e.g., "Linking" or "AdminDashboard")
consistent with other files like ban.ts and mute.ts; locate the new
CommandsCollection<Role>() call in linkAdminDashboard and change it to new
CommandsCollection<Role>("YourChosenName") so the collection is named for
logging and debugging.
- Line 112: The arrow function passed to then contains an unnecessary async;
remove the async keyword and return the Promise directly so the call becomes
void wait(5000).then(() => msg.delete()); update the anonymous arrow used with
wait(...).then to drop async and ensure msg.delete() is returned (no additional
await/try needed).
In `@src/commands/management/audit.ts`:
- Around line 26-45: The reply builds an audit log view from the variable list
returned by api.tg.auditLog.getById.query but doesn't handle the empty-array
case; add a guard using list.length (or list?.length) before calling the
fmt/context.reply flow and send a concise fallback message like "No audit
entries for this user" (or include that line in the formatted output when list
is empty) so users don't see only headers/separators; update the logic around
the list variable and the fmt/context.reply call to return the fallback when
list is empty.
In `@src/commands/management/grants.ts`:
- Around line 105-109: In changeDuration, changeStartDate, and changeStartTime,
ctx.editMessageText returns a Promise that is currently not awaited; update each
handler (changeDuration, changeStartDate, changeStartTime) to either await
ctx.editMessageText(...) or explicitly void the returned promise (e.g., void
ctx.editMessageText(...)) so errors aren't silently swallowed and the edit
completes before navigating with ctx.menu.nav("grants-main") or similar
navigation.
- Around line 150-154: The loop calls res.deleteMessage() without awaiting it,
which can drop errors; update the block around conversation.waitFor(":text") so
the deletion is awaited or explicitly fire-and-forget using void (e.g., await
res.deleteMessage() or void res.deleteMessage()), keeping the rest of the logic
(text = res.msg.text and duration.zod.safeParse(text)) intact to ensure failures
are not silently lost.
- Line 161: The variable _startTimeMenu is misleadingly prefixed with an
underscore despite being used by the menu system; rename it to startTimeMenu
(remove the leading underscore) and update all references where it is registered
with conversation.menu() so the declaration and any uses match (e.g., update the
const _startTimeMenu declaration and the call conversation.menu(_startTimeMenu)
to use startTimeMenu).
In `@src/commands/moderation/ban.ts`:
- Around line 77-98: The handler function for the unban command mixes manual
temporary-message deletion with the ephemeral helper; replace the manual
patterns so all temporary error replies use ephemeral(context.reply(...)) for
consistency. Specifically, in the handler where you check for missing userId
(currently creating msg and calling wait(...).then(() => msg.delete())) and
where getUser returns null (same manual delete), change those to call
ephemeral(context.reply(...)) with the same message text; leave the call to
Moderation.unban and its existing ephemeral usage unchanged. Ensure
imports/usages of ephemeral remain available and remove the now-unneeded
wait-based delete calls and local msg variables.
In `@src/commands/moderation/mute.ts`:
- Line 83: The arrow function passed to the Promise returned by wait() in mute
command is marked async but does not use await; remove the redundant async from
the callback in the expression using wait(5000).then(async () => msg.delete())
so it becomes a plain function that calls msg.delete(), updating the code around
the wait(...) invocation in the mute handler (reference: wait and msg.delete).
- Line 91: The then callback passed to wait is declared async but does not use
await, so remove the unnecessary async to avoid misleading async behavior;
update the call site where wait(5000).then(async () => msg.delete()) is used
(referencing the wait function and the msg variable) to a non-async callback
such as then(() => msg.delete()), or if you intended to await msg.delete() make
the surrounding function async and use await wait(5000); await msg.delete();
instead.
In `@src/lib/managed-commands/index.ts`:
- Around line 268-270: commandID currently builds IDs using typeof cmd.trigger
=== "string" ? cmd.trigger : cmd.trigger.join("_"), which can produce collisions
for multi-trigger arrays; update the commandID(AnyCommand) implementation to
produce an unambiguous ID by encoding array triggers uniquely (for example, when
cmd.trigger is an array use JSON.stringify(cmd.trigger) or a stable hash of the
joined components) while preserving the original string trigger behavior for
string triggers; ensure you reference the commandID function and the cmd.trigger
value when making the change so all callers continue to receive the new
collision-safe ID format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b9bf64a-86a0-4643-ac2c-8fbbba7ab8a9
📒 Files selected for processing (19)
src/commands/index.tssrc/commands/link-admin-dashboard.tssrc/commands/management/audit.tssrc/commands/management/grants.tssrc/commands/management/index.tssrc/commands/management/role.tssrc/commands/management/userid.tssrc/commands/moderation/ban.tssrc/commands/moderation/banall.tssrc/commands/moderation/del.tssrc/commands/moderation/index.tssrc/commands/moderation/kick.tssrc/commands/moderation/mute.tssrc/commands/report.tssrc/commands/search.tssrc/lib/managed-commands/command.tssrc/lib/managed-commands/index.tssrc/utils/duration.tssrc/utils/types.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/commands/index.ts (2)
51-61:⚠️ Potential issue | 🟠 MajorGuard permission API checks against transport/runtime failures.
Calls on Lines 52, 54, and 60 can throw and bubble out of permission evaluation. Permission checks should fail closed (
false/ empty roles), not fail hard.💡 Proposed fix
overrideGroupAdminCheck: async (userId, groupId, ctx) => { - const { status: groupRole } = await ctx.getChatMember(userId) + const { status: groupRole } = await ctx.getChatMember(userId).catch(() => ({ status: "member" as const })) if (groupRole === "administrator" || groupRole === "creator") return true - const isDbAdmin = await api.tg.permissions.checkGroup.query({ userId, groupId }) + const isDbAdmin = await api.tg.permissions.checkGroup.query({ userId, groupId }).catch(() => false) return isDbAdmin }, }, getUserRoles: async (userId) => { // TODO: cache this to avoid hitting the db on every command - const { roles } = await api.tg.permissions.getRoles.query({ userId }) + const { roles } = await api.tg.permissions.getRoles.query({ userId }).catch(() => ({ roles: null })) return roles || [] },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/index.ts` around lines 51 - 61, Wrap the external permission/transport calls in safe try/catch blocks so permission evaluation fails closed: in overrideGroupAdminCheck (which calls ctx.getChatMember and api.tg.permissions.checkGroup.query) catch any thrown errors and return false (optionally log the error), and in getUserRoles (which calls api.tg.permissions.getRoles.query) catch errors and return an empty array (optionally log). Ensure you only change behavior for exceptions (preserve existing successful return values) and reference overrideGroupAdminCheck, ctx.getChatMember, api.tg.permissions.checkGroup.query, getUserRoles, and api.tg.permissions.getRoles.query when locating the changes.
40-44:⚠️ Potential issue | 🟠 MajorDo not expose raw exception text to users.
On Line 43, returning
String(error)can leak internals. Keep full details in logs and send a generic user-facing message instead.💡 Proposed fix
- await context.reply(`An error occurred: ${String(error)}`) + await context.reply("An internal error occurred while processing your command. Please try again later.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/index.ts` around lines 40 - 44, The handlerError currently sends the raw exception text to users via context.reply(String(error)); update handlerError so it does NOT expose internals: keep the full error details in the server log (use logger.error({ error }, ... ) as-is or include stack), but change the context.reply call to send a generic user-facing message (e.g., "An unexpected error occurred. Please try again later."), optionally include a short error/incident id if you emit telemetry; ensure you reference the handlerError function and the context.reply invocation when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/index.ts`:
- Around line 31-39: The missingPermissions handler currently awaits
context.deleteMessage which can throw and abort the denied-access flow; change
it to be best-effort like the other hooks by wrapping the deletion in a
try/catch (or not awaiting it) so errors are swallowed and do not interrupt
execution: inside missingPermissions (referencing missingPermissions,
context.deleteMessage, ephemeral, logger.info, printCtxFrom) call
context.deleteMessage in a try { await context.deleteMessage() } catch (_) { /*
ignore */ } or simply trigger it without await to ensure the user reply is sent
and errors from deleteMessage don’t break the flow.
In `@src/commands/management/audit.ts`:
- Around line 16-19: The code uses parseInt(args.username, 10) which accepts
numeric prefixes and can yield partial matches—change the parsing to strict
numeric validation: first check args.username against a full-digit regex (e.g.
/^\d+$/) and only then convert to a Number (or Number.parseInt with assurance)
to assign userId; if the string is not a pure integer, call
getTelegramId(args.username) as currently done. Update the logic around userId,
parseInt, and getTelegramId to use this strict check so only fully numeric
usernames become numeric userId values.
In `@src/lib/managed-commands/index.ts`:
- Around line 268-270: The current commandID function can produce colliding IDs
(e.g., "a_b" vs ["a","b"]); update commandID (the private static method
commandID in the managed-commands index) to produce an unambiguous
representation of cmd.trigger — e.g., serialize the trigger with its type
information (use a stable serializer such as JSON.stringify on cmd.trigger or
prefix string vs array) so string triggers and array triggers cannot produce the
same ID.
- Around line 372-401: The permission check currently can throw from external
calls (overrideGroupAdminCheck, ctx.getChatMember, getUserRoles, or
isAllowedInGroups) which will crash handling; update checkPermissions to catch
exceptions from those calls and fail closed by returning false (and optionally
logging the error). Concretely, wrap the await
this.hooks.overrideGroupAdminCheck(ctx.from.id, ctx.chatId, ctx) call and the
await ctx.getChatMember(ctx.from.id) call in try/catch blocks that return false
on error, and wrap the await this.getUserRoles(ctx.from.id, ctx) call in a
try/catch that returns false if it throws; also guard isAllowedInGroups usage if
it can throw. Ensure you still return true where logic requires (e.g., when
admin check succeeds), but any exception from external providers should result
in returning false from checkPermissions.
---
Duplicate comments:
In `@src/commands/index.ts`:
- Around line 51-61: Wrap the external permission/transport calls in safe
try/catch blocks so permission evaluation fails closed: in
overrideGroupAdminCheck (which calls ctx.getChatMember and
api.tg.permissions.checkGroup.query) catch any thrown errors and return false
(optionally log the error), and in getUserRoles (which calls
api.tg.permissions.getRoles.query) catch errors and return an empty array
(optionally log). Ensure you only change behavior for exceptions (preserve
existing successful return values) and reference overrideGroupAdminCheck,
ctx.getChatMember, api.tg.permissions.checkGroup.query, getUserRoles, and
api.tg.permissions.getRoles.query when locating the changes.
- Around line 40-44: The handlerError currently sends the raw exception text to
users via context.reply(String(error)); update handlerError so it does NOT
expose internals: keep the full error details in the server log (use
logger.error({ error }, ... ) as-is or include stack), but change the
context.reply call to send a generic user-facing message (e.g., "An unexpected
error occurred. Please try again later."), optionally include a short
error/incident id if you emit telemetry; ensure you reference the handlerError
function and the context.reply invocation when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ce06687-b31e-4af7-918d-4d28e72c4269
📒 Files selected for processing (3)
src/commands/index.tssrc/commands/management/audit.tssrc/lib/managed-commands/index.ts
lorenzocorallo
approved these changes
Mar 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.