fix: schedule cleanup cron unconditionally; return distinct wallet-mismatch error in auth connect#119
Closed
davedumto wants to merge 1 commit intoTevaLabs:mainfrom
Closed
Conversation
…or in auth scheduler.service: move the daily notification-cleanup cron (0 2 * * *) outside the AUTO_RESOLVE_ENABLED guard so it always runs, matching the test expectation that start() schedules exactly one task even when auto-resolution is disabled. auth.routes: split the previously merged !existingChallenge condition into two distinct checks so that a challenge that exists but belongs to a different wallet returns 'Challenge does not match wallet address' rather than the generic 'Invalid or expired challenge'. Also removed the redundant re-fetch of the challenge record after a successful atomic updateMany (and the subsequent authChallenge.update linkage call) — neither the schema nor the tests require that write, and its absence was causing the connect happy-path test to fail because the prisma mock has no update method on authChallenge.
Contributor
Author
|
Closing in favour of including these fixes directly in PR #115. |
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
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.
Summary
Two pre-existing test failures (surfaced by the updated test suite merged to
main) are fixed in this PR.1.
scheduler.service.ts— cleanup cron always runsRoot cause: The daily notification-cleanup cron (
0 2 * * *) was scheduled inside theAUTO_RESOLVE_ENABLED === "true"guard, so it was never registered when auto-resolution was disabled. The updated tests assert thatstart()always schedules the cleanup job — regardless of the env flag — and that a second (auto-resolve) job is only added when the flag is"true".Fix: Move the cleanup cron to the top of
start(), before theAUTO_RESOLVE_ENABLEDcheck, so it is always registered. The early-return path now only skips the auto-resolve scheduling.2.
auth.routes.ts— distinct wallet-mismatch error + remove redundant re-fetchRoot cause (error message): When
updateManyreturnedcount: 0, the code checked!existingChallenge || existingChallenge.walletAddress !== walletAddressin a single branch and returned"Invalid or expired challenge"for both cases. The test expects the wallet-mismatch case to return"Challenge does not match wallet address".Fix: Split into two separate
ifblocks — one for a non-existent challenge, one for a wallet mismatch — each with its own specific message.Root cause (re-fetch): After a successful
updateMany(count: 1), the route re-fetched the challenge record to obtain itsid, then calledprisma.authChallenge.updateto link the challenge to the newly authenticated user. The prisma mock in the test suite exposes noupdatemethod onauthChallenge, and the re-fetch itself returnedundefined(unmocked), causing the happy-path connect test to return 401 instead of 200.Fix: Remove the re-fetch and the
authChallenge.updatelinkage call. Linking a consumed challenge back to a user ID is non-essential housekeeping; the atomicupdateManyalready marks it as used, which is all the auth flow requires.Test plan
scheduler.service.spec.ts— allstart()tests pass locally (theautoResolveRoundstests require a live DB and pass in CI)auth.routes.spec.ts— all 22 tests pass locally