Skip to content

Feature/feishu topic workspace isolation#1551

Open
n-wen wants to merge 2 commits into
chenhg5:mainfrom
n-wen:feature/feishu-topic-workspace-isolation
Open

Feature/feishu topic workspace isolation#1551
n-wen wants to merge 2 commits into
chenhg5:mainfrom
n-wen:feature/feishu-topic-workspace-isolation

Conversation

@n-wen

@n-wen n-wen commented Jul 15, 2026

Copy link
Copy Markdown

Summary

修复飞书开启 thread_isolation 与 multi-workspace 同时使用时,同一群内不同 Topic 共用 workspace 的问题。Topic 现在使用独立的 workspace binding key;已有 chatId 级 binding 会保留为默认值,并复制给首次访问、尚未显式绑定的 Topic,兼容旧版本回退。

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing behavior to change)
  • Documentation only
  • Internal refactor / chore (no user-visible change)

Testing

  1. 修复后, 一个会话中每个topic可以独立bind/unbind互相不影响
  2. workspace_bindings.json在保留原来的feishu:oc_:chatId: {}配置下, 会独立产生feishu:oc_:topic:om_xxx的配置作为topic的独立配置
  3. 回滚后, 使用原配置, topic之间workspace互相不隔离

Automated tests added in this PR

  • TestLark_ThreadIsolationUsesRootSessionKey in platform/feishu/platform_test.go — 断言 Topic 消息生成独立 ChannelKey 与默认 chat key。
  • TestLark_GroupReplyAllWithThreadIsolationUsesRootSessionKeyWithoutMention in platform/feishu/platform_test.go — 覆盖 group reply-all 下的 Topic key。
  • TestInteractivePlatform_CardActionUsesCallbackSessionKey in platform/feishu/platform_test.go — 断言卡片回调保留 Topic workspace key。
  • TestWorkspaceBindingManager_InheritChannelKeyPreservesDefault in core/workspace_binding_test.go — 断言 Topic 继承默认 chat binding,且不删除默认 key。
  • TestWorkspaceBindingManager_InheritChannelKeyDoesNotOverwriteDestination in core/workspace_binding_test.go — 断言显式 Topic binding 不会被默认值覆盖。
  • TestWorkspaceBindingManager_InheritChannelKeyConcurrentTopics in core/workspace_binding_test.go — 断言多个 Topic 可并发继承默认 binding。
  • TestMigrateLegacyWorkspaceBindings_CopiesProjectAndSharedDefaults in core/multi_workspace_test.go — 覆盖 project 与 shared binding 默认值复制。
  • TestCUJ_H4_FeishuTopicsKeepWorkspaceBindingsIsolated in core/cuj_test.go — 覆盖 Topic 默认继承、独立 bind、查询与 unbind 回退默认值。

For bug fixes only — regression test

  • Regression test name: TestCUJ_H4_FeishuTopicsKeepWorkspaceBindingsIsolated
  • Manual verification this test catches the regression:
    • Reverted the fix locally; the regression test failed as expected.

Critical User Journeys (CUJ) impact

  • No CUJ touched (small refactor, doc change, etc.)
  • A — basic conversation
  • B — session lifecycle (/new /switch /list /history etc.)
  • C — agent execution control (/mode /cancel /stop permissions)
  • D — security & permissions (allow_from admin_from banned_words rate limits)
  • E — scheduled tasks (/cron /timer)
  • F — config switching (/lang /provider /model reload)
  • G — error handling & robustness (LLM failure, ws reconnect, agent crash)
  • H — multi-platform / multi-project isolation
  • I — UI rendering correctness (cards, streaming, display modes)

If any CUJ group is touched, confirm:

  • go test ./core/ -run TestCUJ passes locally.
  • If the change alters an existing user-visible flow, the corresponding
    CUJ test was updated (or a new CUJ added) to cover the new behavior.

Manual / user-visible behavior change

