From 20de0e332ea708ac8d8460cfc2f900cc2208f0bb Mon Sep 17 00:00:00 2001 From: viettranx Date: Sat, 11 Apr 2026 21:45:37 +0700 Subject: [PATCH] feat(feishu): add /addwriter /removewriter /writers commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds parity with Telegram and Discord for file-writer management commands, closing the UX gap where users saw an error mentioning /addwriter but the Feishu channel had no handler. - New maybeHandleWriterCommand routes /addwriter, /removewriter, /writers from the Feishu inbound flow. Runs at step 5a — after checkGroupPolicy — so commands never bypass allowlist or pairing enforcement. Step 2a rejects slash commands in DM chats early so users get a clear hint without waking the agent pipeline. - Target user is identified via reply-to (fetches parent message sender) or first non-bot @mention. A bare /addwriter with no target shows the usage hint instead of silently self-granting, preventing accidental privilege capture in empty-writer groups. - Refuses to run while botOpenID is unresolved so a @mention of the bot itself cannot be mistaken for a human target. - 10s context timeout on each handler bounds worst-case Lark API latency (parent message lookup, permission store access). - feishu.New() gains variadic Option parameter with WithAgentStore and WithConfigPermStore mirroring Telegram's pattern. The gateway now wires pgStores.Agents and pgStores.ConfigPermissions into Feishu channel on startup. - 12 new unit tests with fakeConfigPermStore and httptest Lark server cover DM rejection, nil-store graceful degradation, bootstrap via self-mention, bare-command usage hint, bot-probe race refusal, non-writer rejection, grant via mention, remove last-writer guard, empty and populated list output, reply-to target resolution via Lark im/v1/messages lookup, and non-command passthrough. 40 total tests in the feishu package, all green with -race. Closes #818. --- cmd/gateway_channels_setup.go | 16 +- docs/05-channels-messaging.md | 23 + docs/17-changelog.md | 17 +- internal/channels/feishu/bot.go | 16 + internal/channels/feishu/commands_writers.go | 290 ++++++++++++ .../channels/feishu/commands_writers_test.go | 446 ++++++++++++++++++ internal/channels/feishu/feishu.go | 45 +- 7 files changed, 824 insertions(+), 29 deletions(-) create mode 100644 internal/channels/feishu/commands_writers.go create mode 100644 internal/channels/feishu/commands_writers_test.go diff --git a/cmd/gateway_channels_setup.go b/cmd/gateway_channels_setup.go index 2e0d0548..d9d97197 100644 --- a/cmd/gateway_channels_setup.go +++ b/cmd/gateway_channels_setup.go @@ -126,12 +126,18 @@ func registerConfigChannels(cfg *config.Config, channelMgr *channels.Manager, ms if cfg.Channels.Feishu.Enabled { if cfg.Channels.Feishu.AppID == "" { recordMissingConfig(channels.TypeFeishu, "Set channels.feishu.app_id in config.") - } else if f, err := feishu.New(cfg.Channels.Feishu, msgBus, pgStores.Pairing, nil); err != nil { - channelMgr.RecordFailure(channels.TypeFeishu, "", err) - slog.Error("failed to initialize feishu channel", "error", err) } else { - channelMgr.RegisterChannel(channels.TypeFeishu, f) - slog.Info("feishu/lark channel enabled (config)") + feishuOpts := []feishu.Option{ + feishu.WithAgentStore(pgStores.Agents), + feishu.WithConfigPermStore(pgStores.ConfigPermissions), + } + if f, err := feishu.New(cfg.Channels.Feishu, msgBus, pgStores.Pairing, nil, feishuOpts...); err != nil { + channelMgr.RecordFailure(channels.TypeFeishu, "", err) + slog.Error("failed to initialize feishu channel", "error", err) + } else { + channelMgr.RegisterChannel(channels.TypeFeishu, f) + slog.Info("feishu/lark channel enabled (config)") + } } } } diff --git a/docs/05-channels-messaging.md b/docs/05-channels-messaging.md index dc60a7dd..bf13e33b 100644 --- a/docs/05-channels-messaging.md +++ b/docs/05-channels-messaging.md @@ -397,6 +397,29 @@ When a user pastes a Lark docx (document) URL in a message, the channel automati **Configuration**: No new config flags. Supported document type and cache tunables (8000 rune limit, 10-URL cap, 5-min TTL, 128-entry cache) are hardcoded. +### Writer Management Commands + +Group chats support file-write permission management via slash commands. Permissions are scoped to the group via `group:feishu:`. DM users who attempt these commands receive a hint that they only work in groups. + +**Commands** (group-only): + +| Command | Description | Requires Target | Permission | +|---------|-------------|:---:|:---:| +| `/addwriter <@user or reply>` | Grant file_writer permission to target user | Yes | Writers only | +| `/removewriter <@user or reply>` | Revoke file_writer permission from target user | Yes | Writers only | +| `/writers` | List current group writers with displayName | No | -- | + +**Target specification**: Commands require explicit identification via reply-to or @mention. Bare `/addwriter` without a target is rejected — prevents accidental privilege capture. + +**Bootstrap behavior**: Groups with no writers allow the first writer to grant themselves via `/addwriter @self` (explicit self-mention). This enables initial configuration without external admin intervention. + +**Authorization**: +- Only existing writers can manage the writer list (enforce via `IsGroupFileWriter` check) +- Last-writer guard: If removing a writer would leave zero writers, operation is rejected with user-facing message +- Database errors are fail-open; security issues are logged as `security.writer_check_failed` + +**Implementation**: Timeout of 10 seconds bounds Feishu API calls. Requires `AgentStore` and `ConfigPermissionStore` wired to the Feishu channel via constructor options. + --- ## 7. Discord diff --git a/docs/17-changelog.md b/docs/17-changelog.md index e8c6189f..1936d7c8 100644 --- a/docs/17-changelog.md +++ b/docs/17-changelog.md @@ -34,21 +34,14 @@ All notable changes to GoClaw Gateway are documented here. Format follows [Keep ### Fixed -#### Feishu/Lark Thread Reply Routing — Issue #818 Phase 1 (2026-04-11) -- **Thread detection**: Inbound messages with `thread_id` now properly route responses back to the same Feishu thread via `/open-apis/im/v1/messages/{id}/reply` endpoint -- **Metadata propagation**: New `feishu_reply_target_id` metadata key added to `routingMetaKeys` allowlist so all outbound messages (text, cards, files, reactions) land in the correct thread -- **Graceful fallback**: If thread root is deleted, channel falls back to regular `SendMessage()` for robustness +#### Feishu/Lark Writer Management Commands — Issue #818 Closed (2026-04-11) +- **Issue #818 resolution**: Closes UX gap where users saw `/addwriter` error messages but Feishu had no handler +- **Phase 1 — Thread reply routing**: Inbound messages with `thread_id` now properly route responses back to the same Feishu thread via `/open-apis/im/v1/messages/{id}/reply`. New `feishu_reply_target_id` metadata key included in `routingMetaKeys` allowlist. Graceful fallback to `SendMessage()` if thread root deleted +- **Phase 2 — Document auto-fetch**: Pasted Lark docx URLs auto-detected and fetched via `/open-apis/docx/v1/documents/{id}/raw_content`. Content injected as `[Lark Doc: URL]` markers. LRU cache (128 entries, 5-min TTL) + 8000-rune truncation per document. Requires `docx:document:readonly` permission + owner grant +- **Phase 3 — Writer management commands**: Added `/addwriter <@user or reply>`, `/removewriter`, `/writers` for group file-write permission control. Group-only (DMs rejected early). Requires existing writer authorization. Last-writer guard prevents removing final writer. Empty-writer groups allow bootstrap via explicit `/addwriter @self`. 10s timeout bounds Feishu API calls ### Added -#### Feishu/Lark Document URL Auto-Fetch — Issue #818 Phase 2 (2026-04-11) -- **Automatic docx fetching**: Users paste Lark docx URLs in chat; channel auto-detects and fetches raw text via `/open-apis/docx/v1/documents/{id}/raw_content` -- **Transparent context injection**: Document content embedded as `[Lark Doc: URL] ... [End of Lark Doc]` markers, visible to agent for reasoning -- **Smart caching**: LRU cache per channel instance (128 entries, 5-minute TTL) reduces redundant API calls -- **Rune-safe truncation**: 8000-rune limit per document prevents token budget overrun -- **Access control**: Requires bot app `docx:document:readonly` permission + per-document grant from owner. Access denied displays inline marker with grant instructions -- **Safeguards**: Docx-only (sheets/base/wiki deferred), max 10 URLs per message, soft-fail on errors (no message blocks) - ### Testing #### Test Coverage Improvement — Wave 1-3 (2026-04-11) diff --git a/internal/channels/feishu/bot.go b/internal/channels/feishu/bot.go index 87bcffd8..be0c95d2 100644 --- a/internal/channels/feishu/bot.go +++ b/internal/channels/feishu/bot.go @@ -58,6 +58,15 @@ func (c *Channel) handleMessageEvent(ctx context.Context, event *MessageEvent) { return } + // 2a. Slash commands in DMs are rejected early with a clear hint so + // they never reach the agent pipeline (otherwise users typing + // "/addwriter" in a DM would waste an LLM turn). The full writer + // command router is gated behind group policy below at step 5a. + if mc.ChatType != "group" && c.isWriterSlashCommand(mc) { + c.sendCommandReply(ctx, mc, "This command only works in group chats.") + return + } + // 3. Resolve sender name (cached) senderName := c.resolveSenderName(ctx, mc.SenderID) @@ -86,6 +95,13 @@ func (c *Channel) handleMessageEvent(ctx context.Context, event *MessageEvent) { return } + // 5a. Writer management slash commands run AFTER the group policy + // gate so commands cannot bypass allowlists or pairing. Commands + // short-circuit the agent pipeline to avoid consuming LLM tokens. + if c.maybeHandleWriterCommand(ctx, mc) { + return + } + // 6. RequireMention check — record to history if not mentioned requireMention := true if c.cfg.RequireMention != nil { diff --git a/internal/channels/feishu/commands_writers.go b/internal/channels/feishu/commands_writers.go new file mode 100644 index 00000000..94c27a6f --- /dev/null +++ b/internal/channels/feishu/commands_writers.go @@ -0,0 +1,290 @@ +package feishu + +import ( + "context" + "encoding/json" + "fmt" + "log/slog" + "strings" + "time" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// writerCommandTimeout bounds the worst-case latency of a writer management +// command (DB lookups + optional parent-message fetch + send reply). Mirrors +// Discord's 10s handler timeout so a flaky Lark backend cannot block the +// inbound message goroutine indefinitely. +const writerCommandTimeout = 10 * time.Second + +// isWriterSlashCommand reports whether the message content begins with one +// of the writer management slash commands. Used to short-circuit DM slash +// commands early, before any agent routing or group-policy gating runs. +func (c *Channel) isWriterSlashCommand(mc *messageContext) bool { + text := strings.TrimSpace(mc.Content) + if !strings.HasPrefix(text, "/") { + return false + } + cmd := strings.ToLower(strings.SplitN(text, " ", 2)[0]) + switch cmd { + case "/addwriter", "/removewriter", "/writers": + return true + } + return false +} + +// resolveAgentUUID converts the channel's configured agent key (which may be +// either a raw UUID or an agent_key string) into a canonical UUID the +// ConfigPermissionStore expects. Required by all writer commands — if the +// channel has no agent context, the commands disable themselves with a clear +// message rather than crashing. +func (c *Channel) resolveAgentUUID(ctx context.Context) (uuid.UUID, error) { + key := c.AgentID() + if key == "" { + return uuid.Nil, fmt.Errorf("no agent key configured") + } + if id, err := uuid.Parse(key); err == nil { + return id, nil + } + if c.agentStore == nil { + return uuid.Nil, fmt.Errorf("agent store unavailable") + } + ctx = store.WithTenantID(ctx, c.TenantID()) + agent, err := c.agentStore.GetByKey(ctx, key) + if err != nil { + return uuid.Nil, fmt.Errorf("agent %q not found: %w", key, err) + } + return agent.ID, nil +} + +// maybeHandleWriterCommand inspects an inbound Feishu message for writer +// management slash commands and handles them in-channel (without routing to +// the agent pipeline). Returns true when the message was consumed as a +// command so the caller can short-circuit. +// +// Supported commands (group chats only): +// - /addwriter — grant file_writer permission to a target user +// - /removewriter — revoke file_writer permission +// - /writers — list current writers in the group +// +// Target user is identified by reply-to first, then by the first @mention +// that is not the bot itself. Mirrors Telegram / Discord patterns for +// consistency across channels. +func (c *Channel) maybeHandleWriterCommand(ctx context.Context, mc *messageContext) bool { + text := strings.TrimSpace(mc.Content) + if text == "" || !strings.HasPrefix(text, "/") { + return false + } + cmd := strings.SplitN(text, " ", 2)[0] + switch strings.ToLower(cmd) { + case "/addwriter": + c.handleFeishuWriterCommand(ctx, mc, "add") + return true + case "/removewriter": + c.handleFeishuWriterCommand(ctx, mc, "remove") + return true + case "/writers": + c.handleFeishuListWriters(ctx, mc) + return true + } + return false +} + +// sendCommandReply posts a short text reply to the chat where the command +// was received. Uses the normal sendText path so thread routing remains +// consistent with the rest of the channel. +func (c *Channel) sendCommandReply(ctx context.Context, mc *messageContext, text string) { + receiveIDType := resolveReceiveIDType(mc.ChatID) + replyTarget := "" + if mc.ThreadID != "" { + replyTarget = mc.MessageID + } + if err := c.sendText(ctx, mc.ChatID, receiveIDType, text, replyTarget); err != nil { + slog.Warn("feishu.writer_cmd.reply_failed", "error", err, "chat_id", mc.ChatID) + } +} + +// resolveWriterTarget selects the target user for grant/revoke. +// Preference: reply-to (via ParentID → fetch sender) first, then first +// non-bot @mention. Returns empty strings if no target can be determined. +func (c *Channel) resolveWriterTarget(ctx context.Context, mc *messageContext) (userID, displayName string) { + // Reply-to path: fetch parent message to recover its sender open_id. + if mc.ParentID != "" && c.client != nil { + resp, err := c.client.GetMessage(ctx, mc.ParentID) + if err == nil && len(resp.Items) > 0 && resp.Items[0].Sender.ID != "" { + id := resp.Items[0].Sender.ID + name := c.resolveSenderName(ctx, id) + return id, name + } + } + // Mention path: first non-bot mention in the command text. + for _, m := range mc.Mentions { + if m.OpenID == "" || m.OpenID == c.botOpenID { + continue + } + return m.OpenID, m.Name + } + return "", "" +} + +// handleFeishuWriterCommand implements /addwriter and /removewriter. +// +// The target user MUST be specified explicitly — either by reply-to or by +// @mention. Target-less self-grant is intentionally NOT supported: a curious +// user exploring the command should not accidentally capture first-writer +// privilege in an empty group. To self-grant, users can @mention themselves. +// This aligns Feishu's semantics with Telegram and Discord. +// +// The only "bootstrap" carveout is that when the writer list is empty, any +// sender is allowed to pass the authorization gate — so whoever types the +// first /addwriter @target gets to seed the allowlist. +func (c *Channel) handleFeishuWriterCommand(parentCtx context.Context, mc *messageContext, action string) { + // Group policy + DM rejection are already enforced by bot.go before + // this handler is invoked — the DM check here is belt-and-suspenders + // for callers that bypass the normal pipeline (e.g. direct test calls). + if mc.ChatType != "group" { + c.sendCommandReply(parentCtx, mc, "This command only works in group chats.") + return + } + if c.configPermStore == nil { + c.sendCommandReply(parentCtx, mc, "File writer management is not available.") + return + } + // When the bot probe has not yet resolved the bot's open_id, mention + // filtering cannot distinguish bot self-mentions. Refuse to run writer + // commands until the probe completes so a user cannot accidentally + // grant the bot itself as a file writer via /addwriter @bot. + if c.botOpenID == "" { + c.sendCommandReply(parentCtx, mc, "Bot identity not yet resolved — please retry in a moment.") + return + } + + ctx, cancel := context.WithTimeout(parentCtx, writerCommandTimeout) + defer cancel() + + agentID, err := c.resolveAgentUUID(ctx) + if err != nil { + slog.Debug("feishu.writer_cmd.agent_resolve_failed", "error", err) + c.sendCommandReply(ctx, mc, "File writer management is not available (no agent).") + return + } + + groupID := fmt.Sprintf("group:%s:%s", c.Name(), mc.ChatID) + senderID := mc.SenderID // Feishu open_id has no suffix (Telegram's "|username" strip not needed) + + existingWriters, _ := c.configPermStore.ListFileWriters(ctx, agentID, groupID) + + // Authorization gate: only existing writers can manage the allowlist. + // Empty-writer groups allow the very first caller to seed the list — + // but an explicit target is still required, so we don't silently grant + // the caller when they type /addwriter with no reply/@mention. + if len(existingWriters) > 0 { + isWriter := false + for _, w := range existingWriters { + if w.UserID == senderID { + isWriter = true + break + } + } + if !isWriter { + c.sendCommandReply(ctx, mc, "Only existing file writers can manage the writer list.") + return + } + } else if action == "remove" { + c.sendCommandReply(ctx, mc, "No file writers configured yet. Use /addwriter to add the first one.") + return + } + + targetID, targetName := c.resolveWriterTarget(ctx, mc) + if targetID == "" { + verb := "add" + if action == "remove" { + verb = "remove" + } + c.sendCommandReply(ctx, mc, fmt.Sprintf("To %s a writer: reply to their message with /%swriter, or @mention them (including yourself to self-grant).", verb, verb)) + return + } + if targetName == "" { + targetName = targetID + } + + switch action { + case "add": + meta, _ := json.Marshal(map[string]string{"displayName": targetName}) + if err := c.configPermStore.Grant(ctx, &store.ConfigPermission{ + AgentID: agentID, + Scope: groupID, + ConfigType: store.ConfigTypeFileWriter, + UserID: targetID, + Permission: "allow", + Metadata: meta, + }); err != nil { + slog.Warn("feishu.writer_cmd.add_failed", "error", err, "target", targetID) + c.sendCommandReply(ctx, mc, "Failed to add writer. Please try again.") + return + } + c.sendCommandReply(ctx, mc, fmt.Sprintf("Added %s as a file writer.", targetName)) + + case "remove": + if len(existingWriters) <= 1 { + c.sendCommandReply(ctx, mc, "Cannot remove the last file writer.") + return + } + if err := c.configPermStore.Revoke(ctx, agentID, groupID, store.ConfigTypeFileWriter, targetID); err != nil { + slog.Warn("feishu.writer_cmd.remove_failed", "error", err, "target", targetID) + c.sendCommandReply(ctx, mc, "Failed to remove writer. Please try again.") + return + } + c.sendCommandReply(ctx, mc, fmt.Sprintf("Removed %s from file writers.", targetName)) + } +} + +// handleFeishuListWriters implements /writers. +func (c *Channel) handleFeishuListWriters(parentCtx context.Context, mc *messageContext) { + if mc.ChatType != "group" { + c.sendCommandReply(parentCtx, mc, "This command only works in group chats.") + return + } + if c.configPermStore == nil { + c.sendCommandReply(parentCtx, mc, "File writer management is not available.") + return + } + ctx, cancel := context.WithTimeout(parentCtx, writerCommandTimeout) + defer cancel() + agentID, err := c.resolveAgentUUID(ctx) + if err != nil { + slog.Debug("feishu.writer_cmd.agent_resolve_failed", "error", err) + c.sendCommandReply(ctx, mc, "File writer management is not available (no agent).") + return + } + + groupID := fmt.Sprintf("group:%s:%s", c.Name(), mc.ChatID) + writers, err := c.configPermStore.List(ctx, agentID, store.ConfigTypeFileWriter, groupID) + if err != nil { + slog.Warn("feishu.writer_cmd.list_failed", "error", err) + c.sendCommandReply(ctx, mc, "Failed to list writers. Please try again.") + return + } + if len(writers) == 0 { + c.sendCommandReply(ctx, mc, "No file writers configured for this group. Use /addwriter to add one.") + return + } + + type fwMeta struct { + DisplayName string `json:"displayName"` + } + var sb strings.Builder + fmt.Fprintf(&sb, "File writers for this group (%d):\n", len(writers)) + for i, w := range writers { + var meta fwMeta + _ = json.Unmarshal(w.Metadata, &meta) + label := w.UserID + if meta.DisplayName != "" { + label = meta.DisplayName + } + fmt.Fprintf(&sb, "%d. %s (ID: %s)\n", i+1, label, w.UserID) + } + c.sendCommandReply(ctx, mc, sb.String()) +} diff --git a/internal/channels/feishu/commands_writers_test.go b/internal/channels/feishu/commands_writers_test.go new file mode 100644 index 00000000..58c91c0e --- /dev/null +++ b/internal/channels/feishu/commands_writers_test.go @@ -0,0 +1,446 @@ +package feishu + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/channels" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// fakeConfigPermStore is a minimal in-memory ConfigPermissionStore that +// implements only the methods the writer commands actually call. Other +// methods are no-ops or return nil so the interface is satisfied without +// a full fake implementation. +type fakeConfigPermStore struct { + mu sync.Mutex + perms []store.ConfigPermission +} + +func (f *fakeConfigPermStore) CheckPermission(_ context.Context, _ uuid.UUID, _, _, _ string) (bool, error) { + return false, nil +} + +func (f *fakeConfigPermStore) Grant(_ context.Context, perm *store.ConfigPermission) error { + f.mu.Lock() + defer f.mu.Unlock() + // Replace existing row for idempotency (matches real store's upsert). + for i, p := range f.perms { + if p.AgentID == perm.AgentID && p.Scope == perm.Scope && p.ConfigType == perm.ConfigType && p.UserID == perm.UserID { + f.perms[i] = *perm + return nil + } + } + f.perms = append(f.perms, *perm) + return nil +} + +func (f *fakeConfigPermStore) Revoke(_ context.Context, agentID uuid.UUID, scope, configType, userID string) error { + f.mu.Lock() + defer f.mu.Unlock() + kept := f.perms[:0] + for _, p := range f.perms { + if p.AgentID == agentID && p.Scope == scope && p.ConfigType == configType && p.UserID == userID { + continue + } + kept = append(kept, p) + } + f.perms = kept + return nil +} + +func (f *fakeConfigPermStore) List(_ context.Context, agentID uuid.UUID, configType, scope string) ([]store.ConfigPermission, error) { + f.mu.Lock() + defer f.mu.Unlock() + var out []store.ConfigPermission + for _, p := range f.perms { + if p.AgentID == agentID && p.ConfigType == configType && (scope == "" || p.Scope == scope) { + out = append(out, p) + } + } + return out, nil +} + +func (f *fakeConfigPermStore) ListFileWriters(ctx context.Context, agentID uuid.UUID, scope string) ([]store.ConfigPermission, error) { + return f.List(ctx, agentID, store.ConfigTypeFileWriter, scope) +} + +// newTestChannel builds a minimal Feishu Channel suitable for writer-command +// unit tests: BaseChannel with a UUID agent key, stub lark client pointing +// at the provided httptest server, and the supplied permission store. +// botOpenID is pre-set to a fake value so writer commands pass the +// "bot identity resolved" guard (real production probes Lark for this). +func newTestChannel(t *testing.T, srvURL string, permStore store.ConfigPermissionStore, agentUUID uuid.UUID) *Channel { + t.Helper() + base := channels.NewBaseChannel(channels.TypeFeishu, nil, nil) + base.SetAgentID(agentUUID.String()) + return &Channel{ + BaseChannel: base, + client: NewLarkClient("app", "secret", srvURL), + configPermStore: permStore, + botOpenID: "ou_fake_bot", + } +} + +// captureReplies returns an httptest server that records every POST message +// sent by the channel so the test can assert the reply content. +func captureReplies(t *testing.T) (*httptest.Server, *[]string) { + t.Helper() + var mu sync.Mutex + replies := make([]string, 0, 4) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == tokenEndpoint { + _, _ = w.Write([]byte(`{"code":0,"msg":"ok","tenant_access_token":"tok","expire":7200}`)) + return + } + if strings.HasSuffix(r.URL.Path, "/messages") || strings.HasSuffix(r.URL.Path, "/reply") { + raw, _ := io.ReadAll(r.Body) + var body map[string]any + _ = json.Unmarshal(raw, &body) + // The Feishu "post" content wraps the reply text — we just record + // the raw content payload so the test can grep it. + if content, ok := body["content"].(string); ok { + mu.Lock() + replies = append(replies, content) + mu.Unlock() + } + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"code":0,"msg":"","data":{"message_id":"om_reply"}}`)) + })) + t.Cleanup(srv.Close) + return srv, &replies +} + +// assertReplyContains checks that at least one captured reply contains the +// expected substring. Useful because the "post" content is double-encoded +// JSON with sender name wrappers. +func assertReplyContains(t *testing.T, replies []string, want string) { + t.Helper() + for _, r := range replies { + if strings.Contains(r, want) { + return + } + } + t.Errorf("no reply contained %q; captured replies: %v", want, replies) +} + +// TestWriterCmd_RejectsInDM verifies DMs are rejected with a clear message. +func TestWriterCmd_RejectsInDM(t *testing.T) { + srv, replies := captureReplies(t) + ch := newTestChannel(t, srv.URL, &fakeConfigPermStore{}, uuid.New()) + + mc := &messageContext{ + ChatID: "oc_dm_1", + MessageID: "om_cmd", + SenderID: "ou_alice", + ChatType: "p2p", + Content: "/addwriter", + } + handled := ch.maybeHandleWriterCommand(context.Background(), mc) + if !handled { + t.Fatal("expected command to be handled (consumed)") + } + assertReplyContains(t, *replies, "only works in group chats") +} + +// TestWriterCmd_NotAvailableWhenNoPermStore verifies graceful degradation. +func TestWriterCmd_NotAvailableWhenNoPermStore(t *testing.T) { + srv, replies := captureReplies(t) + ch := newTestChannel(t, srv.URL, nil, uuid.New()) + + mc := &messageContext{ + ChatID: "oc_grp_1", MessageID: "om_cmd", SenderID: "ou_alice", + ChatType: "group", Content: "/writers", + } + handled := ch.maybeHandleWriterCommand(context.Background(), mc) + if !handled { + t.Fatal("expected command to be handled") + } + assertReplyContains(t, *replies, "not available") +} + +// TestWriterCmd_BootstrapSelfGrantViaSelfMention verifies the first caller +// in an empty-writer group CAN bootstrap the allowlist, but ONLY when they +// explicitly @mention themselves — no accidental self-grant from a bare +// /addwriter typo. +func TestWriterCmd_BootstrapSelfGrantViaSelfMention(t *testing.T) { + srv, replies := captureReplies(t) + perm := &fakeConfigPermStore{} + agentID := uuid.New() + ch := newTestChannel(t, srv.URL, perm, agentID) + + mc := &messageContext{ + ChatID: "oc_grp_1", MessageID: "om_cmd", SenderID: "ou_alice", + ChatType: "group", Content: "/addwriter @_user_1", + Mentions: []mentionInfo{{Key: "@_user_1", OpenID: "ou_alice", Name: "Alice"}}, + } + handled := ch.maybeHandleWriterCommand(context.Background(), mc) + if !handled { + t.Fatal("expected command to be handled") + } + assertReplyContains(t, *replies, "Added") + + got, _ := perm.ListFileWriters(context.Background(), agentID, "group:feishu:oc_grp_1") + if len(got) != 1 || got[0].UserID != "ou_alice" { + t.Errorf("expected 1 writer (ou_alice), got %+v", got) + } +} + +// TestWriterCmd_BareAddWriterShowsUsageHint guards M1: a bare /addwriter +// with no target must NOT silently self-grant. Users typing the command +// exploratorily should see instructions, not capture first-writer. +func TestWriterCmd_BareAddWriterShowsUsageHint(t *testing.T) { + srv, replies := captureReplies(t) + perm := &fakeConfigPermStore{} + agentID := uuid.New() + ch := newTestChannel(t, srv.URL, perm, agentID) + + mc := &messageContext{ + ChatID: "oc_grp_1", MessageID: "om_cmd", SenderID: "ou_alice", + ChatType: "group", Content: "/addwriter", // no mention, no reply-to + } + handled := ch.maybeHandleWriterCommand(context.Background(), mc) + if !handled { + t.Fatal("expected command to be handled") + } + assertReplyContains(t, *replies, "reply to their message") + + // Critical: no grant must be created. + got, _ := perm.ListFileWriters(context.Background(), agentID, "group:feishu:oc_grp_1") + if len(got) != 0 { + t.Errorf("expected 0 writers (no accidental self-grant), got %+v", got) + } +} + +// TestWriterCmd_BotProbeIncomplete guards M2: when botOpenID is empty +// (probe has not resolved yet), writer commands refuse with a retry hint +// rather than risk granting the bot itself as a writer via @mention. +func TestWriterCmd_BotProbeIncomplete(t *testing.T) { + srv, replies := captureReplies(t) + ch := newTestChannel(t, srv.URL, &fakeConfigPermStore{}, uuid.New()) + ch.botOpenID = "" // simulate probe-not-yet-complete + + mc := &messageContext{ + ChatID: "oc_grp_1", MessageID: "om_cmd", SenderID: "ou_alice", + ChatType: "group", Content: "/addwriter @_user_1", + Mentions: []mentionInfo{{Key: "@_user_1", OpenID: "ou_alice", Name: "Alice"}}, + } + handled := ch.maybeHandleWriterCommand(context.Background(), mc) + if !handled { + t.Fatal("expected command to be handled") + } + assertReplyContains(t, *replies, "Bot identity not yet resolved") +} + +// TestWriterCmd_ReplyToTargetResolution guards M4: the reply-to code path +// must correctly fetch the parent message sender and use it as the target. +func TestWriterCmd_ReplyToTargetResolution(t *testing.T) { + // Custom server: satisfies token + /im/v1/messages/{id} GET (parent lookup) + // + any outbound message POST (command reply). + var replies []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == tokenEndpoint { + _, _ = w.Write([]byte(`{"code":0,"msg":"ok","tenant_access_token":"tok","expire":7200}`)) + return + } + if r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/open-apis/im/v1/messages/") { + // Parent message lookup — return Bob as the sender. + _, _ = w.Write([]byte(`{"code":0,"msg":"","data":{"items":[{"message_id":"om_parent","msg_type":"text","body":{"content":"{\"text\":\"hi\"}"},"sender":{"id":"ou_bob","id_type":"open_id","sender_type":"user"}}]}}`)) + return + } + // Outbound command reply. + raw, _ := io.ReadAll(r.Body) + var body map[string]any + _ = json.Unmarshal(raw, &body) + if content, ok := body["content"].(string); ok { + replies = append(replies, content) + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"code":0,"msg":"","data":{"message_id":"om_reply"}}`)) + })) + defer srv.Close() + + perm := &fakeConfigPermStore{} + agentID := uuid.New() + _ = perm.Grant(context.Background(), &store.ConfigPermission{ + AgentID: agentID, Scope: "group:feishu:oc_grp_1", + ConfigType: store.ConfigTypeFileWriter, UserID: "ou_alice", Permission: "allow", + }) + ch := newTestChannel(t, srv.URL, perm, agentID) + + mc := &messageContext{ + ChatID: "oc_grp_1", MessageID: "om_cmd", SenderID: "ou_alice", + ChatType: "group", Content: "/addwriter", + ParentID: "om_parent", // reply-to Bob's message + } + handled := ch.maybeHandleWriterCommand(context.Background(), mc) + if !handled { + t.Fatal("expected command to be handled") + } + assertReplyContains(t, replies, "Added") + + got, _ := perm.ListFileWriters(context.Background(), agentID, "group:feishu:oc_grp_1") + if len(got) != 2 { + t.Errorf("expected 2 writers after reply-to grant, got %d: %+v", len(got), got) + } + foundBob := false + for _, w := range got { + if w.UserID == "ou_bob" { + foundBob = true + break + } + } + if !foundBob { + t.Errorf("expected bob to be granted via reply-to, got writers: %+v", got) + } +} + +// TestWriterCmd_NonWriterCannotGrant verifies authorization: when writers +// exist, a non-writer caller must be rejected. +func TestWriterCmd_NonWriterCannotGrant(t *testing.T) { + srv, replies := captureReplies(t) + perm := &fakeConfigPermStore{} + agentID := uuid.New() + // Seed: alice is already a writer. + _ = perm.Grant(context.Background(), &store.ConfigPermission{ + AgentID: agentID, Scope: "group:feishu:oc_grp_1", + ConfigType: store.ConfigTypeFileWriter, UserID: "ou_alice", Permission: "allow", + }) + ch := newTestChannel(t, srv.URL, perm, agentID) + + mc := &messageContext{ + ChatID: "oc_grp_1", MessageID: "om_cmd", SenderID: "ou_bob", + ChatType: "group", Content: "/addwriter", + Mentions: []mentionInfo{{Key: "@_user_1", OpenID: "ou_carol", Name: "Carol"}}, + } + handled := ch.maybeHandleWriterCommand(context.Background(), mc) + if !handled { + t.Fatal("expected command to be handled") + } + assertReplyContains(t, *replies, "Only existing file writers") + + // Verify no new grant was created (still just alice). + got, _ := perm.ListFileWriters(context.Background(), agentID, "group:feishu:oc_grp_1") + if len(got) != 1 { + t.Errorf("expected writers unchanged (1), got %d", len(got)) + } +} + +// TestWriterCmd_WriterGrantsViaMention verifies an existing writer can add +// a new user via @mention, and the grant row reflects the new target. +func TestWriterCmd_WriterGrantsViaMention(t *testing.T) { + srv, replies := captureReplies(t) + perm := &fakeConfigPermStore{} + agentID := uuid.New() + _ = perm.Grant(context.Background(), &store.ConfigPermission{ + AgentID: agentID, Scope: "group:feishu:oc_grp_1", + ConfigType: store.ConfigTypeFileWriter, UserID: "ou_alice", Permission: "allow", + }) + ch := newTestChannel(t, srv.URL, perm, agentID) + + mc := &messageContext{ + ChatID: "oc_grp_1", MessageID: "om_cmd", SenderID: "ou_alice", + ChatType: "group", Content: "/addwriter @_user_1", + Mentions: []mentionInfo{{Key: "@_user_1", OpenID: "ou_bob", Name: "Bob"}}, + } + handled := ch.maybeHandleWriterCommand(context.Background(), mc) + if !handled { + t.Fatal("expected command to be handled") + } + assertReplyContains(t, *replies, "Added") + + got, _ := perm.ListFileWriters(context.Background(), agentID, "group:feishu:oc_grp_1") + if len(got) != 2 { + t.Errorf("expected 2 writers after grant, got %d: %+v", len(got), got) + } +} + +// TestWriterCmd_RemoveLastWriterRejected verifies the last-writer guard. +func TestWriterCmd_RemoveLastWriterRejected(t *testing.T) { + srv, replies := captureReplies(t) + perm := &fakeConfigPermStore{} + agentID := uuid.New() + _ = perm.Grant(context.Background(), &store.ConfigPermission{ + AgentID: agentID, Scope: "group:feishu:oc_grp_1", + ConfigType: store.ConfigTypeFileWriter, UserID: "ou_alice", Permission: "allow", + }) + ch := newTestChannel(t, srv.URL, perm, agentID) + + mc := &messageContext{ + ChatID: "oc_grp_1", MessageID: "om_cmd", SenderID: "ou_alice", + ChatType: "group", Content: "/removewriter", + Mentions: []mentionInfo{{Key: "@_user_1", OpenID: "ou_alice", Name: "Alice"}}, + } + handled := ch.maybeHandleWriterCommand(context.Background(), mc) + if !handled { + t.Fatal("expected command to be handled") + } + assertReplyContains(t, *replies, "Cannot remove the last") + + got, _ := perm.ListFileWriters(context.Background(), agentID, "group:feishu:oc_grp_1") + if len(got) != 1 { + t.Errorf("expected writers unchanged, got %d", len(got)) + } +} + +// TestWriterCmd_ListEmpty verifies /writers on a group with no writers +// returns the instructional "no writers configured" message. +func TestWriterCmd_ListEmpty(t *testing.T) { + srv, replies := captureReplies(t) + ch := newTestChannel(t, srv.URL, &fakeConfigPermStore{}, uuid.New()) + + mc := &messageContext{ + ChatID: "oc_grp_1", MessageID: "om_cmd", SenderID: "ou_alice", + ChatType: "group", Content: "/writers", + } + handled := ch.maybeHandleWriterCommand(context.Background(), mc) + if !handled { + t.Fatal("expected command to be handled") + } + assertReplyContains(t, *replies, "No file writers configured") +} + +// TestWriterCmd_ListPopulated verifies /writers enumerates existing writers. +func TestWriterCmd_ListPopulated(t *testing.T) { + srv, replies := captureReplies(t) + perm := &fakeConfigPermStore{} + agentID := uuid.New() + meta, _ := json.Marshal(map[string]string{"displayName": "Alice"}) + _ = perm.Grant(context.Background(), &store.ConfigPermission{ + AgentID: agentID, Scope: "group:feishu:oc_grp_1", + ConfigType: store.ConfigTypeFileWriter, UserID: "ou_alice", Permission: "allow", + Metadata: meta, + }) + ch := newTestChannel(t, srv.URL, perm, agentID) + + mc := &messageContext{ + ChatID: "oc_grp_1", MessageID: "om_cmd", SenderID: "ou_alice", + ChatType: "group", Content: "/writers", + } + ch.maybeHandleWriterCommand(context.Background(), mc) + assertReplyContains(t, *replies, "File writers for this group") + assertReplyContains(t, *replies, "Alice") +} + +// TestWriterCmd_NonCommandNotConsumed verifies plain text passes through. +func TestWriterCmd_NonCommandNotConsumed(t *testing.T) { + srv, _ := captureReplies(t) + ch := newTestChannel(t, srv.URL, &fakeConfigPermStore{}, uuid.New()) + + mc := &messageContext{ + ChatID: "oc_grp_1", MessageID: "om_cmd", SenderID: "ou_alice", + ChatType: "group", Content: "hello bot", + } + if ch.maybeHandleWriterCommand(context.Background(), mc) { + t.Errorf("plain text must not be consumed as command") + } +} diff --git a/internal/channels/feishu/feishu.go b/internal/channels/feishu/feishu.go index 9e990787..68bc1d86 100644 --- a/internal/channels/feishu/feishu.go +++ b/internal/channels/feishu/feishu.go @@ -35,21 +35,39 @@ const ( // Channel connects to Feishu/Lark via native HTTP + WebSocket. type Channel struct { *channels.BaseChannel - cfg config.FeishuConfig - client *LarkClient - botOpenID string - senderCache sync.Map // open_id → *senderCacheEntry - dedup sync.Map // message_id → struct{} - reactions sync.Map // chatID → *reactionState - docCache *docCache // LRU+TTL cache for Lark docx raw_content lookups - groupAllowList []string // Feishu-specific: per-group sender allowlist (separate from BaseChannel allowList) - stopCh chan struct{} - httpServer *http.Server - wsClient *WSClient + cfg config.FeishuConfig + client *LarkClient + botOpenID string + senderCache sync.Map // open_id → *senderCacheEntry + dedup sync.Map // message_id → struct{} + reactions sync.Map // chatID → *reactionState + docCache *docCache // LRU+TTL cache for Lark docx raw_content lookups + agentStore store.AgentStore // optional — agent key → UUID lookup for writer commands + configPermStore store.ConfigPermissionStore // optional — group file writer ACL for /addwriter et al. + groupAllowList []string // Feishu-specific: per-group sender allowlist (separate from BaseChannel allowList) + stopCh chan struct{} + httpServer *http.Server + wsClient *WSClient // pairingService, pairingDebounce, approvedGroups, groupHistory, historyLimit // are inherited from channels.BaseChannel. } +// Option configures optional Feishu channel dependencies, mirroring the +// Telegram channel's pattern so the gateway wiring code can add stores +// post-construction without breaking the New() signature. +type Option func(*Channel) + +// WithAgentStore enables agent key → UUID resolution, required for writer +// management commands (/addwriter, /writers, /removewriter). +func WithAgentStore(s store.AgentStore) Option { return func(c *Channel) { c.agentStore = s } } + +// WithConfigPermStore enables the group file writer ACL used by writer +// management commands. When nil, the commands fail with a clear "not +// available" message instead of crashing. +func WithConfigPermStore(s store.ConfigPermissionStore) Option { + return func(c *Channel) { c.configPermStore = s } +} + // Lark docs auto-fetch tunables. Kept as consts rather than config fields // because YAGNI — operators can ask for knobs later if real usage needs them. const ( @@ -72,7 +90,7 @@ type senderCacheEntry struct { } // New creates a new Feishu/Lark channel. -func New(cfg config.FeishuConfig, msgBus *bus.MessageBus, pairingSvc store.PairingStore, pendingStore store.PendingMessageStore) (*Channel, error) { +func New(cfg config.FeishuConfig, msgBus *bus.MessageBus, pairingSvc store.PairingStore, pendingStore store.PendingMessageStore, opts ...Option) (*Channel, error) { if cfg.AppID == "" || cfg.AppSecret == "" { return nil, fmt.Errorf("feishu app_id and app_secret are required") } @@ -101,6 +119,9 @@ func New(cfg config.FeishuConfig, msgBus *bus.MessageBus, pairingSvc store.Pairi ch.SetPairingService(pairingSvc) ch.SetGroupHistory(channels.MakeHistory(channels.TypeFeishu, pendingStore, base.TenantID())) ch.SetHistoryLimit(historyLimit) + for _, opt := range opts { + opt(ch) + } return ch, nil }