From 9428bdfb54906f41e2e81189aa8b9250d2434c83 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 15 May 2026 02:21:31 +0000 Subject: [PATCH] fix: six bugs found in deep code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/core/dream.ts | 81 +++++++++++++++++++++++++----- src/core/gateway.ts | 4 +- src/frontend/telegram/callbacks.ts | 8 ++- src/frontend/telegram/handlers.ts | 1 + src/storage/cron-store.ts | 5 +- src/storage/history.ts | 2 +- 6 files changed, 83 insertions(+), 18 deletions(-) diff --git a/src/core/dream.ts b/src/core/dream.ts index 5e4c1afbe..8ed26d7bb 100644 --- a/src/core/dream.ts +++ b/src/core/dream.ts @@ -30,6 +30,8 @@ export type DreamState = { last_run: number; /** Human-readable ISO timestamp of the last completed dream run. */ last_run_at?: string; + /** Unix millisecond timestamp of the last time a dream was started (success or failure). */ + last_started?: number; /** "idle" when no dream is running, "running" while one is active. */ status: "idle" | "running"; }; @@ -39,6 +41,7 @@ export type DreamState = { const DREAM_INTERVAL_MS = 12 * 60 * 60 * 1000; // 12 hours const DREAM_STATE_FILE = pathFiles.dreamState; const DREAM_TIMEOUT_MS = 10 * 60 * 1000; // 10-minute max +const DREAM_ABORT_GRACE_MS = 30 * 1000; // wait up to 30s for SDK to honour abort const DREAM_LOGS_DIR = resolve(dirs.logs, "dreams"); // ── State ──────────────────────────────────────────────────────────────────── @@ -96,24 +99,29 @@ export async function forceDream(): Promise { async function executeDream(trigger: "auto" | "forced"): Promise { const state = readDreamState(); const now = Date.now(); + const previousLastRun = state?.last_run ?? 0; dreaming = true; - writeDreamState({ last_run: now, status: "running" }); + // Preserve last_run from the previous completed run so a crash between here + // and completion doesn't reset the 12-hour interval to the start of the + // failed run (which would suppress the next dream for up to 12h from the + // crash). Use a separate last_started to track when this attempt began. + writeDreamState({ last_run: previousLastRun, last_started: now, status: "running" }); log( "dream", - `${trigger === "forced" ? "Force-triggering" : "Triggering"} memory consolidation (last run: ${state?.last_run ? new Date(state.last_run).toISOString() : "never"})`, + `${trigger === "forced" ? "Force-triggering" : "Triggering"} memory consolidation (last run: ${previousLastRun ? new Date(previousLastRun).toISOString() : "never"})`, ); try { - const dreamLogPath = await runDreamAgent(state?.last_run ?? 0); - writeDreamState({ last_run: Date.now(), status: "idle" }); + const dreamLogPath = await runDreamAgent(previousLastRun); + writeDreamState({ last_run: Date.now(), last_started: now, status: "idle" }); log( "dream", `Memory consolidation complete (${trigger}), log: ${dreamLogPath}`, ); } catch (err) { logError("dream", `Memory consolidation failed (${trigger})`, err); - writeDreamState({ last_run: Date.now(), status: "idle" }); + writeDreamState({ last_run: previousLastRun, last_started: now, status: "idle" }); if (trigger === "forced") throw err; } finally { dreaming = false; @@ -195,6 +203,8 @@ If commands fail, log the error and continue — this stage is optional.` `**Prompt:**\n\`\`\`\n${prompt}\n\`\`\`\n\n---\n`, ); + const abortController = new AbortController(); + const options = { model, systemPrompt: configRef.mempalace @@ -203,6 +213,7 @@ If commands fail, log the error and continue — this stage is optional.` cwd: workspace, permissionMode: "bypassPermissions" as const, allowDangerouslySkipPermissions: true, + abortController, ...(configRef.claudeBinary ? { pathToClaudeCodeExecutable: configRef.claudeBinary } : {}), @@ -213,12 +224,18 @@ If commands fail, log the error and continue — this stage is optional.` disallowedTools: [...DISALLOWED_TOOLS_BACKGROUND], }; + let timeoutFired = false; let timeoutHandle: ReturnType | null = null; const timeoutPromise = new Promise((_, reject) => { - const t = setTimeout( - () => reject(new Error("Dream agent timed out")), - DREAM_TIMEOUT_MS, - ); + const t = setTimeout(() => { + timeoutFired = true; + try { + abortController.abort(); + } catch { + /* ignore */ + } + reject(new Error("Dream agent timed out")); + }, DREAM_TIMEOUT_MS); t.unref(); // Don't prevent Node.js from exiting cleanly during shutdown timeoutHandle = t; }); @@ -240,13 +257,33 @@ If commands fail, log the error and continue — this stage is optional.` try { await Promise.race([agentPromise, timeoutPromise]); } catch (err) { + const wasTimeout = timeoutFired; + if (timeoutHandle) { + clearTimeout(timeoutHandle); + timeoutHandle = null; + } appendDreamLog( dreamLogFile, `\n---\n**Dream FAILED at ${new Date().toISOString()}:** ${err}\n`, ); - // On timeout, wait for the agent to actually finish before releasing the - // dreaming lock to prevent overlapping dream runs - await agentPromise.catch(() => {}); + if (wasTimeout) { + // Give the SDK a bounded grace window to honour the abort signal. + // If it still hasn't settled we release the dreaming lock anyway — + // better to allow the next dream than hang forever. + const settled = await raceWithTimeout( + agentPromise.catch(() => "settled" as const), + DREAM_ABORT_GRACE_MS, + ); + if (settled === "timed_out") { + logWarn( + "dream", + `Dream SDK ignored abort after ${DREAM_ABORT_GRACE_MS}ms — releasing lock`, + ); + } + } else { + // Non-timeout failure — agentPromise has already settled. + await agentPromise.catch(() => {}); + } throw err; } finally { if (timeoutHandle) clearTimeout(timeoutHandle); @@ -379,3 +416,23 @@ function writeDreamState(state: DreamState): void { logError("dream", "Failed to write dream state", err); } } + +// ── Async helpers ──────────────────────────────────────────────────────────── + +async function raceWithTimeout( + p: Promise, + ms: number, +): Promise { + let t: ReturnType | null = null; + try { + return await Promise.race([ + p, + new Promise<"timed_out">((resolve) => { + t = setTimeout(() => resolve("timed_out"), ms); + t.unref(); + }), + ]); + } finally { + if (t) clearTimeout(t); + } +} diff --git a/src/core/gateway.ts b/src/core/gateway.ts index 7bd78227d..dabf40d79 100644 --- a/src/core/gateway.ts +++ b/src/core/gateway.ts @@ -47,7 +47,7 @@ export async function withRetry(fn: () => Promise): Promise { const classified = classify(err); if (!classified.retryable) { // Wrap in AbortError to prevent further retries - throw new AbortError(classified); + throw new AbortError(classified.message); } const delayMs = classified.retryAfterMs ?? 1000 * Math.pow(2, attempt - 1); @@ -189,7 +189,7 @@ export class Gateway { // String-id routing (Teams) — must match an active context. chatId = this.findContextByStringId(rawChatId); } - if (!chatId) { + if (chatId == null) { return { ok: false, error: "No active chat context" }; } diff --git a/src/frontend/telegram/callbacks.ts b/src/frontend/telegram/callbacks.ts index 22180006d..d7f79729c 100644 --- a/src/frontend/telegram/callbacks.ts +++ b/src/frontend/telegram/callbacks.ts @@ -18,7 +18,7 @@ import { enablePulse, isPulseEnabled, } from "../../core/pulse.js"; -import { handleCallbackQuery } from "./handlers.js"; +import { handleCallbackQuery, isAccessAllowed } from "./handlers.js"; import { escapeHtml } from "./formatting.js"; import { renderSettingsText, @@ -279,7 +279,11 @@ export function registerCallbacks( return; } - // Forward other callbacks to the AI backend + // Forward other callbacks to the AI backend — access-controlled + if (!(await isAccessAllowed(ctx, bot))) { + await ctx.answerCallbackQuery().catch(() => {}); + return; + } handleCallbackQuery(ctx, bot, config); }); } diff --git a/src/frontend/telegram/handlers.ts b/src/frontend/telegram/handlers.ts index 6361b6603..f2743f05b 100644 --- a/src/frontend/telegram/handlers.ts +++ b/src/frontend/telegram/handlers.ts @@ -1162,6 +1162,7 @@ export async function handleStickerMessage( ): Promise { if (!ctx.message || !ctx.chat || !shouldHandleInGroup(ctx)) return; if (!(await isAccessAllowed(ctx, bot))) return; + if (ctx.from?.id && isUserRateLimited(ctx.from.id)) return; const chatId = String(ctx.chat.id); const isGroup = ctx.chat.type === "group" || ctx.chat.type === "supergroup"; diff --git a/src/storage/cron-store.ts b/src/storage/cron-store.ts index d0e7e962d..be06522a8 100644 --- a/src/storage/cron-store.ts +++ b/src/storage/cron-store.ts @@ -146,9 +146,12 @@ export function validateCronExpression( try { const cron = new Cron(expr, { timezone: timezone ?? undefined }); const nextDate = cron.nextRun(); + if (!nextDate) { + return { valid: false, error: "Expression has no future occurrences" }; + } return { valid: true, - next: (nextDate as Date).toISOString(), + next: nextDate.toISOString(), }; } catch (err) { return { diff --git a/src/storage/history.ts b/src/storage/history.ts index af86f984f..5114a8981 100644 --- a/src/storage/history.ts +++ b/src/storage/history.ts @@ -77,7 +77,7 @@ export function loadHistory(): void { /* backup also corrupt */ } logError( - "sessions", + "history", "History data corrupt and no valid backup — starting fresh", ); }