Skip to content

feat(slack): implement Phase 1 bot behavior spec#242

Merged
hrygo merged 16 commits intohrygo:mainfrom
aaronwong1989:feat/241-thread-ownership-policy
Mar 8, 2026
Merged

feat(slack): implement Phase 1 bot behavior spec#242
hrygo merged 16 commits intohrygo:mainfrom
aaronwong1989:feat/241-thread-ownership-policy

Conversation

@aaronwong1989
Copy link
Copy Markdown
Collaborator

@aaronwong1989 aaronwong1989 commented Mar 8, 2026

Implement Phase 1 of bot-behavior-spec.md for multi-bot Slack channel support. Add Owner Policy configuration (owner_only/trusted/public). Add bot-centric thread ownership tracking with TTL expiration. Implement R3/R4/R5 ownership rules. Resolves #241

HotPlexBot01 and others added 13 commits March 8, 2026 03:52
- Add owner definition and policy configuration
- Unify decision flow diagram
- Add DM behavior and edge cases
- Clean up duplicate content
- Consolidate configuration structure
- Add Owner policy config (owner_only, trusted, public)
- Add ThreadOwnershipTracker for thread ownership management
- Implement unified decision flow for message response
- Support multi-owner threads via @mention
- Add TTL expiration for thread ownership
- Integrate ownership logic in events.go and socketmode.go

Phase 1 MVP features implemented:
- @mention detection and response
- Owner policy configuration
- Thread ownership tracking
- Ownership transfer on @mention
- Multi-owner support
- TTL expiration

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Implement R5: ownership release when other bots mentioned
- Add background cleanup goroutine for expired ownerships
- Add Stop() method for graceful shutdown
- Fix ownership tracking with context-based lifecycle

Issues addressed:
- P0: R5 rule implementation (ownership release)
- P1: Memory leak prevention (periodic cleanup)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- R3: @mention transfers ownership (was of bot mentions)
- R4: Multi-bot mention creates shared ownership
- R5: @mentions excluding current owner releases ownership
- Added human用户过滤,防止非 bot 用户成为所有者
- Added concurrency安全改进 (atomic.Bool for Stop)
- 緻加单元测试覆盖并发场景

- 修复代码构建错误

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix R3: use SetOwners for Claim ownership
- Fix R4: only set known bot IDs as owners
- Add Stop() idempotent protection with atomic.Bool
- Fix TTL test: use HasOwner
- Add CleanupExpired test
- Add UpdateLastActive test

All tests pass.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rygo#241)

Key changes:
- ThreadOwnershipTracker now tracks threads owned by THIS bot
- Removed multi-owner tracking (OwnerIDs map)
- Simplified API: Claim(), Release(), Owns()
- Each bot instance independently tracks its owned threads
- R3/R4: Multi-bot scenarios handled by independent bot instances
- R5: Ownership release when others are mentioned

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All Phase 1 MVP features implemented:
- @mention detection and response
- Thread reply mode
- Owner policy (owner_only/trusted/public)
- Bot-centric thread ownership tracking
- Ownership transfer (R3)
- Multi-owner support (independent tracking)
- TTL expiration

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@aaronwong1989 aaronwong1989 changed the title docs(design): Thread Ownership Policy Design feat(slack): implement Phase 1 bot behavior spec Mar 8, 2026
Resolve conflicts in socketmode.go:
- Keep thread ownership decision logic
- Remove duplicate bot mention check (upstream has it at line 218-221)
- Preserve upstream improvements (event_type, event_ts metadata)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hrygo
Copy link
Copy Markdown
Owner

hrygo commented Mar 8, 2026

🔍 PR #242 综合审查报告

审查完成,发现 4 个关键问题6 个建议改进


🔴 必须修复 (Critical)

1. Nil Pointer Panic - socketmode.go:140-148

严重程度: Critical | 置信度: 95%

