fix: harden feishu plugin error handling - #58
Conversation
|
/gemini review |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUnifies Feishu message handling to a per-sessionKey FIFO queue, moves post-idle prompting to an event-driven nudge, tightens logging/error handling, threads LogFn broadly, adds session-poison detection/invalidation, and updates extraction, download, sender, and session mapping behaviors and docs. Changes
Sequence DiagramsequenceDiagram
participant Gateway as Feishu Gateway
participant Queue as Session Queue
participant Chat as Chat Handler
participant ActionBus as Action Bus
participant Event as Event Handler
participant Nudge as Nudge (idle)
Gateway->>Queue: enqueueMessage(ctx, deps)
alt ctx.shouldReply === false
Queue->>Chat: handleChat(ctx, deps, signal?)
else reply required
Queue->>Queue: push into FIFO per sessionKey
Queue->>Queue: start drainLoop if not running
Queue->>Chat: handleQueuedMessage (drain iteration)
end
Chat->>ActionBus: emit(sessionId, action, log?)
ActionBus->>Event: notify subscribers
Event->>Event: handleV2Event / handleMessagePartUpdated
alt session.idle detected
Event->>Nudge: check nudge config & iterations
Nudge->>Chat: handleChat(nudge synthetic payload, deps, signal?)
Chat->>ActionBus: emit(sessionId, { type: "session-idle" }, log?)
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the robustness, observability, and error handling of the Feishu plugin. Key changes include migrating from warn to error logs for critical failures, implementing a session poisoning detection mechanism to automatically reset corrupted sessions, and refining the streaming card lifecycle management. I have reviewed the changes and suggest upgrading the log level for session poisoning detection to error to improve visibility into this critical failure mode.
| log("warn", "检测到 session 历史数据中毒，创建新 session", { | ||
| sessionKey, oldSessionId: session.id, error: sessionError.message, | ||
| }) |
There was a problem hiding this comment.
为了与本次 PR 的目标“加强错误处理和日志记录”保持一致，建议将此处的日志级别从 warn 提升到 error。会话历史中毒并因此强制创建新会话是一个值得关注的异常情况，使用 error 级别能更好地在日志系统中突出显示此类事件，便于后续追踪和分析。
| log("warn", "检测到 session 历史数据中毒，创建新 session", { | |
| sessionKey, oldSessionId: session.id, error: sessionError.message, | |
| }) | |
| log("error", "检测到 session 历史数据中毒，创建新 session", { | |
| sessionKey, oldSessionId: session.id, error: sessionError.message, | |
| }) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/feishu/content-extractor.ts (1)
29-73:⚠️ Potential issue | 🟠 MajorNormalize empty extractor results before returning.
This function documents that it returns at least one
PromptPart, but some branches now return[](extractText()for blank text, and the newextractPost()parse-failure path). That can silently drop the Feishu message entirely and may violate downstream request validation that expects a non-emptypartsarray.Proposed fix
export async function extractParts( feishuClient: InstanceType<typeof Lark.Client>, messageId: string, messageType: string, rawContent: string, log: LogFn, maxResourceSize: number, ): Promise<PromptPart[]> { try { + let parts: PromptPart[] + // 先按消息类型分发到专用提取函数，再由各函数处理自己的细节。 switch (messageType) { case "text": - return extractText(rawContent) + parts = extractText(rawContent) + break case "image": - return await extractImage(feishuClient, messageId, rawContent, log, maxResourceSize) + parts = await extractImage(feishuClient, messageId, rawContent, log, maxResourceSize) + break case "post": - return await extractPost(feishuClient, messageId, rawContent, log, maxResourceSize) + parts = await extractPost(feishuClient, messageId, rawContent, log, maxResourceSize) + break case "file": - return await extractFile(feishuClient, messageId, rawContent, log, maxResourceSize) + parts = await extractFile(feishuClient, messageId, rawContent, log, maxResourceSize) + break case "audio": - return await extractAudio(feishuClient, messageId, rawContent, log, maxResourceSize) + parts = await extractAudio(feishuClient, messageId, rawContent, log, maxResourceSize) + break case "media": - return extractMediaFallback() + parts = extractMediaFallback() + break case "sticker": - return [{ type: "text", text: "[表情包]" }] + parts = [{ type: "text", text: "[表情包]" }] + break case "interactive": - return extractInteractive(rawContent, log) + parts = extractInteractive(rawContent, log) + break case "share_chat": - return extractShareChat(rawContent, log) + parts = extractShareChat(rawContent, log) + break case "share_user": - return [{ type: "text", text: "[分享了一个用户名片]" }] + parts = [{ type: "text", text: "[分享了一个用户名片]" }] + break case "merge_forward": - return [{ type: "text", text: "[合并转发消息]" }] + parts = [{ type: "text", text: "[合并转发消息]" }] + break default: - return [{ type: "text", text: `[不支持的消息类型: ${messageType}]` }] + parts = [{ type: "text", text: `[不支持的消息类型: ${messageType}]` }] } + + return parts.length + ? parts + : [{ type: "text", text: `[消息内容为空或解析失败: ${messageType}]` }] } catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/feishu/content-extractor.ts` around lines 29 - 73, The extractParts function can return an empty PromptPart[] (e.g., extractText() for blank text or extractPost() on parse failure), which breaks callers expecting at least one part; update extractParts to normalize any empty array result into a single fallback text part like [{ type: "text", text: `[消息内容为空: ${messageType}]` }]; specifically, after each awaited branch (or centrally before returning from extractParts) check if the value is an array with length === 0 and replace it with the fallback, referencing extractParts, extractText, and extractPost to locate the affected branches and ensure downstream validation always receives a non-empty parts array.src/handler/session-queue.ts (2)
98-117:⚠️ Potential issue | 🟠 MajorThis drops the Phase-2 idle auto-prompt flow for group chats.
handleChat()is supposed to hand queue state back so the drain loop can keep running idle auto-prompt iterations once user messages are drained.drainLoop()ignores the return value and exits as soon asstate.queuebecomes empty, so the documented group idle phase never runs.Based on learnings,
src/handler/session-queue.tsshould transition from Phase 1 user-message processing to the idle auto-prompt phase when the queue becomes empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handler/session-queue.ts` around lines 98 - 117, drainLoop currently stops as soon as state.queue is empty, which drops the Phase-2 idle auto-prompt flow; change drainLoop to observe the return value of handleChat (e.g., a boolean or a sentinel on QueueState) and keep the loop running for idle iterations when handleChat signals "continue idle". Specifically, in drainLoop (which currently shifts items from state.queue and calls handleChat(item.ctx, item.deps)), capture handleChat's return (or read a flag on QueueState that handleChat sets) and use that to extend the loop condition from merely state.queue.length > 0 to while (state.queue.length > 0 || shouldContinueIdle) so cleanupStateIfIdle and state.processing=false only run after idle phase finishes; update handleChat (or QueueState) to return or expose the continue-idle signal accordingly.
57-90:⚠️ Potential issue | 🔴 CriticalDon't block the gateway on the full drain loop.
enqueueMessage()sits under the gateway'sawait onMessage(ctx)path. AwaitingdrainLoop()here means the websocket handler stays blocked until the current reply fully finishes, so later messages cannot enqueue promptly, cannot interrupt a P2P run, and end up behind a head-of-line stall.Based on learnings,
src/feishu/gateway.tsawaitsonMessage(ctx)andsrc/handler/session-queue.tsshould keep AbortController-based interruption for P2P instead of a blocking unified FIFO.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handler/session-queue.ts` around lines 57 - 90, enqueueMessage currently blocks the gateway because handleQueuedMessage awaits drainLoop; change handleQueuedMessage so after state.queue.push it does not await drainLoop but instead sets state.processing = true and starts drainLoop asynchronously (e.g., void drainLoop(sessionKey, state)) to avoid blocking onMessage; also ensure drainLoop itself clears state.processing when finished and that the session state includes an AbortController (created/stored in getOrCreateState) so P2P runs can be interrupted via that controller rather than relying on a blocking FIFO.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/codex-code-review.yml:
- Around line 10-13: In the codex job's permissions block remove the unused
"pull-requests: write" entry to follow least-privilege; keep "contents: read"
and any existing "outputs" unchanged. Ensure the "post_feedback" job continues
to use "issues: write" for PR commenting (do not move permissions there), and
run the workflow to verify no steps in the codex job require pull-requests write
access.
- Around line 8-35: Add a guard to skip the codex job when the OPENAI_API_KEY
secret is not present: add an if condition on the codex job (the job with
id/name "codex") such as if: ${{ secrets.OPENAI_API_KEY }} so the job is not run
for forked PRs lacking the secret, ensuring the step that runs the action (step
id "run_codex" which uses openai/codex-action@v1 and references the
OPENAI_API_KEY secret) is never executed when the secret is absent.
In `@src/feishu/gateway.ts`:
- Around line 180-190: Don't trust parsedAction.chatId for routing: when
building syntheticCtx in the send_message branch (the code around
parseCardActionValue and syntheticCtx / FeishuMessageContext), set
syntheticCtx.chatId from the callback event's authoritative chat id (use
action.chatId / context open_chat_id) rather than parsedAction.chatId; if
parsedAction.chatId exists but differs, emit a warning log mentioning both IDs
and continue routing using action.chatId. Also keep using parsedAction.text for
content and parsedAction.chatType only if it matches or is absent, but never let
parsedAction.chatId override the event's chat id.
In `@src/feishu/markdown.ts`:
- Around line 49-62: The code currently strips HTML tags before ensuring code
fences are closed, which can corrupt trailing/unfinished code blocks; fix
cleanMarkdown by calling closeCodeBlocks on the raw text before calling
extractCodeBlocks and before applying HTML_TAG_RE to segments (or alternatively
call closeCodeBlocks immediately after replacing <br> and before
extractCodeBlocks), then proceed to extractCodeBlocks(…), strip HTML from
non-code segments using HTML_TAG_RE, re-insert codeBlocks, and finally return
the result; reference functions/idents: cleanMarkdown, closeCodeBlocks,
extractCodeBlocks, HTML_TAG_RE, segments, codeBlocks.
In `@src/handler/action-bus.ts`:
- Around line 32-33: The module removed the per-session bulk cleanup API; add
back and export an unsubscribeAll(sessionId: string) function that locates the
subscribers Map entry for the given sessionId, iterates/clears the
Set<ActionCallback> (or simply delete the map entry) and releases resources so
the session's callbacks are removed; ensure unsubscribeAll is consistent with
subscribe(sessionId, cb) and emit(sessionId, action) semantics and is exported
from the module so callers can force per-session cleanup.
In `@src/handler/chat.ts`:
- Around line 229-247: The timer callback can race with the main flow: if
sendTextMessage(...) completes after the main flow set done = true, the code
currently returns and leaves the sent "正在思考…" message orphaned because
placeholderId/registerPending are never set; fix by, after awaiting
sender.sendTextMessage(feishuClient, chatId, ...), capture res.messageId and if
done is true then proactively delete that message (e.g., call the
sender.deleteMessage or the appropriate message deletion API with feishuClient,
chatId and res.messageId and log) and return; otherwise continue with the
existing flow that sets placeholderId and calls registerPending(activeSessionId,
{ placeholderId, feishuClient }).
In `@src/handler/interactive.ts`:
- Around line 144-162: The code currently calls markSeen(requestId) before the
async sendInteractiveCard, which causes failed sends to be permanently treated
as seen; change the flow so requestId is only added to seenRequestIds (i.e.,
call markSeen(requestId)) after a successful send (when
sender.sendInteractiveCard(...) returns res.ok === true), and ensure that on
res.ok === false or in the catch block you do not mark the id (or you rollback
by removing it from seenRequestIds/unmarkSeen); update the success branch to
call markSeen and keep the error logging in the failure branches (referencing
requestId, sender.sendInteractiveCard, markSeen, seenRequestIds, deps.log) so
retries/SSE replays can retry failed sends.
---
Outside diff comments:
In `@src/feishu/content-extractor.ts`:
- Around line 29-73: The extractParts function can return an empty PromptPart[]
(e.g., extractText() for blank text or extractPost() on parse failure), which
breaks callers expecting at least one part; update extractParts to normalize any
empty array result into a single fallback text part like [{ type: "text", text:
`[消息内容为空: ${messageType}]` }]; specifically, after each awaited branch (or
centrally before returning from extractParts) check if the value is an array
with length === 0 and replace it with the fallback, referencing extractParts,
extractText, and extractPost to locate the affected branches and ensure
downstream validation always receives a non-empty parts array.
In `@src/handler/session-queue.ts`:
- Around line 98-117: drainLoop currently stops as soon as state.queue is empty,
which drops the Phase-2 idle auto-prompt flow; change drainLoop to observe the
return value of handleChat (e.g., a boolean or a sentinel on QueueState) and
keep the loop running for idle iterations when handleChat signals "continue
idle". Specifically, in drainLoop (which currently shifts items from state.queue
and calls handleChat(item.ctx, item.deps)), capture handleChat's return (or read
a flag on QueueState that handleChat sets) and use that to extend the loop
condition from merely state.queue.length > 0 to while (state.queue.length > 0 ||
shouldContinueIdle) so cleanupStateIfIdle and state.processing=false only run
after idle phase finishes; update handleChat (or QueueState) to return or expose
the continue-idle signal accordingly.
- Around line 57-90: enqueueMessage currently blocks the gateway because
handleQueuedMessage awaits drainLoop; change handleQueuedMessage so after
state.queue.push it does not await drainLoop but instead sets state.processing =
true and starts drainLoop asynchronously (e.g., void drainLoop(sessionKey,
state)) to avoid blocking onMessage; also ensure drainLoop itself clears
state.processing when finished and that the session state includes an
AbortController (created/stored in getOrCreateState) so P2P runs can be
interrupted via that controller rather than relying on a blocking FIFO.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a9e7fbf-3822-4865-853a-2aba809aff1e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
.github/codex/prompts/review.md.github/workflows/codex-code-review.ymlAGENTS.mdCLAUDE.mdpackage.jsonsrc/feishu/cardkit.tssrc/feishu/content-extractor.tssrc/feishu/dedup.tssrc/feishu/gateway.tssrc/feishu/group-filter.tssrc/feishu/history.tssrc/feishu/markdown.tssrc/feishu/quote.tssrc/feishu/resource.tssrc/feishu/sender.tssrc/feishu/session-chat-map.tssrc/feishu/streaming-card.tssrc/feishu/user-name.tssrc/handler/action-bus.tssrc/handler/chat.tssrc/handler/error-recovery.tssrc/handler/event.tssrc/handler/interactive.tssrc/handler/session-queue.tssrc/index.tssrc/session.tssrc/tools/send-card.tssrc/types.tssrc/utils/ttl-map.ts
| /** sessionId → 订阅回调集合。 */ | ||
| const subscribers = new Map<string, Set<ActionCallback>>() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add unsubscribeAll(sessionId) back to preserve cleanup contract.
当前模块缺少批量清理入口，不利于会话级强制回收；建议补回 unsubscribeAll 导出以维持 action-bus API 完整性。
♻️ Suggested patch
const subscribers = new Map<string, Set<ActionCallback>>()
+export function unsubscribeAll(sessionId: string): void {
+ subscribers.delete(sessionId)
+}
+
/**
* 向指定 session 的订阅者广播事件。
*
* 这里采用 fire-and-forget：As per coding guidelines src/handler/action-bus.ts: “Implement per-session event pub-sub: subscribe(sessionId, cb) returns unsubscribe function, emit(sessionId, action) fire-and-forget routing, unsubscribeAll(sessionId) cleanup.”
Also applies to: 40-66, 90-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/handler/action-bus.ts` around lines 32 - 33, The module removed the
per-session bulk cleanup API; add back and export an unsubscribeAll(sessionId:
string) function that locates the subscribers Map entry for the given sessionId,
iterates/clears the Set<ActionCallback> (or simply delete the map entry) and
releases resources so the session's callbacks are removed; ensure unsubscribeAll
is consistent with subscribe(sessionId, cb) and emit(sessionId, action)
semantics and is exported from the module so callers can force per-session
cleanup.
There was a problem hiding this comment.
Code Review
This pull request refactors and improves the Feishu plugin, focusing on better error handling, logging, and code documentation. Key changes include adding a more robust error handling mechanism for resource downloads, improving the StreamingCard lifecycle management, and enhancing the ActionBus with better error reporting. I have identified an issue in the file size calculation logic where the data URL prefix is incorrectly included in the size estimation, and provided a suggestion to fix it.
| const sizeMB = (result.resource.dataUrl.length * 0.75 / (1024 * 1024)).toFixed(1) | ||
| return [{ type: "text", text: `[文件: ${fileName}, ${sizeMB}MB]` }] |
There was a problem hiding this comment.
文件大小计算不准确。
当前计算文件大小的方式 result.resource.dataUrl.length * 0.75 会将 data URL 的前缀 data:<mime>;base64, 也计算在内，导致最终显示的文件大小不准确，会比实际值偏大。
建议先提取出 Base64 数据部分，再进行计算，以获得更准确的文件大小。
const commaIndex = result.resource.dataUrl.indexOf(',');
const base64Data = commaIndex >= 0 ? result.resource.dataUrl.slice(commaIndex + 1) : "";
const sizeInBytes = base64Data.length * 0.75;
const sizeMB = (sizeInBytes / (1024 * 1024)).toFixed(1);
return [{ type: "text", text: `[文件: ${fileName}, ${sizeMB}MB]` }]
|
/gemini review |
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/feishu/gateway.ts (1)
58-60:⚠️ Potential issue | 🟠 Major不要把代理 URL 原样写进日志。
这里会把
proxyUrl全量打到日志里；如果环境变量里带了user:pass@host，凭据会直接落盘。🔐 建议修复
if (proxyUrl) { wsAgent = new HttpsProxyAgent(proxyUrl) - log("info", "WS proxy enabled", { proxy: proxyUrl }) + let proxyForLog = proxyUrl + try { + const parsed = new URL(proxyUrl) + if (parsed.username || parsed.password) { + parsed.username = "***" + parsed.password = "***" + } + proxyForLog = parsed.toString() + } catch { + proxyForLog = "[invalid-proxy-url]" + } + log("info", "WS proxy enabled", { proxy: proxyForLog }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/feishu/gateway.ts` around lines 58 - 60, The code logs the full proxyUrl (used to construct wsAgent via new HttpsProxyAgent(proxyUrl)), which may contain credentials; change the logging to avoid writing sensitive user:pass into logs by parsing proxyUrl and redacting credentials or logging only the hostname/port/scheme. In the block that constructs wsAgent (refer to wsAgent, HttpsProxyAgent, and log("info", "WS proxy enabled", { proxy: proxyUrl })), replace the logged value with a sanitized string (e.g., remove url.username and url.password or use url.host) and log that instead.
♻️ Duplicate comments (1)
src/feishu/gateway.ts (1)
191-198:⚠️ Potential issue | 🟠 Major这里仍然会把部分群聊按钮误路由成 p2p 会话。
parseCardActionValue()在send_message分支里会把缺失的chatType默认成"p2p"，而这里又直接把它写进syntheticCtx。这样旧卡片或外部构造的按钮只要没带chatType，即使chatId已经改成回调上下文，后续 sessionKey 仍会按 p2p 生成，导致群会话复用和队列语义出错。请把
chatType保持为“缺失可见”的状态，再在这里基于回调上下文兜底；如果拿不到权威值，宁可拒绝该按钮，也不要静默默认成p2p。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/feishu/gateway.ts` around lines 191 - 198, The parsed card action is currently defaulting missing chatType to "p2p" (in parseCardActionValue's send_message branch), which then gets written into syntheticCtx and corrupts sessionKey generation; instead, preserve a missing/undefined parsedAction.chatType (do not default to "p2p"), and in this handler use the callback context to fill chatType only when the callback provides an authoritative value (e.g., if callbackChatId && callbackChatType exist use that); if after consulting callback context chatType is still unavailable, reject the button action (return an error or no-op) rather than silently defaulting to "p2p" so sessionKey/session routing remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/feishu/gateway.ts`:
- Around line 58-60: The code logs the full proxyUrl (used to construct wsAgent
via new HttpsProxyAgent(proxyUrl)), which may contain credentials; change the
logging to avoid writing sensitive user:pass into logs by parsing proxyUrl and
redacting credentials or logging only the hostname/port/scheme. In the block
that constructs wsAgent (refer to wsAgent, HttpsProxyAgent, and log("info", "WS
proxy enabled", { proxy: proxyUrl })), replace the logged value with a sanitized
string (e.g., remove url.username and url.password or use url.host) and log that
instead.
---
Duplicate comments:
In `@src/feishu/gateway.ts`:
- Around line 191-198: The parsed card action is currently defaulting missing
chatType to "p2p" (in parseCardActionValue's send_message branch), which then
gets written into syntheticCtx and corrupts sessionKey generation; instead,
preserve a missing/undefined parsedAction.chatType (do not default to "p2p"),
and in this handler use the callback context to fill chatType only when the
callback provides an authoritative value (e.g., if callbackChatId &&
callbackChatType exist use that); if after consulting callback context chatType
is still unavailable, reject the button action (return an error or no-op) rather
than silently defaulting to "p2p" so sessionKey/session routing remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dccb0c2a-e500-4977-95e2-ea0cc0865bd8
📒 Files selected for processing (8)
AGENTS.mdCLAUDE.mdsrc/feishu/content-extractor.tssrc/feishu/gateway.tssrc/feishu/markdown.tssrc/handler/chat.tssrc/handler/interactive.tssrc/handler/session-queue.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/feishu/markdown.ts
- src/handler/session-queue.ts
- src/handler/interactive.ts
|
/gemini review |
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/handler/interactive.ts (1)
25-52: 把这几个 payload 形状改成interface更贴合仓库约定。这里只有
ParsedCardActionValue需要保留联合type；PermissionReplyActionValue、QuestionReplyActionValue、SendMessageActionValue都是纯对象契约，用interface更一致，也更符合后续扩展习惯。 As per coding guidelines,**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handler/interactive.ts` around lines 25 - 52, Replace the three object type aliases PermissionReplyActionValue, QuestionReplyActionValue, and SendMessageActionValue with equivalent interfaces (keeping their property shapes, literal "action" values, optional chatType, and existing comments), and keep ParsedCardActionValue as the discriminated union type that references those interface names; ensure exported visibility remains the same and that no property names or literal types are changed so existing usages still type-check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/feishu/gateway.ts`:
- Around line 193-203: The current check treats any missing
parsedAction.chatType as an expired card and returns a warning, which breaks
legacy default send_message buttons that omit chatType; change this to a
backward-compatible branch: when parsedAction.action === "send_message" and
parsedAction.chatType is missing, do not return—log a compatibility warning and
allow processing to continue (or set a safe inferred chatType derived from
callbackChatId/payloadChatId if needed) so onMessage() still runs; only reject
when chatType is present but invalid or when it's genuinely ambiguous for
routing.
---
Nitpick comments:
In `@src/handler/interactive.ts`:
- Around line 25-52: Replace the three object type aliases
PermissionReplyActionValue, QuestionReplyActionValue, and SendMessageActionValue
with equivalent interfaces (keeping their property shapes, literal "action"
values, optional chatType, and existing comments), and keep
ParsedCardActionValue as the discriminated union type that references those
interface names; ensure exported visibility remains the same and that no
property names or literal types are changed so existing usages still type-check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11cf2547-4a22-4421-bae6-c690ed858355
📒 Files selected for processing (2)
src/feishu/gateway.tssrc/handler/interactive.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/feishu/gateway.ts (1)
266-310:⚠️ Potential issue | 🟠 MajorMove
chatTyperesolution into detached async path to unblock the callback response.The code awaits
resolveCardActionChatType()(which callslarkClient.im.chat.get()) before returning the callback response. If that lookup is slow, the click will miss Feishu's fixed 3-second response window and trigger a retry, causing flaky and potentially duplicate button handling.Wrap the async work in an IIFE and return
buildCallbackResponse(action, log)immediately:♻️ Suggested shape
- const resolvedChatType = await resolveCardActionChatType({ - chatId: targetChatId, - payloadChatType: parsedAction.chatType, - // 旧卡片缺 chatType 或 payload chatId 已明显过期时，必须查飞书权威会话信息再继续。 - requireAuthoritativeLookup: !parsedAction.chatType || chatIdMismatch, - larkClient, - log, - }) - if (!resolvedChatType) { - log("error", "send_message 按钮 chatType 无法确定，已拒绝处理", { - callbackChatId, - payloadChatId: parsedAction.chatId, - payloadChatType: parsedAction.chatType ?? "", - targetChatId, - operatorId: action.operatorId, - }) - return { - toast: { type: "warning", content: "⚠️ 卡片已过期，请重新触发" }, - } - } - if (!parsedAction.chatType) { - log("warn", "send_message 按钮缺少 chatType，已按飞书会话信息兼容推断", { - callbackChatId, - payloadChatId: parsedAction.chatId, - targetChatId, - resolvedChatType, - }) - } - const syntheticCtx: FeishuMessageContext = { - chatId: targetChatId, - messageId: `btn-${randomUUID()}`, - messageType: "text", - content: parsedAction.text, - rawContent: JSON.stringify({ text: parsedAction.text }), - chatType: resolvedChatType, - senderId: action.operatorId ?? "", - shouldReply: true, - } - void Promise.resolve(onMessage(syntheticCtx)).catch((err: unknown) => { + void (async () => { + const resolvedChatType = await resolveCardActionChatType({ + chatId: targetChatId, + payloadChatType: parsedAction.chatType, + requireAuthoritativeLookup: !parsedAction.chatType || chatIdMismatch, + larkClient, + log, + }) + if (!resolvedChatType) { + log("error", "send_message 按钮 chatType 无法确定，已拒绝处理", { + callbackChatId, + payloadChatId: parsedAction.chatId, + payloadChatType: parsedAction.chatType ?? "", + targetChatId, + operatorId: action.operatorId, + }) + return + } + + const syntheticCtx: FeishuMessageContext = { + chatId: targetChatId, + messageId: `btn-${randomUUID()}`, + messageType: "text", + content: parsedAction.text, + rawContent: JSON.stringify({ text: parsedAction.text }), + chatType: resolvedChatType, + senderId: action.operatorId ?? "", + shouldReply: true, + } + await onMessage(syntheticCtx) + })().catch((err: unknown) => { log("error", "send_message 按钮处理失败", { error: err instanceof Error ? err.message : String(err), }) }) // 即使后台还没处理完，也要马上给飞书回一个 toast。 return buildCallbackResponse(action, log)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/feishu/gateway.ts` around lines 266 - 310, The current flow awaits resolveCardActionChatType before returning, which can exceed Feishu's 3s callback window; detach the chatType lookup and downstream onMessage handling into an async background task (e.g., an immediately-invoked async function or Promise.resolve().then(async () => {...})) that performs resolveCardActionChatType(...) using larkClient, builds the synthetic FeishuMessageContext (chatId: targetChatId, messageId: `btn-${randomUUID()}`, messageType/text/rawContent/chatType from resolved value, senderId: action.operatorId, shouldReply), calls onMessage(syntheticCtx) and logs errors, while the outer function returns buildCallbackResponse(action, log) immediately; keep logging for missing chatType and authoritative lookup logic inside the detached task so the callback returns without awaiting network I/O.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/feishu/gateway.ts`:
- Around line 266-310: The current flow awaits resolveCardActionChatType before
returning, which can exceed Feishu's 3s callback window; detach the chatType
lookup and downstream onMessage handling into an async background task (e.g., an
immediately-invoked async function or Promise.resolve().then(async () => {...}))
that performs resolveCardActionChatType(...) using larkClient, builds the
synthetic FeishuMessageContext (chatId: targetChatId, messageId:
`btn-${randomUUID()}`, messageType/text/rawContent/chatType from resolved value,
senderId: action.operatorId, shouldReply), calls onMessage(syntheticCtx) and
logs errors, while the outer function returns buildCallbackResponse(action, log)
immediately; keep logging for missing chatType and authoritative lookup logic
inside the detached task so the callback returns without awaiting network I/O.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7e65f7d-12f8-44f5-9922-ebdc4cc08f11
📒 Files selected for processing (2)
src/feishu/gateway.tssrc/handler/interactive.ts
|
@codex review |
|
@claude review |
|
/gemini review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Claude finished @NeverMore93's task in 2m 36s —— View job PR Review: fix: harden feishu plugin error handlingTasks:
Overall AssessmentThis is a solid hardening PR. The core improvements are well-executed: log-level escalation, session-poisoning detection, 🔴 Critical: Gateway handler can block all WebSocket eventsFile:
// session-queue.ts — drainLoop blocks the first caller
async function handleQueuedMessage(...) {
state.queue.push({ ctx, deps })
if (state.processing) return // ← subsequent messages return immediately
await drainLoop(sessionKey, state) // ← first caller is blocked here
}Fix: Start if (state.processing) return
// Don't await — just start the loop and return immediately
void drainLoop(sessionKey, state).catch(err => ...)This is especially important because the old P2P interruptible strategy was explicitly removed, so there's no other way to avoid head-of-line blocking. 🟠 High:
|
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the Feishu plugin, including enhanced error handling, session management, and resource extraction logic. Key changes include a more robust FIFO message queue, better handling of streaming card updates, and improved error recovery mechanisms. The review feedback highlights a critical issue where invalidating a 'poisoned' session only clears the local cache, allowing the plugin to re-fetch the same invalid session from the server. I have included the suggested changes to implement a blacklist mechanism for sessions to ensure they are not reused after being marked as invalid.
| export function invalidateSession(sessionKey: string): void { | ||
| sessionCache.delete(sessionKey) | ||
| } |
There was a problem hiding this comment.
The invalidateSession(sessionKey) call only clears the local cache. However, getOrCreateSession searches for existing sessions on the server using a title prefix and picks the latest one. If a session is "poisoned" due to its history (as detected in chat.ts), simply invalidating the local cache will result in getOrCreateSession picking up the same poisoned session again from the server in the next request. To truly reset the session, the plugin should either rename the poisoned session on the server or getOrCreateSession should be modified to avoid reusing sessions that have been marked as invalid.
const blacklistedSessions = new TtlMap<true>(24 * 60 * 60 * 1_000)
/**
* 主动使指定逻辑会话失效。
*
* 下次 `getOrCreateSession()` 将重新去 OpenCode 侧查找或创建。
*/
export function invalidateSession(sessionKey: string, sessionId?: string): void {
sessionCache.delete(sessionKey)
if (sessionId) {
blacklistedSessions.set(sessionId, true)
}
}| const candidates = sessions.filter( | ||
| (s) => s.title && s.title.startsWith(titlePrefix), | ||
| ) |
There was a problem hiding this comment.
When filtering for candidate sessions to reuse, we should exclude those that have been explicitly blacklisted (e.g., due to session history poisoning).
| const candidates = sessions.filter( | |
| (s) => s.title && s.title.startsWith(titlePrefix), | |
| ) | |
| const candidates = sessions.filter( | |
| (s) => s.title && s.title.startsWith(titlePrefix) && !blacklistedSessions.has(s.id), | |
| ) |
| log("error", "检测到 session 历史数据中毒，创建新 session", { | ||
| sessionKey, oldSessionId: session.id, error: sessionError.message, | ||
| }) | ||
| invalidateSession(sessionKey) |
There was a problem hiding this comment.
Pass the session.id to invalidateSession so it can be blacklisted and not reused by getOrCreateSession in subsequent requests.
| invalidateSession(sessionKey) | |
| invalidateSession(sessionKey, session.id) |
|
Closing this PR and recreating a fresh one after a soft reset to origin/main. |
1 participant
Summary
Changes
src/handler/*src/feishu/*src/session.ts,src/types.tspackage.json,package-lock.json1.7.8Test Plan
npm run typechecknpm run buildnpm publish --dry-runSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores