feat(messaging/slack): Assistant branding + DataTableBlock skills (#565)

[aaronwong1989]

Summary

Continues Slack Block Kit upgrade (#565) — Phase 1.2 + Phase 3.

Phase 1.2: Assistant Status Branding

• SlackConfig/SlackBotConfig 新增 display_name/icon_emoji 字段（bot 级覆盖 + 平台级 fallback）
• fillSlackExtras 注入 extras，Adapter.ConfigureWith 读取并存储
• SetAssistantStatus 传递 Username/IconEmoji 到 Slack Assistant API

Phase 3: DataTableBlock Skills List

• 每个 SkillGroup → DataTableBlock（columns: Name / Description，caption: source + count）
• 行数保护：超过 99 行截断并显示 ... and N more
• 消除分页需求：每个 DataTableBlock 占 1 block 位
• Fallback: isInvalidBlocksError → postSkillsMessageFallback（纯文本）

Files Changed

File	Change
internal/config/config.go	+DisplayName/IconEmoji to SlackConfig + SlackBotConfig
cmd/hotplex/messaging_init.go	fillSlackExtras injects branding with bot-level override
internal/messaging/slack/adapter.go	+displayName/iconEmoji fields, read in ConfigureWith
internal/messaging/slack/status.go	SetAssistantStatus passes Username/IconEmoji
internal/messaging/slack/skills_list.go	DataTableBlock 替代 SectionBlock 分页
internal/messaging/slack/validator.go	(无 CardBlock/CarouselBlock dead code，已清理)
docs/specs/slack-block-kit-upgrade.md	更新验收标准

Verification

• make check 全量通过（lint 0 issues, tests all pass, build OK）

Closes #565

🤖 Generated with Claude Code

[hotplex-ai]

Code Review — hotplex-ai

Verdict: APPROVE | P0:0 P1:1 P2:0 P3:1

发现

[P1] sanitizeCarouselBlock 未处理 0-card carousel (internal/messaging/slack/validator.go:416-421)

validateCarouselBlock 正确拒绝 <1 card 的 carousel，但 sanitizeCarouselBlock 只裁剪上限，不处理下限。0-card carousel 会穿过 sanitize 被发送到 Slack API 导致 invalid_blocks 拒绝。应与 sanitizeActionBlock（nil return → drop block）保持一致。

[P3] SkillsBlockSoftLimit/SkillsBlockHardLimit 成为死代码 (internal/messaging/skills_helpers.go)

本 PR 移除了 skills_list.go 中对这两个常量的唯一引用。建议清理。

---

pnpm-workspace.yaml 不属于本 PR scope，但不阻塞，建议后续拆分。

[hotplex-ai]

[P1] sanitizeCarouselBlock 缺少 0-card 下限处理。当 len(b.Elements) == 0 时应返回 nil 让 SanitizeBlocks drop 该 block，与 sanitizeActionBlock 模式一致：

func sanitizeCarouselBlock(b *slack.CarouselBlock) *slack.CarouselBlock {
    if len(b.Elements) > maxCarouselCards {
        b.Elements = b.Elements[:maxCarouselCards]
    }
    if len(b.Elements) < minCarouselCards {
        return nil // drop, consistent with sanitizeActionBlock
    }
    return b
}

同时 SanitizeBlocks 的 case *slack.CarouselBlock: 需 nil-check：

case *slack.CarouselBlock:
    if sb := sanitizeCarouselBlock(b); sb != nil {
        sanitized = append(sanitized, sb)
    }

[hotplex-ai]

Code Review — hotplex-ai (修正)

Verdict: REQUEST_CHANGES | P0:0 P1:1 P2:0 P3:1

上方 COMMENT 已含详细发现和 inline 建议。核心 P1：

sanitizeCarouselBlock 缺少 0-card 下限处理 → 应返回 nil drop block，与 sanitizeActionBlock 模式对齐。

[hotplex-ai]

Code Review — hotplex-ai

Verdict: APPROVE | P0:0 P1:0 P2:2 P3:3

Summary

Branding 实现（DisplayName/IconEmoji）配置注入链路完整、bot 级覆盖正确。Skills 列表从 SectionBlock 分页迁移到 DataTableBlock，逻辑简洁。Fallback 模式在 caller 层（conn_events.go:102-106）保留完好。

主要关注两点：PR 描述与实际实现不匹配（说 CardBlock/CarouselBlock 但用了 DataTableBlock），以及 validator 中 CardBlock/CarouselBlock 代码为无消费者的 dead code。

---

P2 Findings

1. PR title/body 与实际实现不符 — skills_list.go
PR 标题和 body 声明使用 CardBlock/CarouselBlock 展示 skills 列表，但实际实现 buildSkillGroupTable 产出 DataTableBlock。GitHub 上的 PR 描述仍引用旧的 CardBlock 方案，对审核者和后续 git archaeology 造成误导。
→ 建议：更新 PR body 使其与实际代码一致。

2. CardBlock/CarouselBlock validator/sanitizer 为 dead code — validator.go:115-125, 204-228, 273-278, 408-425
PR 新增了 validateCardBlock、validateCarouselBlock、sanitizeCardBlock、sanitizeCarouselBlock 共 4 个函数及 4 个常量（~60 行），但整个代码库无任何路径产出 *slack.CardBlock 或 *slack.CarouselBlock。如果是为了后续阶段预备，建议加注释说明（如 // Future: CardBlock/CarouselBlock for Phase X），否则违反 YAGNI。
→ 建议：移除 dead code 或标注意图。

P3 Findings

3. buildSkillGroupTable 无行数保护 — skills_list.go:50-52
for _, s := range g.Entries 无条件添加所有 skill 为行。若单组 >99 条，会触发 Slack invalid_blocks 拒绝。虽然 caller 的 isInvalidBlocksError fallback 会兜底，但浪费了一次 API 调用。可在 buildSkillGroupTable 中添加 maxDataTableRows 截断。
→ 风险极低（典型 skill group 远小于 100 条），fallback 已覆盖。

4. pnpm-workspace.yaml 与本 PR 无关 — webchat/pnpm-workspace.yaml
sharp 构建配置与 Slack branding/Block Kit 功能完全无关。混入 feature PR 会污染 blame 历史且影响 revert。
→ 建议：单独提交或移除。

5. SkillsHeader(d, 1, 1) 硬编码分页参数 — skills_list.go:24
移除分页后 header 始终显示 "page 1/1"。SkillsHeader 函数签名仍接受 page/total 参数但语义已变。
→ 低优先，可在后续清理中简化 SkillsHeader 签名。

[hotplex-ai]

CardBlock/CarouselBlock case added to ValidateBlocks but no code path produces these block types. The skills_list.go uses DataTableBlock instead. Consider removing or adding a comment like // Future: CardBlock/CarouselBlock for skills carousel to document intent.

[hotplex-ai]

sanitizeCardBlock + sanitizeCarouselBlock (lines 418-425) are dead code — no call path exercises these. If kept for future use, please add a tracking comment. Otherwise remove to reduce maintenance surface.

[hotplex-ai]

buildSkillGroupTable adds all g.Entries unconditionally. If a single group has >99 skills, the DataTableBlock will exceed maxDataTableRows=100 and Slack will reject it. The caller fallback handles this, but pre-send truncation (or calling SanitizeBlocks) would avoid the wasted API call. Low risk since typical groups are small.

[hotplex-ai]

After removing pagination, SkillsHeader(d, 1, 1) always produces "page 1/1". The function signature still takes page/total params but the semantics have changed. Consider simplifying the header generation in a follow-up.

[hotplex-ai]

This file (sharp build config) is unrelated to the Slack branding/Block Kit feature. Should be in a separate commit to keep the feature branch clean for revert/bisect.

[hotplex-ai]

Code Review — hotplex-ai

Verdict: ✅ APPROVE | P0:0 P1:0 P2:2 P3:2

PR 实现正确：branding 配置链路（config → fillSlackExtras → Adapter.ConfigureWith → SetAssistantStatus）完整，DataTableBlock skills 替代分页方案简洁合理。以下为建议性改进项。

---

P2 Findings

1. [P2] 验收标准与实现不匹配 — spec 仍写 CarouselBlock 但代码用 DataTableBlock
📄 docs/specs/slack-block-kit-upgrade.md:170-171

验收标准勾选了 Skills 列表用 CarouselBlock 展示（每个 SkillGroup → CardBlock，包裹在 CarouselBlock），但实际实现是 DataTableBlock。PR body 描述准确，但 spec 文件 Phase 3 验收标准文字需同步更新为 DataTableBlock 措辞，避免后续维护混淆。

2. [P2] block 总数无硬上限保护
📄 internal/messaging/slack/skills_list.go:31-33

旧代码有 SkillsBlockHardLimit=50 截断保护，重构后移除了该 guard。每个 DataTableBlock 占 1 block 位，若 skill groups 超过 49 个将超过 Slack 50-block 限制。实践中概率低但非零。建议在循环后加回一个 block count cap：

if len(blocks) > 50 {
    blocks = blocks[:50]
}

P3 Findings

3. [P3] PR 标题提及 "CardBlock/CarouselBlock" 但实际未使用
PR 标题：feat(messaging/slack): Assistant branding + CardBlock/CarouselBlock skills，但代码中无 CardBlock/CarouselBlock 相关实现。建议更新为 feat(messaging/slack): Assistant branding + DataTableBlock skills。

4. [P3] SkillsHeader(d, 1, 1) 语义哨兵不够明确
📄 internal/messaging/slack/skills_list.go:24

硬编码 1, 1 作为"无分页"的哨兵值语义不够清晰。虽然 SkillsHeader 内部 total > 1 guard 保证了正确性，但建议添加一行注释说明意图（如 // page=1, total=1: non-paginated, suppresses "Part X/Y" suffix）。

[hotplex-ai]

[P2] block 总数无硬上限

旧代码有 SkillsBlockHardLimit=50 截断，重构后已移除。每个 DataTableBlock = 1 block 位，若 skill groups > 49 个会超 Slack 50-block 限制。

建议循环后加回 guard：

if len(blocks) > 50 {
    blocks = blocks[:50]
}

[hotplex-ai]

[P3] 语义哨兵注释建议

SkillsHeader(d, 1, 1) 硬编码 1, 1 作为"无分页"信号。虽然 total > 1 guard 保证了正确性，但建议加注释说明意图：

header := messaging.SkillsHeader(d, 1, 1) // non-paginated: suppresses "Part X/Y" suffix

[hotplex-ai]

[P2] 验收标准与实现不匹配

此行勾选了 CarouselBlock 展示，但实际 skills_list.go 使用的是 DataTableBlock。建议更新为：

[x] Phase 3: Skills 列表用 DataTableBlock 展示（每个 SkillGroup → DataTableBlock，columns: Name/Description）

保持 spec 与代码一致。

[hotplex-ai]

Code Review — hotplex-ai

Verdict: ✅ APPROVE | P0:0 P1:0 P2:1 P3:2

Findings

P2 — SkillsBlockSoftLimit / SkillsBlockHardLimit 已成为死代码
internal/messaging/skills_helpers.go:12-13

skills_list.go 重构后改用 maxBlocksPerMessage（validator.go:13）做 block 计数保护，这两个常量不再被任何路径引用。建议在本 PR 或后续 cleanup 中移除，避免混淆哪组限制实际生效。

P3 — block count guard 使用 >= 而非 >，与 validator 语义不一致
internal/messaging/slack/skills_list.go:33

validator 用 > maxBlocksPerMessage（允许恰好 100），而此处用 >=（最多 99）。不是 bug（更安全），但阈值不一致可能误导后续开发者。可考虑加注释说明故意预留 1 个 slot。

P3 — SkillsPerPage 现仅用于 fallback 路径
internal/messaging/skills_helpers.go:14

主路径已改用 maxDataTableRows（DataTableBlock 行数控制），SkillsPerPage 仅在 postSkillsMessageFallback 和飞书适配器中使用。加个注释说明其 fallback 用途即可。

Summary

Branding 层实现干净（bot 级覆盖 → 平台级 fallback），DataTableBlock 重构合理。无安全/并发/逻辑问题。以上均为低优先级代码卫生建议。

[hotplex-ai]

Code Review — hotplex-ai

Verdict: APPROVE | P0:0 P1:0 P2:0 P3:3

P3 Findings (non-blocking)

P3-1: sendSkillsList 静默截断 skill groups 未提示用户 (internal/messaging/slack/skills_list.go:34)
每个 DataTableBlock 占 1 block 位，当 group 数量超过 maxBlocksPerMessage-1 时循环 break 但无任何截断提示。header 显示 Skills (N) 总数但实际只展示子集。实践中 99 个 skill source 极为罕见（当前阈值 100-1=99），fallback 纯文本路径包含全部 groups。建议：若截断发生可在 fallback 文本中追加 ... and X more groups 提示。[UNCERTAIN] — 触发概率极低。

P3-2: SetAssistantStatus godoc 未说明 branding 职责 (internal/messaging/slack/status.go:307)
函数名和文档仅描述 "set assistant status text"，未提及同时也设置 Username/IconEmoji branding。建议补充 godoc 说明 branding 副作用。

P3-3: 新增 display_name/icon_emoji 配置字段缺 config.yaml 示例 (internal/config/config.go:270-272)
spec 文档已更新但 configs/config.yaml 未添加示例条目，用户无参考配置。建议作为 follow-up 补充。

Summary

• 前次 review 发现（dead code 常量 + block guard 语义）已全部修复 ✓
• 配置向后兼容：omitempty + 零值默认，无 migration 影响 ✓
• 无 API 签名变更，无下游破坏 ✓
• branding 字段遵循 Adapter 已有的 write-once-in-ConfigureWith 模式，无需 mutex ✓

[hotplex-ai]

P3 [UNCERTAIN]: 当 group 数量超过 maxBlocksPerMessage-1 时此处 break 静默丢弃剩余 groups，header 却显示总数。实践中 99 groups 极为罕见，fallback 纯文本包含全部内容，影响很小。可考虑在截断时追加提示或在 fallback 文本中注明。

[hotplex-ai]

P3: SetAssistantStatus 现在同时设置 Username/IconEmoji branding，但 godoc 仅描述 status text 职责。建议补充 branding 行为说明，方便后续调用者理解副作用。

[hotplex-ai]

P3: display_name 和 icon_emoji 新增配置字段在 configs/config.yaml 中缺少示例条目。spec 文档已更新但用户无参考配置，建议作为 follow-up 补充。

[hotplex-ai]

Code Review — hotplex-ai

Verdict: ✅ APPROVE | P0:0 P1:0 P2:0 P3:2

Two non-blocking P3 observations — both cosmetic/defense-in-depth, no action required for merge.

---

P3 Findings

[P3] Misleading godoc: Branding for Assistant status and messages (config.go:270)
The comment says "and messages" but DisplayName/IconEmoji are only consumed in SetAssistantStatus (status.go:320-325). Regular PostMessageContext calls do not use these fields. Suggest updating to Branding for Assistant status (paid workspaces).

[P3] Block ID uses g.Source without sanitization (skills_list.go:38)
fmt.Sprintf("skills_%s_%d", g.Source, i) embeds Source directly into a Slack block ID. Currently safe — Source is always "project" or "global" (constants). Defense-in-depth: if custom source names are ever introduced, sanitize or truncate to 255 chars. The fallback path (isInvalidBlocksError → plain text) already handles API rejection gracefully.

---

Reviewed Changes

• ✅ Assistant branding: config → extras injection → adapter → SetAssistantStatus (clean additive chain, backward compatible)
• ✅ DataTableBlock skills list: eliminates pagination, one table per group, row cap at 99
• ✅ Dead code removal: SkillsBlockSoftLimit/SkillsBlockHardLimit (zero references confirmed)
• ✅ Spec doc updated to reflect actual implementation
• ✅ All backward compatible: new fields optional with omitempty, zero-value = no change

[hotplex-ai]

Code Review — hotplex-ai

Verdict: ✅ APPROVE | P0:0 P1:0 P2:0 P3:0

4-agent 并行审查完成（Bug/安全/并发 · 合规/DRY · 历史/兼容 · SOLID/性能/文档），所有维度 PASS。

变更摘要：

• SlackConfig/SlackBotConfig 新增 display_name/icon_emoji 品牌字段（向后兼容，omitempty）
• fillSlackExtras 按既有的平台→Bot fallback 模式传播品牌配置
• SetAssistantStatus 将品牌字段应用到 Slack Assistant API
• Skills 列表从分页 Markdown SectionBlock 重写为单页 DataTableBlock（含 fallback）

审查亮点：

• Mutex 合规：Adapter.mu 显式字段，未嵌入/传指针
• 配置传播：品牌字段与凭证字段遵循相同的 happens-before 模式（ConfigureWith → Start）
• 删除的 SkillsBlockSoftLimit/SkillsBlockHardLimit 无外部引用
• Block 限制：maxBlocksPerMessage-1 guard + maxDataTableRows-1 row cap + isInvalidBlocksError fallback 三层防护
• 无安全注入风险：品牌值为配置文件注入，非用户输入