feat(feishu): stream replies into a live interactive card - #7823
feat(feishu): stream replies into a live interactive card#7823roycema-vibecoding wants to merge 2 commits into
Conversation
Architecture check — raising the objection against my own PRBefore a reviewer has to spend a round-trip on it: I think there is a legitimate The objection
and its PR ⑤ is specifically:
Two facts follow, and neither favours this PR:
The duplication it warns about is real for streaming specifically: there is no shared So the strongest form of the objection is: extract a shared streaming helper first and The counter-argument, also measuredThe one thing I did deliberately is keep the machinery out of the channel. All of it —
Put plainly: of the seven streaming implementations this would be the only one already For accuracy, since I put this loosely elsewhere: Feishu is not the only channel What I would like a decision on
One practical note for whoever reviews: this needs the |
CI status: one red check, and it is not this diffFinal tally on The 11 skips are expected — a fork PR does not get the five AI reviewers or CodeQL. The red one is That is #7776 — "test isolation: the global session-create rate limiter makes For the record, the 15 files in this PR are I cannot re-run it from here — Also worth flagging from this run, since it was caught and fixed rather than argued |
|
Cross-link rather than a review: I filed #7846 for the Feishu proactive-send gap ( Two things in this diff I flagged in that issue as the precedent to follow rather than re-litigate, in case it is useful to have them named by someone outside the change:
Pushing cumulative text to One question I could not answer from the diff, and cannot test without a Lark app of my own, so treat it as a question and not a finding: |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
|
🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR has been inactive for 7+ days with failing CI. I've assessed the blockers and they appear resolvable — I'll push fixes directly to this branch as a co-author. Assessment: Sole conflict is a trivial append-at-EOF collision in If you'd prefer I don't touch this PR, add the |
The Feishu renderer buffers a whole turn and sends one text message on on_done, so a long answer leaves the bot looking dead, and a follow-up sent meanwhile is merged into a placeholder while the real reply arrives as an easily-missed quote-reply. Add an opt-in streaming mode that pushes the answer into a Feishu CardKit card as it arrives. The card is the platform's own streaming primitive: a card entity is created, one ordinary message references its card_id, and successive pushes carry the CUMULATIVE text, which the server diffs and animates. Editing a plain text message is deliberately not used -- the official Lark plugin treats that only as a degraded fallback. The mode is off by default (feishu.streaming) and direct-message only; a group turn keeps the buffered reply, because a card animating in a busy room is noise for everyone who did not ask. Every failure path falls back to the buffered text reply, so the worst case is today's behaviour rather than a lost answer. The base FEISHU_CAPABILITIES declaration is unchanged. A derived FEISHU_STREAMING_CAPABILITIES flips only streaming and edit, and only for a turn that actually streams, so a gateway with the flag off still declares itself a non-streaming channel. rich_blocks stays False because a CardKit card renders markdown, not tappable choices, and returns_message_id is untouched because the card id lives in the renderer, as it does for Telegram. Ported from two MIT-licensed OpenClaw Feishu plugins; attribution is recorded in NOTICE with the licence text in THIRD-PARTY-NOTICES.
The SAST scan flagged the CardKit call, and the per-file coverage floor was not met by the first commit: client.py sat at 77% and renderer.py at 72%, both under the 80 gate, because the card plumbing and the streaming branches had no tests of their own. Assert the Open Platform origin is https before the request is built, so a configured origin can never turn a urllib call into an arbitrary-file read, and suppress the audit rule the way this repo already does elsewhere -- with the check that makes the suppression sound rather than merely quiet. Add tests for the tenant-token cache, the 200-with-a-non-zero-body-code path, both HTTPError shapes, the refused non-https origin, and the interactive card reply; and for the streaming renderer: card opened once across the two on_turn_start calls, cumulative frames, the tool footer, a dead card, a raising push, and each on_done outcome including the recalled anchor that must not fall back to text. client.py 77% -> 96%, renderer.py 72% -> 98%.
7cfe41b to
51c64cb
Compare
|
Rebased onto main Conflicts resolved (both were textual, union-kept, no behaviour change):
Gates run locally on changed files: black, isort, flake8 clean; Please review the resolution. A maintainer push makes the maintainer the last pusher, so a second approver is needed under the repo's last-push rule. Reply if anything looks wrong. |
Design Review (Fable 5, fork) — ✅ PASSDesign-level review of The Design-Verdict: PASS Opt-in, DM-only, capability-honest, with every failure path landing on the pre-existing buffered reply — the right shape, thoroughly reasoned and pinned. Suggestions
[DESIGN-REVIEWED] 51c64cb |
Opus 4.8 Review (fork) — 🔴 changes requested (blocking)Reviewed A streaming turn whose final card push fails after an earlier push landed silently loses the answer's tail and suppresses the text fallback. BLOCKING — src/kiro_crew/feishu/streaming_card.py:494 A multi-chunk DM turn lands an early content push ( |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of I have everything I need: I read the review contract, the PR intent, the full authoritative diff, and verified against the trusted base that (a) per-channel streaming with a local throttle is the existing structure (Telegram/WeCom do the same, no shared helper exists), (b) the outbound-authz AST test really does read the first First-Principles-Verdict: CONCERNS The parallel HTTP/token path and the FEISHU_DOMAIN branch rest on unverified lark-oapi claims; the guarded import defends a partial-install ghost. Not justified as shipped
What this change shipsIntent: let a Feishu DM user watch the answer appear live instead of staring at a silent bot until the turn ends — an ADDITION.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 51c64cb |
GPT 5.6 Review (fork) —
|
Problem / Motivation
The Feishu channel renders a whole turn into a buffer and sends it as one plain-text
message on
on_done.FEISHU_CAPABILITIESaccordingly declaresstreaming=False,edit=False,rich_blocks=False.What a user observes:
complete — no typing indicator, no placeholder, no partial text.
answered with a placeholder, while the real answer arrives as a quote-reply
anchored to the earliest message of the batch — easy to miss entirely in a
busy DM.
markdown.
The renderer's own docstring anticipates the follow-up, and the module spec lists
edit-in-place streaming as the natural next step.
Why it matters
This is the difference between a channel that feels broken and one that feels
alive. For any turn that takes more than a couple of seconds — which is most
non-trivial ones — the user cannot tell whether the bot received the message, is
working, or has crashed, so the rational response is to send it again, which makes
things worse by triggering the merge-into-placeholder path.
Feishu/Lark is the primary work chat for a large number of teams in China, and it
is the one first-party channel where a long answer is invisible until it is
finished.
What changed (motivation → approach → change)
Goal. Show the answer as it is produced, without giving up the guarantee that
a failure still delivers the complete reply.
Approach, and the alternative rejected. The obvious approach is to
PATCH/im/v1/messages/:idper chunk. Both reference plugins reject it, and theofficial Lark plugin keeps it only as a degraded fallback: Feishu's real streaming
primitive is a CardKit card entity, where each push carries the cumulative
text and the server diffs successive pushes to animate them. The typewriter effect
is therefore server-side, and this change only decides how often to push. That
also means a dropped intermediate frame is self-repairing — the next push is
cumulative, and the final full replace fixes any gap — so no frame needs a retry.
An earlier reading of the community plugin's FAQ suggested streaming had been
abandoned over rate limits. That FAQ is stale and contradicted by its own shipped
source, which contains a complete streaming implementation gated behind an
opt-in flag. Corrected in #7548.
What was built.
src/kiro_crew/feishu/streaming_card.py(new) — the CardKit lifecycle: createthe card entity, reply with an
interactivemessage referencing itscard_id,push cumulative text on a throttle, then
PATCHstreaming_mode=falsebeforethe final full
PUTreplace. One monotonic sequence counter, never rolled backon failure, because Feishu enforces monotonicity and tolerates gaps.
codein the body on business errors:230020(rate limited) drops that oneframe and keeps streaming;
230099demotes excess tables once, then retires;230011/231003mean the anchor message is gone, which retires the sessionand suppresses the text fallback, since replying to a recalled anchor also
fails; anything else retires so the caller falls back to text.
renderer.py— dual mode. Opens the card onon_turn_start(idempotent: it iscalled twice per turn), pushes on chunks, seals on
on_done. Thestreaming_cardimport is guarded and degrades toStreamingCardSession = None,because
kiro_crew.channelsimports this package during gateway boot and a hardimport would turn one absent file into a gateway that cannot start at all.
transport.py—FEISHU_STREAMING_CAPABILITIES = replace(FEISHU_CAPABILITIES, streaming=True, edit=True). Built withdataclasses.replacerather than asecond literal so this module still holds exactly one
TransportCapabilities(...)call for the outbound-authz AST contract to read.
transport_dispatch.py— mode selection:feishu.streamingand a p2p chat.The flag is read strictly rather than through a
getattrdefault, so a configfield gone missing fails loudly instead of leaving a switch that can never turn
on.
config/sections.py,config/loader.py,config-baseline.json— the newboolean, parsed with the strict
_safe_bool, baseline regenerated withscripts/generate_config_baseline.py(484 → 485 entries).Deliberately not changed.
FEISHU_CAPABILITIESstill saysstreaming=False— with the flag off the channel really is non-streaming and must keep saying so.
rich_blocksstaysFalse: it gates whether a renderer attaches an Adaptive Cardof interactive elements, and a CardKit card here renders markdown, not tappable
choices.
max_buttonsstays0, so the zero-widget options path still applies.returns_message_idis untouched: the card id lives in the renderer instance,exactly as Telegram keeps its message id, and
send_message's contract (emptyreturn on success, raise on failure) is unchanged. No interactive buttons — that
needs a card-action callback route this channel does not have.
Not included on purpose. The
RuntimeError: This event loop is already runningcrash (#6534) is a separate defect with its own issue and is not touchedhere, keeping this PR to one concern. Note it does gate end-to-end verification of
this feature on a fresh install, so it is the one to land first.
Tests
test/test_feishu_streaming_card.py(new, 66 tests) takesstreaming_card.pyto 98% line coverage, above the per-file floor of 80:PATCH settingsprecedes the finalPUT.exception-only test would report success for a call that did nothing.
230020keeps the session live and the next frame lands;230099degrades then retires on the second hit;230011/231003retire andset
anchor_goneso no text fallback is attempted; an unrecognised code retires.deliveredin both states, since aTruethere tells the caller not to sendthe text fallback, and getting it wrong duplicates the whole answer.
tiny fragments.
returns the original text on internal failure.
test/test_feishu_dispatch.pygainsTestStreamingModeSelection(flag off; flagon + p2p; flag on + group) and its fake config now carries
streaming, which thestrict read requires.
Full run of the Feishu suite plus every guardrail named above — capability ledger,
outbound authz, options contract, pre-turn ratchet, config loader, config baseline:
908 passed.
Manual verification
Verified end-to-end against a live Feishu tenant before this PR, not just in unit
tests, since throttle behaviour and the business error codes can only be exercised
against the real Open Platform:
markdown (bold, ordered/unordered lists, inline code, links) renders correctly
in the card.
cardkit:card:writescope, granted and published on the OpenPlatform — the card calls 403 without it, which is worth knowing when reviewing.
feishu.streamingoff, behaviour is byte-for-byte the buffered reply.Two defects were found by the new tests and fixed in this branch rather than
shipped:
<br>spacers were being inserted on every fence line, so a literal<br>landed inside code blocks (they now go only on a block's outer edges, andan unterminated mid-stream fence correctly gets none); and the bare-URL pattern
excluded parentheses, which truncated URLs of the
..._(programming_language)shape and left the paren-balancing branchunreachable.
Related Issues
Checklist
Docs:
docs/system-specs/modules/messaging.md— the Feishu section now describesbuffered as the default mode rather than the only one. Note the existing
UNFINISHED [OPTIONStail rule was justified by there being no partial frame, sothat justification is now explicitly scoped to buffered mode: live frames use
split_options_trailer(text, hide_partial=True)and only the final frame uses thedefault, which keeps the doc self-consistent.
Third-party code:
streaming_card.pyis a port of two MIT-licensed projects, soper CONTRIBUTING this is called out explicitly — attribution added to
NOTICEwith the licence text reproduced in
THIRD-PARTY-NOTICES. No per-file copyrightheader was added, matching the repo's existing convention.