if a.config.IsThreadOwnershipEnabled() {
    threadKey := NewThreadKey(ev.Channel, threadID)
    a.ownershipTracker.Claim(threadKey)  // ← PANIC if ownershipTracker is nil
``"

`IsThreadOwnershipEnabled()` 只检查配置是否启用不保证 `ownershipTracker` 已成功初始化**修复**:
```go
if a.config.IsThreadOwnershipEnabled() && a.ownershipTracker != nil {

2. Goroutine 泄漏 - adapter.go:252-272

严重程度: Critical | 置信度: 88%

func (a *Adapter) Stop() error {
    if a.ownershipCleanupCancel != nil {
        a.ownershipCleanupCancel()  // ← 只取消,不等待
    }
    // 缺少: a.ownershipTracker.Stop()

ThreadOwnershipTracker.Stop() 中的 wg.Wait() 永远不会被调用。

修复:

if a.ownershipTracker != nil {
    a.ownershipTracker.Stop()
}

3. Config Validation 静默失败 - adapter.go:72-76

严重程度: Critical | 置信度: 95%

if err := config.Validate(); err != nil {
    logger.Error("Invalid Slack config", "error", err)
    // ← 继续执行,不返回错误
}

无效配置会导致后续运行时错误难以调试。


4. ThreadKey 空值导致 Map 冲突 - thread_ownership.go:14-17

严重程度: Critical | 置信度: 90%

func NewThreadKey(channelID, threadTS string) ThreadKey {
    return ThreadKey(channelID + ":" + threadTS)  // 空 channelID/threadTS 产生冲突
}

空值会生成 "C123:" 或 ":123"` 这样的 key,导致不同线程共享相同 key。


🟠 重要问题 (High)

5. Unknown OwnerPolicy 静默变为 Public - config.go:369-392

default:
    return true  // ← 未知 policy 静默变为 public,安全风险

配置拼写错误(如 owneronly 而非 owner_only)会导致任何人都可以访问 bot。


🟡 建议改进 (Medium)

# 问题 文件
6 Persist 字段定义但未使用 config.go:54-57
7 缺少接口合规性验证 thread_ownership.go
8 缺少并发测试 thread_ownership_test.go
9 events.go 和 socketmode.go 重复决策逻辑 多文件
10 建议添加 GetOwnedThreadCount() 方法 thread_ownership.go

✅ 正面观察

  • 优秀的文档注释和设计规范 (bot-behavior-spec.md)
  • 正确的 Mutex 使用模式
  • ThreadOwnershipTracker 类型设计评分: 8.5/10
  • Bot-centric 所有权模型设计合理

📋 行动计划

合并前必须修复 (预估 15 分钟):

  1. 添加 nil check: socketmode.go:144
  2. 添加 Stop() 调用: adapter.go:252

审查结论: ⚠️ 需要修改后合并

HotPlexBot01 added 2 commits March 8, 2026 08:48
Critical fixes:
- Add nil check for ownershipTracker in socketmode.go to prevent panic
- Call ownershipTracker.Stop() in Adapter.Stop() to prevent goroutine leak
- Return early from NewAdapter on config validation failure
- Validate ThreadKey parameters to prevent empty key collisions
- Change unknown OwnerPolicy default from public to owner_only (secure default)
…o#241)

Fixes:
- Add nil check for webhook.Stop() in Adapter.Stop()
- Initialize webhook in NewAdapter failure path to prevent nil panic
- Update ThreadKey validation to reject empty parameters
- Add tests for empty ThreadKey cases
- Fix test BotToken/SigningSecret format to match validation regex
@hrygo hrygo merged commit 2eb41d4 into hrygo:main Mar 8, 2026
6 checks passed
hrygo pushed a commit that referenced this pull request Mar 16, 2026
* docs(design): add thread ownership policy design (Refs #241)

* docs(design): update thread ownership with per-bot owned_set model (Refs #241)

* docs(design): add slack thread creation flow analysis (Refs #241)

* docs(design): add bot behavior specification with basic/advanced capabilities (Refs #241)

* docs(design): add required channel behavior specification (Refs #241)

* docs(design): finalize bot behavior spec v1.0 (Refs #241)

- Add owner definition and policy configuration
- Unify decision flow diagram
- Add DM behavior and edge cases
- Clean up duplicate content
- Consolidate configuration structure

* docs(design): remove obsolete thread-ownership-policy.md (Refs #241)

* feat(slack): implement Phase 1 bot behavior spec (Refs #241)

- Add Owner policy config (owner_only, trusted, public)
- Add ThreadOwnershipTracker for thread ownership management
- Implement unified decision flow for message response
- Support multi-owner threads via @mention
- Add TTL expiration for thread ownership
- Integrate ownership logic in events.go and socketmode.go

Phase 1 MVP features implemented:
- @mention detection and response
- Owner policy configuration
- Thread ownership tracking
- Ownership transfer on @mention
- Multi-owner support
- TTL expiration

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(slack): address code review issues for Phase 1 (Refs #241)

- Implement R5: ownership release when other bots mentioned
- Add background cleanup goroutine for expired ownerships
- Add Stop() method for graceful shutdown
- Fix ownership tracking with context-based lifecycle

Issues addressed:
- P0: R5 rule implementation (ownership release)
- P1: Memory leak prevention (periodic cleanup)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(slack): address code review issues (Refs #241)

- R3: @mention transfers ownership (was of bot mentions)
- R4: Multi-bot mention creates shared ownership
- R5: @mentions excluding current owner releases ownership
- Added human用户过滤,防止非 bot 用户成为所有者
- Added concurrency安全改进 (atomic.Bool for Stop)
- 緻加单元测试覆盖并发场景

- 修复代码构建错误

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(slack): finalize Phase 1 code review fixes (Refs #241)

- Fix R3: use SetOwners for Claim ownership
- Fix R4: only set known bot IDs as owners
- Add Stop() idempotent protection with atomic.Bool
- Fix TTL test: use HasOwner
- Add CleanupExpired test
- Add UpdateLastActive test

All tests pass.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor(slack): redesign thread ownership to bot-centric model (Refs #241)

Key changes:
- ThreadOwnershipTracker now tracks threads owned by THIS bot
- Removed multi-owner tracking (OwnerIDs map)
- Simplified API: Claim(), Release(), Owns()
- Each bot instance independently tracks its owned threads
- R3/R4: Multi-bot scenarios handled by independent bot instances
- R5: Ownership release when others are mentioned

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs(design): mark Phase 1 implementation complete (Refs #241)

All Phase 1 MVP features implemented:
- @mention detection and response
- Thread reply mode
- Owner policy (owner_only/trusted/public)
- Bot-centric thread ownership tracking
- Ownership transfer (R3)
- Multi-owner support (independent tracking)
- TTL expiration

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(slack): address PR #242 review feedback (Refs #241)

Critical fixes:
- Add nil check for ownershipTracker in socketmode.go to prevent panic
- Call ownershipTracker.Stop() in Adapter.Stop() to prevent goroutine leak
- Return early from NewAdapter on config validation failure
- Validate ThreadKey parameters to prevent empty key collisions
- Change unknown OwnerPolicy default from public to owner_only (secure default)

* fix(slack): address PR #242 review feedback - round 2 (Refs #241)

Fixes:
- Add nil check for webhook.Stop() in Adapter.Stop()
- Initialize webhook in NewAdapter failure path to prevent nil panic
- Update ThreadKey validation to reject empty parameters
- Add tests for empty ThreadKey cases
- Fix test BotToken/SigningSecret format to match validation regex

---------

Co-authored-by: HotPlexBot01 <noreply@hotplex.dev>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
hrygo pushed a commit that referenced this pull request Mar 16, 2026
* docs(design): add thread ownership policy design (Refs #241)

* docs(design): update thread ownership with per-bot owned_set model (Refs #241)

* docs(design): add slack thread creation flow analysis (Refs #241)

* docs(design): add bot behavior specification with basic/advanced capabilities (Refs #241)

* docs(design): add required channel behavior specification (Refs #241)

* docs(design): finalize bot behavior spec v1.0 (Refs #241)

- Add owner definition and policy configuration
- Unify decision flow diagram
- Add DM behavior and edge cases
- Clean up duplicate content
- Consolidate configuration structure

* docs(design): remove obsolete thread-ownership-policy.md (Refs #241)

* feat(slack): implement Phase 1 bot behavior spec (Refs #241)

- Add Owner policy config (owner_only, trusted, public)
- Add ThreadOwnershipTracker for thread ownership management
- Implement unified decision flow for message response
- Support multi-owner threads via @mention
- Add TTL expiration for thread ownership
- Integrate ownership logic in events.go and socketmode.go

Phase 1 MVP features implemented:
- @mention detection and response
- Owner policy configuration
- Thread ownership tracking
- Ownership transfer on @mention
- Multi-owner support
- TTL expiration

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(slack): address code review issues for Phase 1 (Refs #241)

- Implement R5: ownership release when other bots mentioned
- Add background cleanup goroutine for expired ownerships
- Add Stop() method for graceful shutdown
- Fix ownership tracking with context-based lifecycle

Issues addressed:
- P0: R5 rule implementation (ownership release)
- P1: Memory leak prevention (periodic cleanup)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(slack): address code review issues (Refs #241)

- R3: @mention transfers ownership (was of bot mentions)
- R4: Multi-bot mention creates shared ownership
- R5: @mentions excluding current owner releases ownership
- Added human用户过滤,防止非 bot 用户成为所有者
- Added concurrency安全改进 (atomic.Bool for Stop)
- 緻加单元测试覆盖并发场景

- 修复代码构建错误

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(slack): finalize Phase 1 code review fixes (Refs #241)

- Fix R3: use SetOwners for Claim ownership
- Fix R4: only set known bot IDs as owners
- Add Stop() idempotent protection with atomic.Bool
- Fix TTL test: use HasOwner
- Add CleanupExpired test
- Add UpdateLastActive test

All tests pass.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* refactor(slack): redesign thread ownership to bot-centric model (Refs #241)

Key changes:
- ThreadOwnershipTracker now tracks threads owned by THIS bot
- Removed multi-owner tracking (OwnerIDs map)
- Simplified API: Claim(), Release(), Owns()
- Each bot instance independently tracks its owned threads
- R3/R4: Multi-bot scenarios handled by independent bot instances
- R5: Ownership release when others are mentioned

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs(design): mark Phase 1 implementation complete (Refs #241)

All Phase 1 MVP features implemented:
- @mention detection and response
- Thread reply mode
- Owner policy (owner_only/trusted/public)
- Bot-centric thread ownership tracking
- Ownership transfer (R3)
- Multi-owner support (independent tracking)
- TTL expiration

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(slack): address PR #242 review feedback (Refs #241)

Critical fixes:
- Add nil check for ownershipTracker in socketmode.go to prevent panic
- Call ownershipTracker.Stop() in Adapter.Stop() to prevent goroutine leak
- Return early from NewAdapter on config validation failure
- Validate ThreadKey parameters to prevent empty key collisions
- Change unknown OwnerPolicy default from public to owner_only (secure default)

* fix(slack): address PR #242 review feedback - round 2 (Refs #241)

Fixes:
- Add nil check for webhook.Stop() in Adapter.Stop()
- Initialize webhook in NewAdapter failure path to prevent nil panic
- Update ThreadKey validation to reject empty parameters
- Add tests for empty ThreadKey cases
- Fix test BotToken/SigningSecret format to match validation regex

---------

Co-authored-by: HotPlexBot01 <noreply@hotplex.dev>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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.

feat(slack): implement thread ownership policy for multi-bot scenarios

2 participants