fix(feishu): coalesce streaming card final delivery#71523
Conversation
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟡 Throttle bypass enables high-frequency Feishu Card API updates (application-layer DoS)
Description
Vulnerable code: function hasNaturalStreamingBoundary(text: string): boolean {
return /[\n。！？!?；;：:]$/.test(text);
}
function shouldPushStreamingUpdate(previousText: string, nextText: string): boolean {
if (!previousText) return true;
if (hasNaturalStreamingBoundary(nextText)) return true;
return nextText.length - previousText.length >= STREAMING_SIGNIFICANT_DELTA_CHARS;
}
if (!shouldForceUpdate && now - this.lastUpdateTime < this.updateThrottleMs) {
this.schedulePendingFlush();
return;
}RecommendationEnforce a hard rate limit for outbound update calls regardless of text boundaries. Options:
const now = Date.now();
if (now - this.lastUpdateTime < this.updateThrottleMs) {
this.pendingText = mergedInput;
this.schedulePendingFlush();
return;
}
this.lastUpdateTime = now;
const MAX_UPDATES_PER_SEC = 5;
// track a rolling window and refuse/schedule if exceededAlso consider defending-in-depth upstream by limiting how often streaming callbacks can enqueue updates. Analyzed PR: #71523 at commit Last updated on: 2026-04-25T10:02:10Z |
Greptile SummaryThis PR fixes Feishu streaming-card delivery so that multiple Confidence Score: 4/5Safe to merge; all findings are non-blocking style suggestions. No P0 or P1 bugs were identified. The coalescing logic, throttle refactor, and deduplication are all sound. Two P2 observations: the flush-timer callback uses void without a .catch (errors silently swallowed), and lastUpdateTime is stamped before the queued network call completes (throttle accuracy). A third P2 flags the private-field casting in tests. None of these affect production correctness. streaming-card.ts (flush timer error handling and throttle timestamp accuracy), streaming-card.test.ts (private-field cast fragility) Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/feishu/src/streaming-card.ts
Line: 356-363
Comment:
**Unhandled promise rejection in flush timer**
`void this.update(pending)` discards the returned promise. While `updateCardContent` logs its own errors, any unexpected throw inside `update()` (e.g., from `mergeStreamingText` or the queue chain) would produce a silent unhandled rejection — no log, no retry. Consider adding a `.catch` so failures are at least observable.
```suggestion
void this.update(pending).catch((e) => this.log?.(`Flush failed: ${String(e)}`));
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/feishu/src/streaming-card.ts
Line: 377-383
Comment:
**`lastUpdateTime` is stamped before the queue callback executes**
`this.lastUpdateTime = now` is set synchronously, before the queued `updateCardContent` call actually completes. If a prior item in the queue is still in-flight (e.g., a slow network round-trip), the throttle window starts ticking from the *queue-entry* time rather than the *send* time. Under sustained backlog this means the effective burst rate can exceed `1/updateThrottleMs`. Not a correctness issue for normal usage, but worth a comment if the 160 ms rate is a Feishu API constraint rather than a UX hint.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/feishu/src/streaming-card.test.ts
Line: 17-36
Comment:
**Private-field access via `as unknown as` is fragile**
`setStreamingSessionInternals` casts `FeishuStreamingSession` to a plain object shape and directly mutates `state` and `lastUpdateTime`. Because these fields are TypeScript `private`, any rename or layout change in the source class will silently break the tests at runtime with no compile-time warning. Consider exporting a dedicated `_forTest` seeding method (behind a `/* @internal */` comment) or using `vi.spyOn` on prototype methods to avoid the fragile cast.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(feishu): coalesce streaming card fin..." | Re-trigger Greptile |
| this.flushTimer = setTimeout(() => { | ||
| this.flushTimer = null; | ||
| const pending = this.pendingText; | ||
| if (!pending || this.closed) { | ||
| return; | ||
| } | ||
| void this.update(pending); | ||
| }, delayMs); |
There was a problem hiding this comment.
void this.update(pending) discards the returned promise. While updateCardContent logs its own errors, any unexpected throw inside update() (e.g., from mergeStreamingText or the queue chain) would produce a silent unhandled rejection — no log, no retry. Consider adding a .catch so failures are at least observable.
| this.flushTimer = setTimeout(() => { | |
| this.flushTimer = null; | |
| const pending = this.pendingText; | |
| if (!pending || this.closed) { | |
| return; | |
| } | |
| void this.update(pending); | |
| }, delayMs); | |
| void this.update(pending).catch((e) => this.log?.(`Flush failed: ${String(e)}`)); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/feishu/src/streaming-card.ts
Line: 356-363
Comment:
**Unhandled promise rejection in flush timer**
`void this.update(pending)` discards the returned promise. While `updateCardContent` logs its own errors, any unexpected throw inside `update()` (e.g., from `mergeStreamingText` or the queue chain) would produce a silent unhandled rejection — no log, no retry. Consider adding a `.catch` so failures are at least observable.
```suggestion
void this.update(pending).catch((e) => this.log?.(`Flush failed: ${String(e)}`));
```
How can I resolve this? If you propose a fix, please make it concise.| const shouldForceUpdate = shouldPushStreamingUpdate(this.state.currentText, mergedInput); | ||
| const now = Date.now(); | ||
| if (now - this.lastUpdateTime < this.updateThrottleMs) { | ||
| this.pendingText = mergedInput; | ||
| if (!shouldForceUpdate && now - this.lastUpdateTime < this.updateThrottleMs) { | ||
| this.schedulePendingFlush(); | ||
| return; | ||
| } | ||
| this.pendingText = null; | ||
| this.lastUpdateTime = now; |
There was a problem hiding this comment.
lastUpdateTime is stamped before the queue callback executes
this.lastUpdateTime = now is set synchronously, before the queued updateCardContent call actually completes. If a prior item in the queue is still in-flight (e.g., a slow network round-trip), the throttle window starts ticking from the queue-entry time rather than the send time. Under sustained backlog this means the effective burst rate can exceed 1/updateThrottleMs. Not a correctness issue for normal usage, but worth a comment if the 160 ms rate is a Feishu API constraint rather than a UX hint.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/feishu/src/streaming-card.ts
Line: 377-383
Comment:
**`lastUpdateTime` is stamped before the queue callback executes**
`this.lastUpdateTime = now` is set synchronously, before the queued `updateCardContent` call actually completes. If a prior item in the queue is still in-flight (e.g., a slow network round-trip), the throttle window starts ticking from the *queue-entry* time rather than the *send* time. Under sustained backlog this means the effective burst rate can exceed `1/updateThrottleMs`. Not a correctness issue for normal usage, but worth a comment if the 160 ms rate is a Feishu API constraint rather than a UX hint.
How can I resolve this? If you propose a fix, please make it concise.| messageId: string; | ||
| sequence: number; | ||
| currentText: string; | ||
| hasNote: boolean; | ||
| }; | ||
|
|
||
| function setStreamingSessionInternals( | ||
| session: FeishuStreamingSession, | ||
| values: { | ||
| state: StreamingSessionState; | ||
| lastUpdateTime?: number; | ||
| }, | ||
| ) { | ||
| const internals = session as unknown as { | ||
| state: StreamingSessionState; | ||
| lastUpdateTime: number; | ||
| }; | ||
| internals.state = values.state; | ||
| if (values.lastUpdateTime !== undefined) { | ||
| internals.lastUpdateTime = values.lastUpdateTime; |
There was a problem hiding this comment.
as unknown as is fragile
setStreamingSessionInternals casts FeishuStreamingSession to a plain object shape and directly mutates state and lastUpdateTime. Because these fields are TypeScript private, any rename or layout change in the source class will silently break the tests at runtime with no compile-time warning. Consider exporting a dedicated _forTest seeding method (behind a /* @internal */ comment) or using vi.spyOn on prototype methods to avoid the fragile cast.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/feishu/src/streaming-card.test.ts
Line: 17-36
Comment:
**Private-field access via `as unknown as` is fragile**
`setStreamingSessionInternals` casts `FeishuStreamingSession` to a plain object shape and directly mutates `state` and `lastUpdateTime`. Because these fields are TypeScript `private`, any rename or layout change in the source class will silently break the tests at runtime with no compile-time warning. Consider exporting a dedicated `_forTest` seeding method (behind a `/* @internal */` comment) or using `vi.spyOn` on prototype methods to avoid the fragile cast.
How can I resolve this? If you propose a fix, please make it concise.
Summary
Canonical for Feishu streaming-card delivery cluster C143848.
Supersedes #53534 and covers the duplicate-card/root streaming overlap in #39085, #42940, #48382, #48384, and #50822.
Credit: Thanks @allan0509 for the representative streaming-card delivery fix path.
Test plan
pnpm test:serial extensions/feishu/src/reply-dispatcher.test.ts extensions/feishu/src/streaming-card.test.tspnpm check:changedreached clean changed lanes after expanding the sparse worktree: conflict markers, extension prod/test typecheck, extension lint, import cycles, and 58 Feishu test files / 634 tests. The non-tty local wrapper intermittently exited before printing a final status, so the targeted serial test above was rerun after rebase.