在同一个飞书群内:

  • 每个 Topic 可通过 /workspace bind <workspace> 绑定独立 workspace。
  • 未单独绑定的 Topic 继承原 chatId 级 workspace 默认值。
  • Topic 执行 /workspace unbind 后回退到 chatId 默认 workspace。
  • 原 chatId binding 保留,因此回退到旧版 cc-connect 后仍可正常匹配 workspace。

Checklist (reviewer will verify)

  • go build ./... passes
  • go test ./... passes (with -race if touching concurrency)
  • AGENTS.md Pre-Commit Checklist items are satisfied
  • No new hardcoded platform/agent names in core/
  • i18n strings have all-language translations (if any new user-facing text)
  • No secrets / credentials in source

Related

Fixes #1543

wenning and others added 2 commits July 15, 2026 15:12
Use topic-scoped channel keys in thread isolation mode and migrate legacy chat-level bindings so workspace routing no longer leaks across topics.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy chat-level bindings into new topics without deleting the original so unbound topics inherit a default and older versions retain rollback compatibility.

Co-authored-by: Cursor <cursoragent@cursor.com>

@chenhg5 chenhg5 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conclusion: Approve

Overall assessment: This PR fixes the #1543 pain point directly — same group chat, different topics now get independent workspace bindings. The reporter (n-wen) self-implemented with a copy-style migration that preserves the legacy chat-level binding as a default for new topics, which is the right tradeoff: existing users with a single chat-level binding get topic-scoped bindings automatically, but the original default stays so other channels and downgrades remain unaffected. CI is green, blast radius is contained (workspace_bindings + feishu adapter only), and test coverage directly exercises the user's scenario. Merge-ready.

Review scope:

  • Reviewed all 10 files (3 prod, 4 test, 2 doc, 1 example config) with +328/-17.
  • Cross-referenced #1543 (reporter pain point) and PM's direction-A guidance (workspace_bindings key with topic_id + partial migration fallback).
  • Focused on correctness of per-topic binding isolation, backward-compat migration, cross-adapter scope, and idempotency under concurrency.

✅ What looks good:

  • Direct #1543 coverage: populateWorkspaceChannelKeys parses sessionKey (feishu:oc_chat:root:om_root) and emits ChannelKey = "oc_chat:topic:om_root". The new TestCUJ_H4_FeishuTopicsKeepWorkspaceBindingsIsolated walks the exact reporter scenario — A binds workspace-a, B binds workspace-b, both keep their own, unbinding A restores chat-default and doesn't affect B. That's the user-observable outcome and it's tested end-to-end.
  • Migration is copy-not-move: MigrateChannelKey deliberately preserves the source binding (binding is copied via inherited := *binding) so other channels still see the chat-level default and a downgrade to an older version is non-destructive. The early-return guard if lookupLocked(newChannelKey) != nil makes repeated migration a no-op — idempotent by design.
  • Concurrent topic inheritance is covered: TestWorkspaceBindingManager_InheritChannelKeyConcurrentTopics fires MigrateChannelKey from goroutines for two topics inheriting the same default. The test expects both to inherit — i.e. the function is reentrant-safe under the m.mu.Lock() / saveLocked discipline. Good.
  • Cross-adapter scope is correct: populateWorkspaceChannelKeys lives on *feishu.Platform, and LegacyChannelKey is only set when p.threadIsolation is true. The Lark test (TestLark_SessionKeyPrefix) asserts LegacyChannelKey == "" outside topic mode, and Telegram/Discord/WeChat never populate either key — so the migration is a no-op for them and their bindings are unaffected.
  • CardAction dispatch refactor: replacing three copies of h := p.getHandler(); go h(p.dispatchPlatform(), &core.Message{...}) with go p.dispatchCoreMessage(&core.Message{...}) deduplicates the dispatch path. This also ensures populateWorkspaceChannelKeys is invoked uniformly for card actions, otherwise the topic scope would be lost on interactive-card-driven messages.
  • Docs cover the upgrade story: both config.example.toml and docs/feishu.md explicitly call out that the chat-level binding is preserved as the default, so users understand why downgrading still works.

