fix: missing encryption fallback for certain groups

[highesttt]

No description provided.

[linear-code]

PLAT-37742

[indent-zero]

PR Summary

Fixes group message decryption/send failures that occurred when the access token had expired at the moment fetchAndUnwrapGroupKey needed to resolve the group creator's peer key. Previously ensurePeerKey/ensurePeerKeyByID bypassed the token-recovery wrapper, so a code-119 "Access token refresh required" response would fail the entire group key unwrap and prevent the LSOFF plaintext fallback for certain groups.

• Route ensurePeerKey and ensurePeerKeyByID through callLineResult, giving both peer-key paths the standard refresh-and-retry-once behavior on auth errors, and thread the previously-ignored context.Context argument into recovery.
• Add package-level function-var indirection (negotiateE2EEPublicKeyWithClient, getE2EEPublicKeyWithClient) mirroring the existing pattern in client.go so the two API calls can be mocked from tests.
• Extract recoverLineToken as a package-level function var in auth_recovery.go and route callLineUsing/callLineResultUsing through it, so tests can override recovery without seeding recoverTime (which would suppress the recovery closure via the recentTokenRecoveryWindow short-circuit).
• Add unit tests covering refresh-required retry (with a real assertion that recovery ran exactly once) for both peer-key paths, plus a t.Fatal-guarded negative-caching test proving IsNoUsableE2EEPublicKey never triggers recovery.

Issues

All clear! No issues remaining. 🎉

1 issue already resolved

• Nit — the "Recovers" tests never exercise recovery: newPeerKeyTestClient sets recoverTime: time.Now(), so runTokenRecovery short-circuits inside recentTokenRecoveryWindow and the recoverToken closure is never invoked. Only the retry-after-auth-error wiring is asserted, so the tests would still pass if the token-refresh half broke — consider zeroing recoverTime and mocking refresh, or renaming to reflect what is actually covered. (fixed by commit 17e817a)

CI Checks

Waiting for CI checks...

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes

  • Improved reliability when retrieving peer encryption keys, including smoother recovery after authentication refreshes.
  • Reduced unnecessary re-tries by caching “no usable key” outcomes and ensuring consistent behavior across repeated lookups.
  • Ensured peer key negotiation and retrieval return the expected key details after client renewal.

• Tests

  • Added automated test coverage for E2EE peer public-key retrieval, including retry-on-auth-recovery and negative caching scenarios.

Walkthrough

E2EE public-key negotiation and fetch paths now go through callLineResult wrappers, with token recovery routed through a helper closure. ensurePeerKey and ensurePeerKeyByID use their context parameters, and new tests cover auth recovery retries and negative caching.

Changes

E2EE key retrieval refactor

Layer / File(s)	Summary
Recovery wrapper wiring pkg/connector/auth_recovery.go	Adds recoverLineToken and updates recovery callbacks passed into callLineUsing and callLineResultUsing.
Wrapper functions and ensurePeerKey update pkg/connector/e2ee_keys.go	Adds wrapper variables for E2EE public-key negotiation and fetch calls, and updates ensurePeerKey to use ctx and callLineResult for negotiation.
ensurePeerKeyByID update pkg/connector/e2ee_keys.go	Updates ensurePeerKeyByID to use ctx and route the keyVersion == 1 public-key fetch through callLineResult.
Tests for retry and negative caching pkg/connector/e2ee_keys_test.go	Adds a shared test helper and three tests covering retry-on-auth-error for both peer-key paths and negative caching for unusable E2EE keys.

