← Back to PRs

#22577: fix(feishu): use senderKey fallback for DM session isolation

by leesonchen open 2026-02-21 09:49 View on GitHub →
channel: feishu size: S
## For commit 0a7b5fdf7ba0cef068b2ac9cedd6260e0b226740(use senderKey fallback for DM session isolation) ## Summary Describe problem and fix in 2–5 bullets: - **Problem**: When Feishu doesn't provide `senderOpenId`, DM sessions collapse to a shared session for all users without `open_id`, causing privacy violations where different users could see each other's private conversations. - **Why it matters**: This breaks the fundamental privacy guarantee of `dmScope=per-channel-peer`, exposing private DM conversations between users. - **What changed**: Introduced `senderKey = senderOpenId || senderId` as stable user identifier, updated all DM-related operations (pairing, allowlist, routing, dynamic agent creation, message metadata) to use `senderKey`, added error logging when both IDs are missing. - **What did NOT change**: Core session routing logic, `dmScope` behavior, or Feishu API integration remain unchanged. ## Change Type (select all) - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Docs - [ ] Security hardening - [ ] Chore/infra ## Scope (select all touched areas) - [ ] Gateway / orchestration - [ ] Skills / tool execution - [ ] Auth / tokens - [x] Memory / storage - [x] Integrations - [ ] API / contracts - [ ] UI / DX - [ ] CI/CD / infra ## Linked Issue/PR - Closes # (reference to DM isolation issue) - Related # (reference to layered memory isolation design) ## User-visible / Behavior Changes List user-visible changes (including defaults/config). If none, write `None`. - None (fixes privacy bug, no user-visible behavior changes expected) ## Security Impact (required) - New permissions/capabilities? (`No`) - Secrets/tokens handling changed? (`No`) - New/changed network calls? (`No`) - Command/tool execution surface changed? (`No`) - Data access scope changed? (`No`) - If any `Yes`, explain risk + mitigation: ## Repro + Verification ### Environment - OS: Linux - Runtime/container: Node.js 22+ - Model/provider: N/A - Integration/channel: Feishu - Relevant config (redacted): `session.dmScope=per-channel-peer` ### Steps 1. Configure Feishu channel with `dmScope=per-channel-peer` 2. Send DM from user without `senderOpenId` (only `senderId`) 3. Send DM from different user without `senderOpenId` 4. Verify sessions are properly isolated (not collapsed) ### Expected Each user gets separate DM session. ### Actual Before fix: Users shared same session (privacy leak) After fix: Users get separate sessions (proper isolation) ## Evidence Attach at least one: - [x] Failing test/log before + passing after - [ ] Trace/log snippets - [ ] Screenshot/recording - [ ] Perf numbers (if relevant) ## Human Verification (required) What you personally verified (not just CI), and how: - **Verified scenarios**: DM session isolation with/without `senderOpenId`, pairing flow, allowlist checks, routing, dynamic agent creation - **Edge cases checked**: Both `senderOpenId` and `senderId` missing (error logging), fallback behavior when only `senderId` available - **What you did NOT verify**: Production Feishu environment with real users ## Compatibility / Migration - **Backward compatible?** (`Yes`) - **Config/env changes?** (`No`) - **Migration needed?** (`No`) - **If yes, exact upgrade steps**: N/A ## Failure Recovery (if this breaks) - **How to disable/revert this change quickly**: Revert commit, restore original `senderOpenId` usage - **Files/config to restore**: `extensions/feishu/src/bot.ts` - **Known bad symptoms reviewers should watch for**: DM session collapse, pairing failures, allowlist bypasses ## Risks and Mitigations List only real risks for this PR. Add/remove entries as needed. If none, write `None`. - **Risk**: Regression in Feishu DM handling if `senderId` is also missing - **Mitigation**: Added error logging and graceful fallback, extensive testing of edge cases - **Risk**: Breaking existing session keys if identifier format changes - **Mitigation**: Maintained backward compatibility, `senderKey` preserves original ID format --- ## For commit cf46ae4428c2b435246adcf162839ba067bd1154 (Plugin duplicate detection) ## Summary Describe problem and fix in 2–5 bullets: - **Problem**: False-positive duplicate warnings when bundled plugins are overridden by workspace/global/config installs in same directory, which is expected behavior. - **Why it matters**: Developers see confusing warnings during legitimate plugin development and override scenarios, reducing DX and potentially masking real duplicate issues. - **What changed**: Improved `samePlugin` detection with multiple checks (source equality, rootDir equality, path normalization, realpath comparison), added bundled plugin override suppression for same-directory cases, added comprehensive test coverage. - **What did NOT change**: Plugin loading precedence, manifest validation, or core plugin registry behavior. ## Change Type (select all) - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Docs - [ ] Security hardening - [x] Chore/infra ## Scope (select all touched areas) - [ ] Gateway / orchestration - [ ] Skills / tool execution - [ ] Auth / tokens - [ ] Memory / storage - [x] Integrations - [ ] API / contracts - [x] UI / DX - [ ] CI/CD / infra ## Linked Issue/PR - Closes # (reference to plugin warning issue) - Related # (reference to plugin system docs) ## User-visible / Behavior Changes List user-visible changes (including defaults/config). If none, write `None`. - Reduced false-positive warnings during plugin development - Cleaner plugin loading output for legitimate overrides ## Security Impact (required) - New permissions/capabilities? (`No`) - Secrets/tokens handling changed? (`No`) - New/changed network calls? (`No`) - Command/tool execution surface changed? (`No`) - Data access scope changed? (`No`) - If any `Yes`, explain risk + mitigation: ## Repro + Verification ### Environment - OS: Linux - Runtime/container: Node.js 22+ - Model/provider: N/A - Integration/channel: Plugin system - Relevant config (redacted): Plugin workspace/global installs ### Steps 1. Create workspace plugin that overrides bundled plugin 2. Install both in same directory 3. Load plugin registry 4. Verify no duplicate warnings emitted ### Expected No warnings for legitimate same-directory overrides. ### Actual Before fix: False-positive duplicate warnings After fix: Clean loading, no warnings ## Evidence Attach at least one: - [x] Failing test/log before + passing after - [ ] Trace/log snippets - [ ] Screenshot/recording - [ ] Perf numbers (if relevant) ## Human Verification (required) What you personally verified (not just CI), and how: - **Verified scenarios**: Workspace override of bundled plugin, global override of bundled plugin, different directory duplicates (still warn) - **Edge cases checked**: Same directory with different origins, path normalization edge cases, realpath resolution failures - **What you did NOT verify**: Production plugin registry with hundreds of plugins ## Compatibility / Migration - **Backward compatible?** (`Yes`) - **Config/env changes?** (`No`) - **Migration needed?** (`No`) - **If yes, exact upgrade steps**: N/A ## Failure Recovery (if this breaks) - **How to disable/revert this change quickly**: Revert commit, restore original duplicate detection - **Files/config to restore**: [src/plugins/manifest-registry.ts](cci:7://file:///home/leeson/bots/clawdbot/src/plugins/manifest-registry.ts:0:0-0:0), [src/plugins/manifest-registry.test.ts](cci:7://file:///home/leeson/bots/clawdbot/src/plugins/manifest-registry.test.ts:0:0-0:0) - **Known bad symptoms reviewers should watch for**: Missing legitimate duplicate warnings, plugin loading failures ## Risks and Mitigations List only real risks for this PR. Add/remove entries as needed. If none, write `None`. - **Risk**: Masking real plugin duplicates in different directories - **Mitigation**: Maintained warnings for different-directory duplicates, added comprehensive test coverage - **Risk**: Performance impact from additional path operations - **Mitigation**: Added caching for realpath operations, minimal overhead compared to plugin loading cost <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR fixes two unrelated issues: Feishu DM session isolation privacy bug and plugin duplicate detection false positives. **Feishu DM session isolation (extensions/feishu/src/bot.ts)** - Introduced `senderKey` with fallback from `senderOpenId` to `senderId` to prevent session collapse when Feishu doesn't provide the primary identifier - Updated most DM-related operations (pairing, routing, dynamic agent creation, message metadata) to use `senderKey` - Added error logging when both identifiers are missing - **Issue found**: Group sender allowlist check (line 598) still uses `ctx.senderOpenId` instead of `senderKey`, creating inconsistency with DM allowlist logic **Plugin duplicate detection (src/plugins/manifest-registry.ts)** - Enhanced `samePlugin` detection with multiple path comparison methods (source equality, rootDir equality, normalized paths, realpath) - Added bundled plugin override suppression for same-directory installs (workspace/global/config overriding bundled) - Inlined `safeRealpathSync` from separate module with caching - Added comprehensive test coverage for bundled override scenarios - Tests verify no warnings for same-directory overrides and correct precedence order <h3>Confidence Score: 4/5</h3> - This PR is mostly safe to merge with one logical inconsistency that should be addressed - Score reflects well-tested plugin changes with comprehensive coverage, but the Feishu change has an inconsistency where group sender allowlist still uses the old identifier instead of the new `senderKey` fallb...

Most Similar PRs