Skip to content

fix: seven bugs found in deep code review - #168

Closed
dylanneve1 wants to merge 1 commit into
mainfrom
claude/eager-sagan-dJv5V
Closed

dylanneve1 wants to merge 1 commit into
mainfrom
claude/eager-sagan-dJv5V

Conversation

@dylanneve1

Copy link
Copy Markdown
Owner

Summary

Deep review of the full src/ tree, finding and fixing seven bugs ranging from a user-triggerable null dereference crash to an access-control bypass and an unbounded async hang that would permanently deadlock the dream subsystem.

Bug fixes

# File Severity Issue
1 src/storage/history.ts Low Wrong log component label "sessions" in the corrupt-data fallback β€” copy-paste from sessions.ts
2 src/frontend/telegram/handlers.ts Medium handleStickerMessage was missing the isUserRateLimited check present on every other message handler, allowing unlimited AI-triggering sticker spam from one user
3 src/core/dream.ts Medium executeDream wrote last_run = Date.now() at run start instead of preserving the previous completed timestamp. A crash mid-dream would suppress the next scheduled dream for up to 12 h, even if the last successful dream was much earlier. Mirrors the correct previousLastRun + last_started pattern in heartbeat.ts
4 src/core/dream.ts High No AbortController passed to query(), so on timeout the SDK subprocess received no cancellation signal. The post-timeout await agentPromise.catch(() => {}) was also unbounded β€” if the SDK ignored the timeout it would hang forever, holding the dreaming mutex and preventing all future dream runs (including forced /dream commands). Fixed by wiring an AbortController, calling abort() on timeout, and replacing the unbounded await with a DREAM_ABORT_GRACE_MS (30 s) raceWithTimeout, matching the heartbeat.ts pattern
5 src/frontend/telegram/callbacks.ts Medium/Security The catch-all callback_query:data handler forwarded unrecognised callbacks directly to handleCallbackQuery (AI backend) without calling isAccessAllowed first β€” any user who knows a callback_data string could bypass access control and trigger AI queries
6 src/core/gateway.ts Medium AbortError constructed with a TalonError object instead of a string, causing p-retry to record "[object Object]" as the error message. Also tightened the chatId guard from !chatId (falsy, incorrectly rejects 0) to chatId == null
7 src/storage/cron-store.ts High validateCronExpression cast cron.nextRun() directly to Date without checking for null. croner returns null when an expression has no future occurrences (e.g. a one-shot schedule in the past), causing an unhandled TypeError on a user-supplied input. Now returns a proper validation error

Test plan

  • Sticker messages from rate-limited users are silently dropped (no AI response)
  • Dream state last_run in dream_state.json is unchanged after a failed/crashed dream run; only last_started advances
  • /dream forced run aborts cleanly after timeout; subsequent /dream calls succeed (mutex released)
  • Unknown inline keyboard button presses from unauthorised users receive no AI response
  • Scheduling a cron job with a past-only expression returns a friendly error instead of crashing
  • withRetry exhausted-retry log shows the original error message, not [object Object]
  • Log search for [history] shows the correct component label on corrupt-data recovery

https://claude.ai/code/session_01D4EX7RLNwAdyh2iZi9qYmZ


Generated by Claude Code

1. history.ts β€” wrong log component label ("sessions" instead of "history")
   in the corrupt-data fallback path; a copy-paste error from sessions.ts.

2. handlers.ts β€” handleStickerMessage was missing the isUserRateLimited check
   that every other message handler has, allowing unlimited sticker-triggered
   AI queries from a single user.

3. dream.ts β€” executeDream wrote last_run = now at the start of a run rather
   than preserving the previous completed-run timestamp. A crash mid-dream
   would suppress the next scheduled dream for up to 12 h from the crash start
   even if the last successful dream was much earlier. Mirrors the correct
   pattern in heartbeat.ts (previousLastRun + separate last_started field).

4. dream.ts β€” runDreamAgent had no AbortController wired to the query() call,
   so on timeout the SDK subprocess received no cancellation signal. The
   post-timeout await agentPromise.catch(() => {}) was also unbounded β€” if the
   SDK ignored the timeout it would hang forever, holding the dreaming mutex
   and preventing all future dream runs. Fixed by: adding AbortController,
   calling abort() on timeout, and replacing the unbounded await with a
   30-second raceWithTimeout grace period (same pattern as heartbeat.ts).

5. callbacks.ts β€” the catch-all callback_query handler forwarded unrecognised
   callbacks directly to handleCallbackQuery (AI backend) without calling
   isAccessAllowed first, allowing any user who knows a callback_data string
   to bypass access control and trigger AI queries.

6. gateway.ts β€” AbortError was constructed with a TalonError object rather
   than a string message, causing p-retry to serialise it as "[object Object]"
   and lose the original error detail. Also tightened the chatId null guard
   from falsy (!chatId) to strict null check (chatId == null) to avoid
   incorrectly rejecting a numeric chatId of 0.

7. cron-store.ts β€” validateCronExpression cast cron.nextRun() directly to Date
   without checking for null; croner returns null when an expression has no
   future occurrences, causing a TypeError thrown at the user on a valid but
   exhausted schedule. Now returns a proper validation error instead.

https://claude.ai/code/session_01D4EX7RLNwAdyh2iZi9qYmZ
@dylanneve1

Copy link
Copy Markdown
Owner Author

Superseded by #246. I reviewed the old changes and ported the valid/current fixes onto current main in the consolidated PR, while leaving out stale removed-backend changes and broad churn that no longer applies.

@dylanneve1 dylanneve1 closed this May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants