refactor(matrix): move ephemeral state to plugin sqlite#88387
Conversation
openclaw-barnacle
Bot
added
channel: discord
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 363ac064cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| metadata.deviceId = verification.deviceId.trim(); | ||
| } | ||
|
|
||
| return metadata; |
There was a problem hiding this comment.
For upgraded Matrix installs where the only recorded device id is still in the existing startup-verification.json, removing this fallback makes readStoredRootMetadata() return no deviceId. resolvePreferredMatrixStorageRoot() then rejects sibling roots unless they explicitly declare the same device, so after an access-token rotation it selects a fresh canonical root instead of the existing crypto/thread state; the new SQLite startup-verification migration cannot fix this because it also resolves its state path through resolveMatrixStoragePaths() first. Keep importing or honoring this legacy device id before applying the explicit-device match.
Useful? React with 👍 / 👎.
363ac06 to
d2df089
Compare
|
Codex review: needs real behavior proof before merge. Reviewed May 30, 2026, 1:12 PM ET / 17:12 UTC. Summary PR surface: Source +384, Tests +92. Total +476 across 10 files. Reproducibility: yes. for the patch defect from source inspection: current main reads legacy startup-verification deviceId during storage-root selection, while the PR removes it and only migrates startup state after resolving a root. I did not execute a failing PR-head test because this was a read-only review. Review metrics: 1 noteworthy metric.
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: Preserve or migrate legacy startup-verification deviceId before storage-root routing enforces explicit device matching, add a regression test for that upgrade state, and then provide redacted live Matrix startup/token-rotation proof before merge. Do we have a high-confidence way to reproduce the issue? Yes for the patch defect from source inspection: current main reads legacy startup-verification deviceId during storage-root selection, while the PR removes it and only migrates startup state after resolving a root. I did not execute a failing PR-head test because this was a read-only review. Is this the best way to solve the issue? No. Moving Matrix ephemeral state into plugin-state SQLite is a reasonable direction, but the safe implementation must preserve legacy device metadata before root selection rather than recording it only after a root has already been chosen. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against e708a872a1fd. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +384, Tests +92. Total +476 across 10 files. View PR surface stats
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
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
* refactor(matrix): persist ephemeral state in plugin sqlite * test(channels): wire matrix contract plugin state
* refactor(matrix): persist ephemeral state in plugin sqlite * test(channels): wire matrix contract plugin state
* refactor(matrix): persist ephemeral state in plugin sqlite * test(channels): wire matrix contract plugin state
1 participant
Summary
Verification
pnpm test extensions/matrix/src/matrix/client/storage.test.ts extensions/matrix/src/matrix/monitor/inbound-dedupe.test.ts extensions/matrix/src/matrix/monitor/startup-verification.test.ts extensions/matrix/src/matrix/thread-bindings.test.ts -- --reporter=verbosepnpm test:contracts:channels -- src/channels/plugins/contracts/session-binding.registry-backed.contract.test.ts --reporter=verbose/Users/steipete/Projects/agent-scripts/skills/autoreview/scripts/autoreview --mode local --prompt 'Final review after removing storage root legacy startup JSON deviceId fallback. Confirm startup verification no longer relies on legacy JSON for deviceId fallback, thread binding SQLite writes still claim current storage root, all legacy JSON imports are one-shot with markers, no fresh legacy JSON writes, inbound persistence remains best-effort, and account/state-dir isolation is preserved.'/Users/steipete/Projects/agent-scripts/skills/autoreview/scripts/autoreview --mode local --prompt 'Review the incremental fix after CI channel contract failure. Confirm Matrix contract test runtime now exposes plugin-state openKeyedStore through the SDK test helper, no production fallback/shim was added, and the Matrix SQLite state behavior remains intact.'pnpm check:changedvia Blacksmith Testboxtbx_01kswxxmpby4963btjwtm0aqf8, GitHub Actions run26689939748Real behavior proof
Behavior addressed: Matrix plugin-local ephemeral state now persists in plugin-state SQLite for inbound dedupe, startup verification, and thread bindings, with one-shot import from legacy JSON and no movement of crypto/recovery/IDB stores.
Real environment tested: local macOS checkout plus delegated Blacksmith Testbox Linux environment.
Exact steps or command run after this patch: focused Matrix Vitest command above, channel session-binding contract command above, structured autoreview commands above, and
pnpm check:changedin Testboxtbx_01kswxxmpby4963btjwtm0aqf8.Evidence after fix: focused Matrix tests passed 1 Vitest shard with 43 tests; session-binding contract passed 4 Vitest shards with 20 tests; both autoreview runs returned
autoreview clean: no accepted/actionable findings reported; Testbox changed gate exited 0.Observed result after fix: Matrix SQLite state migration and persistence tests pass; bundled channel contract runtime covers Matrix plugin-state access; changed typecheck, lint, import-cycle, dependency, and guard checks pass.
What was not tested: live Matrix homeserver traffic.