fix(agent): skip final reply only when message tool sent to same chat

[Vast-Stars]

Summary

• Fix: when the message tool sends to a different chat_id (e.g. a newly created Feishu group), the final reply to the original chat was incorrectly suppressed
• Track the target chat_id in MessageTool and only skip the outbound reply when it matches the inbound message's chat_id

Changes

• pkg/tools/message.go: add sentChatID field, SentToChatID() method, reset in ResetSentInRound
• pkg/agent/loop.go: alreadySent condition now checks SentToChatID() == msg.ChatID

Test Plan

• go test ./pkg/tools/ — passed
• go test ./pkg/agent/ — passed
• go vet — clean
• Manual test: bot creates Feishu group and sends message to it, original private chat still receives the final reply

bug behavior

拉群之后，能在新群里发消息，但是在原始对话里无法继续。
After bot create group and send messages in the new group, the bot can not send messages in the old chat, which still have a streaming stuck card

[TempestShaw]

It would be better to keep both claude.md and README.local.md in the local environment only.

[nikolasdehor]

The core fix in loop.go is correct: checking mt.SentToChatID() == msg.ChatID before suppressing the reply is the right condition. Without this, sending to a newly created group would swallow the reply to the original private chat.

However, this PR includes several unrelated changes that should be split out:

1. CLAUDE.md and README.local.md -- these are internal fork documentation files and should not be part of an upstream PR. They contain company-specific branching workflows (internal-main, upstream sync instructions in Chinese). Please remove these from this PR.

2. Feishu mention/sender changes (stripMentionPlaceholders signature change, sender_open_id metadata, sender identity prepending) -- these are significant behavioral changes that deserve their own PR. The mention preservation feature (@Name(open_id:xxx)) is useful but orthogonal to the message tool fix.

3. sentChatID tracking in message.go -- this is the correct minimal change for the fix. Using atomic.Value for the chat ID is appropriate.

Regarding the sentChatID implementation specifically:

• Single chat ID tracking: sentChatID only stores the last sent chat ID. If the tool sends to multiple different chats in one round, only the last one is recorded. This means the alreadySent check could incorrectly suppress a reply if the last send happened to target the same chat as the inbound message. Is this scenario possible? If so, consider tracking a set of chat IDs.

I would suggest splitting this into at least two PRs: (a) the message tool chat ID fix, and (b) the Feishu mention/sender identity improvements.

[Vast-Stars]

Thanks for the detailed review!

1 & 2 — Fixed: Force-pushed a clean branch that only contains the core message
tool fix (loop.go + message.go). CLAUDE.md, README.local.md, and the
Feishu mention/sender identity changes have all been removed from this PR. The
mention/sender improvements will be submitted as a separate PR.

3 — Fixed: Good catch! Replaced the single sentChatID (atomic.Value) with
a map[string]struct{} set that tracks all chat_ids sent to in a round. Now if
the agent sends to chats A, B, C, D in one round, the final reply is correctly
suppressed for any of those chats — not just the last one.

[yinwm]

Code Review

Thanks for the fix! The approach is correct, but I have a few concerns that should be addressed before merging.

---

✅ What's Good

1. Correct fix approach - Accurately identified the issue and the solution is straightforward
2. Clear comments - The code comments explain the "why" behind the change
3. Thread-safe - Properly using sync.Mutex to protect sentChatIDs
4. Multi-chat support - Good improvement in the second commit to use a map for tracking multiple chat_ids

---

⚠️ Issues to Address

1. State Update Order Creates Race Window (Important)

// Current code order
t.sentInRound.Store(true)           // ① Set first
t.mu.Lock()
t.sentChatIDs[chatID] = struct{}{}  // ② Update later
t.mu.Unlock()

Problem: There's a theoretical window where HasSentInRound() returns true but sentChatIDs hasn't been updated yet. While the current call order (check HasSentInRound() first, then HasSentToChatID()) won't trigger this issue, the code isn't robust.

Suggestion: Reorder to ensure state consistency:

t.mu.Lock()
if t.sentChatIDs == nil {
    t.sentChatIDs = make(map[string]struct{})
}
t.sentChatIDs[chatID] = struct{}{}
t.mu.Unlock()
t.sentInRound.Store(true)  // Mark as sent last

2. Redundant Field Lacks Design Documentation

sentInRound (atomic.Bool) is functionally redundant with len(sentChatIDs) > 0. Is it kept as a "fast path" to avoid lock contention? If so, please add a comment explaining the design intent.

3. Missing Unit Tests

The PR description mentions tests passed, but there are no new unit tests for this bug fix. Please add tests for HasSentToChatID behavior:

func TestMessageTool_HasSentToChatID(t *testing.T) {
    mt := NewMessageTool()
    mt.SetSendCallback(func(channel, chatID, content string) error { return nil })
    mt.ResetSentInRound()
    
    // Send to chat1
    mt.Execute(context.Background(), map[string]any{
        "content": "hello",
        "channel": "test",
        "chat_id": "chat1",
    })
    
    assert.True(t, mt.HasSentToChatID("chat1"))
    assert.False(t, mt.HasSentToChatID("chat2"))
    assert.True(t, mt.HasSentInRound())
}

4. Nil Map Handling in HasSentToChatID

func (t *MessageTool) HasSentToChatID(chatID string) bool {
    t.mu.Lock()
    defer t.mu.Unlock()
    _, ok := t.sentChatIDs[chatID]
    return ok
}

While reading from a nil map is safe in Go (returns false), consider adding an explicit check or comment for clarity:

if t.sentChatIDs == nil {
    return false
}

---

📝 Minor Suggestions

1. Import grouping: The new sync import should be grouped with sync/atomic:

   import (
       "context"
       "fmt"
       "sync"
       "sync/atomic"
   )

---

Summary

Item	Status
Bug Fix	✅ Correct
Thread Safety	✅ Correct
Code Quality	⚠️ State update order needs adjustment
Test Coverage	❌ Missing unit tests
Maintainability	⚠️ Redundant field needs documentation

Please address items 1 and 3 (state update order and unit tests) before merging.

[sipeed-bot]

@Vast-Stars Hi! This PR has had no activity for over 2 weeks, so I'm closing it for now to keep things tidy. If it's still relevant, feel free to reopen it anytime and we'll pick it back up.