Estimated code review effort: 2 (Simple) | ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	No pull request description was provided, so the change intent cannot be evaluated from the description.	Add a brief PR description summarizing the E2EE recovery and key-fetch fallback behavior changes.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title matches the main change: fixing E2EE/encryption fallback and recovery for affected groups.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch highest/plat-37742

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 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/e2ee_keys_test.go`:
- Around line 17-25: The test helper newPeerKeyTestClient currently seeds
recoverTime with time.Now(), which causes recoverToken() to stay inside
recentTokenRecoveryWindow and skip the retry path. Update LineClient
construction in newPeerKeyTestClient so recoverTime is left unset or initialized
outside the cooldown window, allowing the auth-error retry tests to exercise
refresh/re-login behavior through recoverToken().

🪄 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: f1f60cb0-7cd5-499a-9185-59104b56731d

📥 Commits

Reviewing files that changed from the base of the PR and between bbb1a5d and d2feb91.

📒 Files selected for processing (2)

• pkg/connector/e2ee_keys.go
• pkg/connector/e2ee_keys_test.go

📜 Review details

⏰ Context from checks skipped due to timeout. (4)

• GitHub Check: Lint with 1.25
• GitHub Check: build-docker
• GitHub Check: build-docker
• GitHub Check: Lint with 1.25

🧰 Additional context used

📓 Path-based instructions (2)

**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Use go fmt for code formatting across all Go files
Use goimports with -local "github.com/highesttt/matrix-line-messenger" flag to group project-local imports correctly
Use zerolog for logging throughout the codebase
Do not use Msgf in logging; use Msg with structured fields instead
Use Stringer interface where applicable in Go code

Files:

• pkg/connector/e2ee_keys_test.go
• pkg/connector/e2ee_keys.go

**/!(ltsm)/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/!(ltsm)/**/*.go: Run staticcheck on all Go files excluding pkg/ltsm package (transpiled WASM code)
Run go vet on all Go files excluding pkg/ltsm package (transpiled WASM code)

Files:

• pkg/connector/e2ee_keys_test.go
• pkg/connector/e2ee_keys.go

🔇 Additional comments (2)

pkg/connector/e2ee_keys.go (1)

16-24: LGTM!

Also applies to: 98-98, 119-121, 318-318, 332-335

pkg/connector/e2ee_keys_test.go (1)

1-16: LGTM!

Also applies to: 27-136

[coderabbitai]

🧹 Nitpick comments (1)

pkg/connector/auth_recovery.go (1)

17-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Global mutable var used as a test seam is race-prone.

recoverLineToken is a package-level var function used purely to allow overriding recovery behavior (likely for tests in the upcoming e2ee_keys_test.go layer). Reassigning a shared package-level var across tests is unsynchronized global mutable state — if tests run with t.Parallel(), or overrides aren't restored via t.Cleanup, this risks data races and test pollution. Also, by Go convention context.Context should be the first parameter.

Consider injecting the recovery function via a struct field on LineClient (or via lineCallDeps.recover directly, already present) instead of a global var, so each test/call site can supply its own recovery function without touching shared state.

♻️ Suggested refactor

-var recoverLineToken = func(lc *LineClient, ctx context.Context) error {
-	return lc.recoverToken(ctx)
-}
+func recoverLineToken(ctx context.Context, lc *LineClient) error {
+	return lc.recoverToken(ctx)
+}

And for tests, prefer injecting a stub directly into lineCallDeps.recover (already a func field) rather than swapping the package-level var.

🤖 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/auth_recovery.go` around lines 17 - 20, The package-level
recoverLineToken var is shared mutable state and should not be used as a test
seam. Move token recovery injection onto LineClient or use the existing
lineCallDeps.recover func field so each instance/call can supply its own
implementation, and update the recovery helper signature to follow Go convention
with context.Context first. Adjust any callers in LineClient/recovery flow to
use the injected dependency instead of reassigning the global var.

🤖 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.

Nitpick comments:
In `@pkg/connector/auth_recovery.go`:
- Around line 17-20: The package-level recoverLineToken var is shared mutable
state and should not be used as a test seam. Move token recovery injection onto
LineClient or use the existing lineCallDeps.recover func field so each
instance/call can supply its own implementation, and update the recovery helper
signature to follow Go convention with context.Context first. Adjust any callers
in LineClient/recovery flow to use the injected dependency instead of
reassigning the global var.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: aede692c-c586-40e1-8069-7214edb6f649

📥 Commits

Reviewing files that changed from the base of the PR and between d2feb91 and 17e817a.

📒 Files selected for processing (2)

• pkg/connector/auth_recovery.go
• pkg/connector/e2ee_keys_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/connector/e2ee_keys_test.go

📜 Review details

⏰ Context from checks skipped due to timeout. (4)

• GitHub Check: Lint with 1.25
• GitHub Check: build-docker
• GitHub Check: build-docker
• GitHub Check: Lint with 1.25

🧰 Additional context used

📓 Path-based instructions (2)

**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Use go fmt for code formatting across all Go files
Use goimports with -local "github.com/highesttt/matrix-line-messenger" flag to group project-local imports correctly
Use zerolog for logging throughout the codebase
Do not use Msgf in logging; use Msg with structured fields instead
Use Stringer interface where applicable in Go code

Files:

• pkg/connector/auth_recovery.go

**/!(ltsm)/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/!(ltsm)/**/*.go: Run staticcheck on all Go files excluding pkg/ltsm package (transpiled WASM code)
Run go vet on all Go files excluding pkg/ltsm package (transpiled WASM code)

Files:

• pkg/connector/auth_recovery.go

🔇 Additional comments (1)

pkg/connector/auth_recovery.go (1)

60-60: LGTM! Passthrough wiring preserves existing recovery semantics (runTokenRecovery locking/negative-window behavior unchanged per client.go).

Also applies to: 79-79