fix: five correctness bugs from deep code review - #252
Closed
dylanneve1 wants to merge 2 commits into
Closed
dylanneve1 wants to merge 2 commits into
dylanneve1 wants to merge 2 commits into
Conversation
- formatting.ts: Remove double HTML-escaping of URL in markdown links. Step 3 already escapes the entire text (including link hrefs) via the global entity-escape pass; calling escapeHtml(url) again in step 4 converts `&` β `&amp;`, breaking any URL with query-string parameters (e.g. `?a=1&b=2` rendered as `?a=1&amp;b=2`). - triggers.ts / triggers-extended.test.ts: Fire termination wake for ALL recently-terminated triggers on restart, not just ones that never sent a prior TALON_FIRE: event. A long-running watcher that emitted mid-run signals (setting lastFireAt) and was then killed by a Talon restart never received a terminal wake β the bot silently lost track of it. Removed the `t.lastFireAt === undefined` guard and updated the test to verify the correct behaviour. - sessions.ts: Validate the deserialized sessions JSON before assigning to `store`. If the file contained `null`, an array, or any non-object value, subsequent `store[chatId]` accesses would throw a TypeError and crash the process. Now logs a warning and resets to a fresh store instead of crashing. - history.ts: Same robustness fix β validate that the deserialized history JSON is a plain object before iterating with Object.entries(). Also added a per-entry `Array.isArray(messages)` guard so a single malformed chat entry cannot abort loading for all other chats. - cron.ts (withTimeout): Replace the `let timer: ReturnType<β¦>` + `timer!` non-null assertion anti-pattern with an explicit `| undefined` union and a clean `clearTimeout(timer)`. The old form suppressed a TypeScript strict-mode diagnostic and would confuse readers into thinking `timer` might genuinely be uninitialized. https://claude.ai/code/session_01LaNH9PYx6XiPxGwE1bg9pL
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
Deep audit of the codebase identified five distinct bugs spanning formatting, storage, scheduling, and trigger lifecycle. All are confirmed, all tests pass (2828 β, 34 skipped β same as
main).Bug 1 β Double HTML-escaping of URLs in
markdownToTelegramHtml(formatting.ts)Symptom: Any Markdown link whose URL contains
&(e.g. query-string parameters) is rendered broken in Telegram.[Click here](https://example.com?a=1&b=2)produceshref="β¦?a=1&amp;b=2"instead of the correcthref="β¦?a=1&b=2".Root cause: The function has a deliberate step 3 that HTML-escapes the entire working string (plain text segments between code-block / inline-code placeholders), including Markdown link syntax. By the time step 4 processes
[text](url), theurlcapture group already contains&from step 3. The callescapeHtml(url)then escapes that, yielding&amp;.Fix: Replace
escapeHtml(url)with justurlin the link-replacement regex handler. The URL is already safely escaped from step 3.Bug 2 β Missing termination wake for triggers that previously fired (
triggers.ts)Symptom: A long-running watcher that sent mid-run
TALON_FIRE:signals and was subsequently killed by a Talon restart never receives a terminal "terminated" wake notification. The bot silently loses awareness that the trigger was killed.Root cause:
resumeAfterRestart()had the conditiont.lastFireAt === undefined, which excluded any trigger that had ever calledfireWake(i.e. sent aTALON_FIRE:event). The intent was to avoid redundant wakes, but the effect is that mid-run multi-event triggers disappear silently on restart.Fix: Remove the
t.lastFireAt === undefinedguard. All recently-terminated triggers (within the 5-minute window) now receive a restart wake. The test description and assertion updated to match the correct behaviour.Bug 3 β Session store crashes on malformed JSON (
sessions.ts)Symptom: If
sessions.jsoncontains valid JSON that is not a plain object (e.g.null,[], a number), the process crashes with an uncaughtTypeError: Cannot read properties of nullat the firststore[chatId]access ingetSession().Root cause:
loadSessions()didstore = JSON.parse(...)with no type guard.JSON.parsesucceeds and assignsnull(or an array, etc.) tostore, and Node does not catch this until the next message is processed.Fix: Validate the parsed value is a plain non-null non-array object; if not, log a warning and reset to
{}.Bug 4 β History store crashes on malformed JSON (
history.ts)Symptom: Same class of crash as Bug 3. If
history.jsoncontains a non-object at the top level,Object.entries(raw)throwsTypeErrorand the entire process crashes on the next history write.Fix: Same pattern β validate the deserialized value before iterating. Additionally added a per-entry
Array.isArray(messages)guard so one malformed chat cannot abort loading for all other chats.Bug 5 β
withTimeoutuses non-null assertion on a possibly-uninitialized variable (cron.ts)Symptom:
let timer: ReturnType<typeof setTimeout>is declared without an initializer, then referenced asclearTimeout(timer!)in thefinallyblock. TypeScript strict mode emits a diagnostic here. While the runtime works (thenew Promisecallback assignstimersynchronously), the!assertion hides any future refactoring that might break this guarantee.Fix: Declare
timerasReturnType<typeof setTimeout> | undefinedand callclearTimeout(timer)unconditionally (which is a no-op whenundefined).Test plan
npm testβ 2828 tests pass, 34 skipped (no regressions)triggers-extended.test.tsβ updated to assert the new correct restart-wake behaviourtimer!assertion, proper union type)Generated by Claude Code