fix: require() crash in scheduler + unused imports (CodeQL #337, #338)#66
Merged
Sam-Aitech merged 1 commit intoJun 25, 2026
Conversation
scheduler.ts called runConsolidatedNotificationJob via require() inside a cron callback. The server is built as ESM (package.json type: module), require is undefined here, and the cron only fires at 07:00/19:00 UTC — so it would throw at the worst possible time instead of at boot. sponsorMonitorJob.ts: removing the legacy inline notification dispatch (a36da0f) left notifyUsersOfEvent, getNotificationQueue, and NOTIFICATION_JOB unused — flagged by CodeQL (#337, #338).
|
Sam-Aitech
deleted the
fix/consolidated-notifications-require-and-unused-imports
branch
June 27, 2026 20:46
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
1 participant
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.
Why
Reviewing the consolidated-notification-engine work surfaced one runtime bug and the two open CodeQL findings.
What
scheduler.ts—runConsolidatedNotificationJobwas loaded viarequire()inside the cron callback. This project is ESM (package.jsontype: module);requireis undefined there. Since it's inside the callback, not at module load, it wouldn't throw at boot — it throws the first time the cron actually fires (07:00/19:00 UTC), so it'd ship clean and fail silently in production until someone checked logs. Replaced with a top-level import, and added the missing.catch()to match every other job registration in this file.sponsorMonitorJob.ts— the legacy inline notification dispatch was removed in a36da0f, which leftnotifyUsersOfEvent,getNotificationQueue, andNOTIFICATION_JOBwith no call sites. This is exactly what CodeQL #337 and #338 are flagging. Removed both;processQueuedEngineEvents(same import line) is still used elsewhere and is kept.Verification
grepconfirms zero remaining references to the three removed namesnpx tsc --noEmit— the one error present (web-pushmissing types) reproduces identically on unmodifiedmain; confirmed pre-existing, unrelated to this changenpx vitest run— 331/331 (pre-commit hook)Closes CodeQL #337, #338 (will auto-resolve once this merges and a scan runs).