fix(sessions): treat session.json without sessionId as absent (#228) - #234
Conversation
|
Note on the failing The patch verification passed locally before push: So this PR is blocked on the repo's Claude Code Review workflow credentials / rerun, not on a code failure from this change. |
TerrysPOV
left a comment
There was a problem hiding this comment.
Code review
Found 1 issue:
peekThreadSessionis the one remaining read path that doesn't go throughhasValidSessionId. A corruptedsessions.jsonthread row — exactly the corruption class issue #228 reports — passes theif (!session)check in the Telegram/Discord/statusand/contexthandlers, then crashes onsession.sessionId.slice(0, 8)(or the${session.sessionId}.jsonlinterpolation), reproducing the sameCannot read properties of undefined (reading 'slice')this PR is meant to eliminate.
peekThreadSession returns the raw record without validation:
Vulnerable callers:
claudeclaw/src/commands/telegram.ts
Lines 1073 to 1085 in 404ba91
claudeclaw/src/commands/discord.ts
Lines 1183 to 1196 in 404ba91
Quickest fix: route peekThreadSession through the same hasValidSessionId guard getThreadSession now uses (lines 41-45 of sessionManager.ts), so it returns null for corrupted rows.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the review feedback in 5a6e633.
Added a regression test in node --experimental-strip-types --test tests/sessions-missing-id.test.ts
# 4 pass, 0 failReady for another look when you have a moment. |
|
@TerrysPOV friendly re-review ping — |
TerrysPOV
left a comment
There was a problem hiding this comment.
Code review
peekThreadSession blocker is resolved in 5a6e633 — thanks.
Found 1 remaining issue (same class):
listThreadSessionsis the last unfiltered thread read path. A corruptedsessions.jsonrow will be returned as-is, and the/statusthread-sessions loop in Discord crashes onts.sessionId.slice(0, 8)— sameTypeErrorsignature this PR is meant to eliminate.
listThreadSessions returns the raw map values without hasValidSessionId:
claudeclaw/src/sessionManager.ts
Lines 100 to 105 in 5a6e633
Crash site:
claudeclaw/src/commands/discord.ts
Lines 1169 to 1175 in 5a6e633
Quickest fix, mirroring the peekThreadSession shape in 5a6e633:
return Object.values(data.threads).filter(hasValidSessionId);(Sister mutators incrementThreadTurn / markThreadCompactWarned only bump fields and bail on !session, so they don't crash — they just mutate corrupted rows. Worth a follow-up but not blocking.)
listThreadSessions was the last thread read path returning rows without going through hasValidSessionId. A sessions.json row missing sessionId would reach the Discord /status thread-sessions loop and crash on ts.sessionId.slice(0, 8) — the same TypeError class moazbuilds#234 eliminates. Apply the existing guard. (review follow-up, moazbuilds#234)
|
Resolved in a9eb7cb. |
Required by plugin-version-guard / marketplace-version-guard — master advanced to 1.0.39 after moazbuilds#233 merged, matching this branch. (moazbuilds#234)
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
Maintainer commit — master reached 1.0.40 via moazbuilds#233, so the branch's 1.0.40 bump was a no-op. Re-bump to 1.0.41 to satisfy plugin-version-guard and marketplace-version-guard for PR moazbuilds#234.
TerrysPOV
left a comment
There was a problem hiding this comment.
The listThreadSessions blocker from the prior review is resolved — Object.values(data.threads).filter(hasValidSessionId) in a9eb7cb. I traced the full crash class and it's now complete: every path that dereferences .sessionId (Discord/Telegram/Slack /status + /context, runner, rotation, web UI) is sourced from a validated read, and the web-UI readers stay safe via their parallel UUID_RE.test() guard.
Maintainer-committed the version bump correction in c5e6343: the branch's 1.0.40 bump had become a no-op after #233 landed master at 1.0.40, so I re-bumped to 1.0.41 to clear plugin-version-guard / marketplace-version-guard. Please run bun run bump:plugin-version + bun run bump:marketplace-version yourself on future PRs — and rebase if another version bump lands before this merges.
Approving.
c5e6343 to
54854a9
Compare
TerrysPOV
left a comment
There was a problem hiding this comment.
Re-approving on the current head 54854a9 after reverting my erroneous 1.0.41 bump.
Context: the code fix is complete — the listThreadSessions guard (a9eb7cb) closes the last unvalidated thread-session read path, and I traced the full crash class to confirm no unguarded .sessionId dereference remains. The author's 1.0.40 bump already satisfies plugin-version-guard / marketplace-version-guard (they compare against this PR's base at 1.0.39, and both ran green at this SHA). My earlier extra bump to 1.0.41 was a misdiagnosis that introduced a merge conflict against master's 1.0.40; I've force-pushed it out, so the branch is back to a clean, mergeable state.
Approving.
Fixes #228. Corrupted session.json without sessionId is treated as absent; runner bootstraps instead of crashing on sessionId.slice. Tests: tests/sessions-missing-id.test.ts