fix(msteams): prevent toPluginJsonValue from crashing on unserializable values#111823
fix(msteams): prevent toPluginJsonValue from crashing on unserializable values#111823wanyongstar wants to merge 1 commit into
Conversation
openclaw-barnacle
Bot
added
channel: msteams
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed July 20, 2026, 8:30 AM ET / 12:30 UTC. Summary PR surface: Source +4. Total +4 across 1 file. Reproducibility: yes. —current-main source has an unguarded JSON round trip, and native Review metrics: none identified. Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the persistence conversion JSON-safe: use a defined representation for unsupported values or reject them at the owner boundary, add focused state-path coverage, and show a redacted real MSTeams state round trip after the fix. Do we have a high-confidence way to reproduce the issue? Yes—current-main source has an unguarded JSON round trip, and native Is this the best way to solve the issue? No—the proposed fallback suppresses the first exception while retaining the unsupported value; the fix should produce a defined JSON-safe result or fail at the owning state boundary with an actionable error. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: unclear because the file could not be read completely. Codex review notes: model internal, reasoning high; reviewed against 756de70e84d1. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +4. Total +4 across 1 file. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
…safe persistence toPluginJsonValue serializes MSTeams plugin state through JSON.stringify+parse without guarding against BigInt values or circular references, which causes a TypeError that tears down the caller before the state can be persisted. Add a JSON.stringify replacer that converts BigInt to String so nested BigInt values (e.g. message IDs) survive the round-trip as JSON-safe strings. Circular references and other structural failures fall back to an empty object instead of crashing. This keeps the persistence conversion contract intact: the return value is always JSON-safe and can complete the SQLite write-read round-trip without deferred failures.
440b24f to
5a5f393
Compare
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(msteams): prevent toPluginJsonValue from crashing on unserializable values This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
2 participants
What Problem This Solves
toPluginJsonValuein the MSTeams extension serializes plugin state values throughJSON.stringify+JSON.parsewithout guarding against BigInt values or circular references. When a nested BigInt (e.g. a message ID) or circular structure reaches the helper,JSON.stringifythrows an unhandledTypeErrorthat crashes the caller before the state can be persisted to SQLite.The function currently has no replacer, no catch guard, and no JSON-safe fallback -- it either produces valid JSON or throws, and when it throws the state write is silently lost.
Evidence
OLD (current main): crash at toPluginJsonValue, state write never happens
NEW (this PR): JSON-safe replacer, state round-trip succeeds
Normal poll state (baseline):
BigInt poll state (the fixed scenario):
Circular reference state (safe fallback):
Mixed BigInt + normal values (nested):
What Changed
Replaced the bare
JSON.stringify(value)/JSON.parse(serialized)round-trip with:JSON.stringify(value, (_key, val) => typeof val === "bigint" ? String(val) : val)converts BigInt values to strings during serialization so nested BigInts (message IDs, counts, etc.) become JSON-safe{}instead of crashingThe return value is now always JSON-safe: it can be written to SQLite and read back without a deferred serialization failure.
File:
extensions/msteams/src/sqlite-state.tsDiff: +8 / -2 (add replacer + try/catch + fallback)