🟠 Should improve (non-blocking):

  • Migration runs on every message: migrateLegacyWorkspaceBindings is called unconditionally in handleMessage whenever multiWorkspace is true. The early-return guards make it cheap, but it does take m.mu and call refreshLocked per message. Consider an in-memory "already-migrated" set (keyed by (projectKey, oldKey, newKey)) so the second message in a topic doesn't even attempt the lock. Not a blocker for correctness; matters only for high-throughput deployments.
  • Doc comment on MigrateChannelKey: the doc comment says "copies an existing default binding" but does not mention that the source binding is preserved. A reader scanning the comment could reasonably assume this is a move and accidentally rely on it. Recommend tightening the comment to: "Copies the source binding to the destination; never deletes the source. Idempotent: returns false if the destination already exists."
  • No integration test for "downgrade safety": there's a unit test that the destination survives a reload, but not one that simulates rolling back to an older cc-connect version (where LegacyChannelKey doesn't exist) and confirming topic bindings still resolve. A round-trip test where populateWorkspaceChannelKeys is skipped and the lookup still returns the topic binding would lock that contract in.
  • parseThreadRootID error path: when parts := strings.SplitN(rctx.sessionKey, ":", 3) doesn't yield 3 parts or the chat ID doesn't match, populateWorkspaceChannelKeys silently returns without setting LegacyChannelKey. The no-threadIsolation branch is the common case so this is fine, but a debug log when the parse fails (e.g. unexpected sessionKey shape from a future adapter) would help diagnose configuration drift.
  • Lock granularity under migration: migrateLegacyWorkspaceBindings walks []string{"project:" + e.name, sharedWorkspaceBindingsKey} and calls MigrateChannelKey once per project, each taking m.mu.Lock(). Sequential rather than batch. For multi-project configs that's two lock/unlock cycles per message. Fine in practice; flagging for completeness.

🔵 Optional:

  • Consider exposing a CLI subcommand like /workspace migrate-now for users who want to backfill topic bindings without sending a message (e.g. after manually copying a workspace dir). Skip if not requested.
  • The whitespace-only fixes in cuj_test.go (struct alignment in cujAgent and two Message literals) are gofmt-driven and harmless; just noting they appear in this PR to keep blame clean.

❓ Questions:

  • None blocking. Reporter self-implementation shows clear understanding of the pain point and the migration trade-off. Open to author input on whether the "already-migrated" set is worth adding now or after observing hot-path metrics.

Testing / Risk:

  • Verified evidence: 5/5 CI green (lint / unit-test / smoke-test / regression-test / performance-test, run 29399389477, 2026-07-15T07:34–07:43Z). New tests cover: per-topic binding isolation end-to-end (TestCUJ_H4), migration copy + project-and-shared coverage (TestMigrateLegacyWorkspaceBindings_CopiesProjectAndSharedDefaults), reload-after-migrate round-trip, no-overwrite-on-conflict, and concurrent inheritance. Existing tests for thread-isolation session keys (TestLark_ThreadIsolationUsesRootSessionKey etc.) updated to assert the new fields.
  • Remaining risk: production migration behavior under very long-running sessions with thousands of workspace_bindings.json entries — the manager reads the whole file on refreshLocked. If a single config grows large, the per-message overhead grows proportionally. Worth a perf follow-up if real users report it; the unit-test suite doesn't cover that scale.
  • Cross-adapter risk: confirmed none. Only *feishu.Platform populates the new fields; other adapters leave them empty and the migration is a no-op.

Next step:

  • Approve. Merge-ready. Owner can merge as-is or wait for v1.6.0/v1.5.1 release window per their discretion; this PR is well-isolated and the migration story is safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] multiworkspace mode支持飞书话题绑定隔离

2 participants