fix: chat keys not updating when refreshing#203
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/!(ltsm)/**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors LINE login E2EE key export and refresh handling, adds sentinel E2EE manager errors, and changes group message send flow to return a reconnect status when the own private key is missing. ChangesLINE E2EE key management and reconnect handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TryLogin
participant LineClient
participant E2EEManager
participant SecureStorage
TryLogin->>LineClient: refreshLoginE2EEKeys(loginResult)
LineClient->>LineClient: saveLoginE2EEKeyMetadata(meta)
LineClient->>E2EEManager: export encrypted keys
LineClient->>SecureStorage: SaveSecureDataToFile(exportedKeyMap)
LineClient->>E2EEManager: LoadExportedKeys
E2EEManager-->>TryLogin: refresh result / error
sequenceDiagram
participant HandleMatrixMessage
participant E2EEManager
participant User
HandleMatrixMessage->>E2EEManager: fetchAndUnwrapGroupKey
E2EEManager-->>HandleMatrixMessage: ErrMissingOwnPrivateKey
HandleMatrixMessage->>HandleMatrixMessage: lineGroupE2EEReconnectRequiredError
HandleMatrixMessage-->>User: reconnect status error with notice
Related Issues: Not specified in the provided information. Related PRs: Not specified in the provided information. Suggested labels: e2ee, bug-fix Suggested reviewers: Not specified in the provided information. 🐰 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| WithIsCertain(true). | ||
| WithMessage(lineGroupE2EEReconnectNotice). | ||
| WithSendNotice(true). | ||
| WithErrorReason(event.MessageStatusGenericError) |
There was a problem hiding this comment.
Nit — reconnect notice can loop. lineGroupE2EEReconnectRequiredError triggers on any ErrMissingOwnPrivateKey, but the sentinel bubbles up from Manager.UnwrapGroupSharedKey whenever sharedKey.ReceiverKeyID isn't in keyByRawID/myRawKeyID. If LINE hands us a group key wrapped for a receiver key that the freshly-refreshed keychain also doesn't contain (e.g. an old shared key still targeting a rotated device key), the user follows the instructions, refreshLoginE2EEKeys succeeds, and the next send fails with the same notice — an infinite reconnect prompt from the user's perspective. Consider only surfacing this status when the loaded MyKeyIDs() differs from ReceiverKeyID, or falling back to the existing markGroupNoE2EE + plaintext path once a reconnect has already been suggested.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/connector/send_message_test.go (1)
15-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover the
HandleMatrixMessagereconnect branches too.These tests only prove the helper works in isolation. The user-visible behavior changed in
pkg/connector/send_message.goLines 573-592, whereHandleMatrixMessageshould return the reconnect status instead of falling through to the plaintext fallback. Adding one focused test for each branch would catch regressions if those call sites stop invoking the helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connector/send_message_test.go` around lines 15 - 44, The current tests only cover lineGroupE2EEReconnectRequiredError in isolation, so add focused coverage for the reconnect paths in HandleMatrixMessage where it should return the reconnect MessageStatus instead of falling back to plaintext handling. Update pkg/connector/send_message_test.go with one test per branch that exercises the relevant HandleMatrixMessage path and verifies the returned status/error behavior. Use the existing helper lineGroupE2EEReconnectRequiredError, lineGroupE2EEReconnectNotice, and the matrix send flow symbols in send_message.go to locate the exact branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/connector/client.go`:
- Around line 565-570: The E2EE metadata is being updated in tryLogin before
exportLoginE2EEKeys can fail, which can leave meta partially refreshed when the
export errors. In the login flow around saveLoginE2EEKeyMetadata,
exportLoginE2EEKeys, and meta.ExportedKeyMap, delay mutating meta until after
exportLoginE2EEKeys succeeds, or build the refreshed metadata from the exported
result and only then persist it. Make sure tryLogin only saves the refreshed
metadata once both the export and the ExportedKeyMap update have completed
successfully.
- Around line 574-578: The exportLoginE2EEKeys flow is dropping the freshly
initialized E2EE manager when lc.E2EE is nil, so the client keeps no in-memory
keys after login refresh. Update the lc.E2EE handling in exportLoginE2EEKeys to
install the returned manager into lc.E2EE when no active manager exists, and
only call LoadMyKeyFromExportedMap on an existing manager. Use the lc.E2EE and
exportLoginE2EEKeys symbols to locate the branch.
---
Nitpick comments:
In `@pkg/connector/send_message_test.go`:
- Around line 15-44: The current tests only cover
lineGroupE2EEReconnectRequiredError in isolation, so add focused coverage for
the reconnect paths in HandleMatrixMessage where it should return the reconnect
MessageStatus instead of falling back to plaintext handling. Update
pkg/connector/send_message_test.go with one test per branch that exercises the
relevant HandleMatrixMessage path and verifies the returned status/error
behavior. Use the existing helper lineGroupE2EEReconnectRequiredError,
lineGroupE2EEReconnectNotice, and the matrix send flow symbols in
send_message.go to locate the exact branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89ea6579-f7e4-4713-afeb-9dff34e271ba
📒 Files selected for processing (7)
pkg/connector/client.gopkg/connector/connector.gopkg/connector/login_keys_test.gopkg/connector/send_message.gopkg/connector/send_message_test.gopkg/e2ee/manager.gopkg/e2ee/manager_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Lint with 1.25
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
- GitHub Check: build-docker
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Usego fmtfor code formatting across all Go files
Usegoimportswith-local "github.com/highesttt/matrix-line-messenger"flag to group project-local imports correctly
Usezerologfor logging throughout the codebase
Do not useMsgfin logging; useMsgwith structured fields instead
UseStringerinterface where applicable in Go code
Files:
pkg/connector/login_keys_test.gopkg/connector/send_message_test.gopkg/e2ee/manager.gopkg/connector/connector.gopkg/connector/client.gopkg/e2ee/manager_test.gopkg/connector/send_message.go
**/!(ltsm)/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/!(ltsm)/**/*.go: Runstaticcheckon all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Rungo veton all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Files:
pkg/connector/login_keys_test.gopkg/connector/send_message_test.gopkg/e2ee/manager.gopkg/connector/connector.gopkg/connector/client.gopkg/e2ee/manager_test.gopkg/connector/send_message.go
pkg/connector/connector.go
📄 CodeRabbit inference engine (AGENTS.md)
Implement
bridgev2.NetworkConnectorandbridgev2.NetworkAPIinterfaces in the connector package for bridge logic
Files:
pkg/connector/connector.go
🔇 Additional comments (6)
pkg/e2ee/manager.go (1)
7-7: LGTM!Also applies to: 20-24, 466-466, 633-633
pkg/e2ee/manager_test.go (1)
1-36: LGTM!pkg/connector/send_message.go (1)
8-8: LGTM!Also applies to: 22-22, 35-48, 575-577, 591-592
pkg/connector/connector.go (1)
473-522: LGTM!pkg/connector/login_keys_test.go (1)
1-38: LGTM!pkg/connector/client.go (1)
24-24: LGTM!Also applies to: 549-551
| if lc.E2EE != nil { | ||
| if err := lc.E2EE.LoadMyKeyFromExportedMap(exported); err != nil { | ||
| return fmt.Errorf("load exported keys into active E2EE manager: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Install the freshly initialized manager when lc.E2EE is nil.
exportLoginE2EEKeys returns an initialized manager, but this branch drops it when no active manager exists. That leaves the re-logged-in client without in-memory E2EE keys even after a successful refresh.
Proposed fix
- if lc.E2EE != nil {
- if err := lc.E2EE.LoadMyKeyFromExportedMap(exported); err != nil {
- return fmt.Errorf("load exported keys into active E2EE manager: %w", err)
- }
+ if lc.E2EE == nil {
+ lc.E2EE = mgr
+ } else if err := lc.E2EE.LoadMyKeyFromExportedMap(exported); err != nil {
+ return fmt.Errorf("load exported keys into active E2EE manager: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if lc.E2EE != nil { | |
| if err := lc.E2EE.LoadMyKeyFromExportedMap(exported); err != nil { | |
| return fmt.Errorf("load exported keys into active E2EE manager: %w", err) | |
| } | |
| } | |
| if lc.E2EE == nil { | |
| lc.E2EE = mgr | |
| } else if err := lc.E2EE.LoadMyKeyFromExportedMap(exported); err != nil { | |
| return fmt.Errorf("load exported keys into active E2EE manager: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/connector/client.go` around lines 574 - 578, The exportLoginE2EEKeys flow
is dropping the freshly initialized E2EE manager when lc.E2EE is nil, so the
client keeps no in-memory keys after login refresh. Update the lc.E2EE handling
in exportLoginE2EEKeys to install the returned manager into lc.E2EE when no
active manager exists, and only call LoadMyKeyFromExportedMap on an existing
manager. Use the lc.E2EE and exportLoginE2EEKeys symbols to locate the branch.
1 participant
No description provided.