Skip to content

feat(feishu): stream replies into a live interactive card - #7823

Open
roycema-vibecoding wants to merge 2 commits into
kirodotdev:mainfrom
roycema-vibecoding:feat/feishu-streaming-card
Open

feat(feishu): stream replies into a live interactive card#7823
roycema-vibecoding wants to merge 2 commits into
kirodotdev:mainfrom
roycema-vibecoding:feat/feishu-streaming-card

Conversation

@roycema-vibecoding

Copy link
Copy Markdown

Problem / Motivation

The Feishu channel renders a whole turn into a buffer and sends it as one plain-text
message on on_done. FEISHU_CAPABILITIES accordingly declares streaming=False,
edit=False, rich_blocks=False.

What a user observes:

  • A long turn looks like a dead bot. Nothing at all appears until the answer is
    complete — no typing indicator, no placeholder, no partial text.
  • Sending a second message meanwhile gets merged into the in-flight turn and
    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.
  • The reply is plain text, so tables, headings and code fences arrive as raw
    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/:id per chunk. Both reference plugins reject it, and the
official 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: create
    the card entity, reply with an interactive message referencing its card_id,
    push cumulative text on a throttle, then PATCH streaming_mode=false before
    the final full PUT replace. One monotonic sequence counter, never rolled back
    on failure, because Feishu enforces monotonicity and tolerates gaps.
  • Error handling is code-driven, because Feishu answers HTTP 200 with a non-zero
    code in the body
    on business errors: 230020 (rate limited) drops that one
    frame and keeps streaming; 230099 demotes excess tables once, then retires;
    230011/231003 mean the anchor message is gone, which retires the session
    and 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 on on_turn_start (idempotent: it is
    called twice per turn), pushes on chunks, seals on on_done. The
    streaming_card import is guarded and degrades to StreamingCardSession = None,
    because kiro_crew.channels imports this package during gateway boot and a hard
    import would turn one absent file into a gateway that cannot start at all.
  • transport.pyFEISHU_STREAMING_CAPABILITIES = replace(FEISHU_CAPABILITIES, streaming=True, edit=True). Built with dataclasses.replace rather than a
    second literal so this module still holds exactly one TransportCapabilities(...)
    call for the outbound-authz AST contract to read.
  • transport_dispatch.py — mode selection: feishu.streaming and a p2p chat.
    The flag is read strictly rather than through a getattr default, so a config
    field gone missing fails loudly instead of leaving a switch that can never turn
    on.
  • config/sections.py, config/loader.py, config-baseline.json — the new
    boolean, parsed with the strict _safe_bool, baseline regenerated with
    scripts/generate_config_baseline.py (484 → 485 entries).

Deliberately not changed. FEISHU_CAPABILITIES still says streaming=False
— with the flag off the channel really is non-streaming and must keep saying so.
rich_blocks stays False: it gates whether a renderer attaches an Adaptive Card
of interactive elements, and a CardKit card here renders markdown, not tappable
choices. max_buttons stays 0, so the zero-widget options path still applies.
returns_message_id is untouched: the card id lives in the renderer instance,
exactly as Telegram keeps its message id, and send_message's contract (empty
return 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 running crash (#6534) is a separate defect with its own issue and is not touched
here, 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) takes
streaming_card.py to 98% line coverage, above the per-file floor of 80:

  • The five-step lifecycle in order, and that PATCH settings precedes the final
    PUT.
  • Sequence numbers strictly increase and are never rolled back after a failure.
  • Every content push carries the full cumulative text, not a delta.
  • The HTTP 200 with a non-zero body code path specifically — an
    exception-only test would report success for a call that did nothing.
  • Each error class: 230020 keeps the session live and the next frame lands;
    230099 degrades then retires on the second hit; 230011/231003 retire and
    set anchor_gone so no text fallback is attempted; an unrecognised code retires.
  • delivered in both states, since a True there tells the caller not to send
    the text fallback, and getting it wrong duplicates the whole answer.
  • The throttle, the forced push, and the anti-stutter deferral applying only to
    tiny fragments.
  • Every markdown fixup, including that each protects fenced/inline code and
    returns the original text on internal failure.

test/test_feishu_dispatch.py gains TestStreamingModeSelection (flag off; flag
on + p2p; flag on + group) and its fake config now carries streaming, which the
strict 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:

  • A card appears within roughly a second of sending and fills in progressively;
    markdown (bold, ordered/unordered lists, inline code, links) renders correctly
    in the card.
  • Requires the cardkit:card:write scope, granted and published on the Open
    Platform — the card calls 403 without it, which is worth knowing when reviewing.
  • With feishu.streaming off, behaviour is byte-for-byte the buffered reply.
  • Group turns keep the buffered reply with the flag on.

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, and
an 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 branch
unreachable.

Related Issues

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

Docs: docs/system-specs/modules/messaging.md — the Feishu section now describes
buffered as the default mode rather than the only one. Note the existing
UNFINISHED [OPTIONS tail rule was justified by there being no partial frame, so
that justification is now explicitly scoped to buffered mode: live frames use
split_options_trailer(text, hide_partial=True) and only the final frame uses the
default, which keeps the doc self-consistent.

Third-party code: streaming_card.py is a port of two MIT-licensed projects, so
per CONTRIBUTING this is called out explicitly — attribution added to NOTICE
with the licence text reproduced in THIRD-PARTY-NOTICES. No per-file copyright
header was added, matching the repo's existing convention.

@roycema-vibecoding
roycema-vibecoding requested a review from a team as a code owner September 2, 2026 07:46
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026
@roycema-vibecoding

Copy link
Copy Markdown
Author

Architecture check — raising the objection against my own PR

Before a reviewer has to spend a round-trip on it: I think there is a legitimate
architectural argument against merging this as-is. I would rather put it on the table
myself, with the measurements, than have it discovered.

The objection

docs/request-for-change/rfc-channel-plugin-architecture.md (status partial, doc-pr
#689) exists because every channel is built by copying the previous one — 7 duplicated
turn-dispatch skeletons, 3,796 lines, 9 hand-edited core files per channel. Its §1 closes
with:

An 8th channel (Feishu) is planned. On the current architecture it is copy #8 of the
skeleton and a tenth walk through the seams.

and its PR ⑤ is specifically:

PR ⑤ — Feishu ships as the first contract-native channel. … Estimated ~900 new
lines (adapter + panel) versus ~2,400 on the copy model.

Two facts follow, and neither favours this PR:

  1. Feishu shipped on the copy model rather than as the contract-native pilot, and PR ⑤ is
    still listed unstarted. Feishu is also not one of the 4 channels driving turns through
    messaging/dispatch.py (weixin, wecom, webex, teams).
  2. This PR takes src/kiro_crew/feishu/ from 1,368 to 2,167 lines (+799, +58%). The
    RFC predicted ~2,400 for Feishu on the copy model. I am, almost exactly, the outcome
    it warned about.

The duplication it warns about is real for streaming specifically: there is no shared
streaming/throttle helper
. messaging/renderer.py provides the event hooks plus the
table/redaction helpers and nothing else, so every streaming channel rolls its own.
Grepping the renderers for throttle|_stream_mid|_last_push|min_interval|edit_message:
telegram 38, discord 21, webex 12, wecom 10, slack 5, teams 5. This PR would be the 7th
such implementation.

So the strongest form of the objection is: extract a shared streaming helper first and
Feishu gets it for free; merging this instead makes that extraction bigger.
That is a
fair thing to say and I am not going to pretend otherwise.

The counter-argument, also measured

The one thing I did deliberately is keep the machinery out of the channel. All of it —
the CardKit lifecycle, the sequence counter, the single-flight throttle, the error-code
taxonomy, the markdown fixups — lives in one new module behind a six-member interface:

live / anchor_gone / delivered   (properties)
start()                 -> bool
push(text, *, force=False)
finish(text)            -> bool

feishu/renderer.py touches it in 8 places and never reaches past that interface. The
same grep that scores telegram's renderer at 38 scores Feishu's at 1. Every tunable is
a module-level constant in that one file (CARDKIT_THROTTLE_S, LONG_GAP_THRESHOLD_S,
BATCH_AFTER_GAP_S, the ERR_* codes).

Put plainly: of the seven streaming implementations this would be the only one already
shaped like the shared helper the RFC wants, rather than smeared through a renderer. It is
therefore the cheapest one to re-home later — and arguably the best available starting
point for the extraction, since the throttle / backoff / error-taxonomy split is already
factored out.

For accuracy, since I put this loosely elsewhere: Feishu is not the only channel
declaring streaming=False. Five declare True (slack, discord, telegram, wecom,
whatsapp) and five False (feishu, imessage, teams, webex, weixin) — though teams and
webex both declare edit=True, so updating in place without claiming streaming is already
precedent in this tree.

What I would like a decision on

  1. Merge as-is. It is opt-in (feishu.streaming, default off), DM-only, and every
    failure path falls back to today's buffered reply, so the blast radius with the flag off
    is nil. The RFC debt is real but unchanged in kind: this adds no new seam, it grows
    one module inside an existing channel.
  2. Have me extract messaging/streaming.py first — a shared throttled
    cumulative-push session — and land Feishu on top of it, either in this PR or in a
    preceding one. I am happy to do that; it is more work but it is where the RFC points. If
    you want it, say whether the first port should be Feishu alone or whether telegram and
    discord should move onto it in the same change. I would advise not, given how much of
    the recent conflict pain in this area has been concurrent edits to the same channel
    files.
  3. Hold this until PR ⑤. Reasonable — but note it leaves Feishu the channel where a
    long answer is invisible until it is finished, and Feishu channel: RuntimeError 'This event loop is already running' prevents connection #6534 already gates the channel
    entirely, so "later" here is compounding rather than neutral.

One practical note for whoever reviews: this needs the cardkit:card:write scope, granted
and published on the Open Platform. It is a separate permission set from
im:message:* and the card calls fail closed without it. I verified the full path against
a live tenant, which is the part the automated triage on #7548 correctly said it could not
exercise.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026
@roycema-vibecoding

Copy link
Copy Markdown
Author

CI status: one red check, and it is not this diff

Final tally on 7cfe41b0a: 54 green, 11 skipped, 1 red.

The 11 skips are expected — a fork PR does not get the five AI reviewers or CodeQL.

The red one is Backend Tests (Windows) (3), and it fails in
test/test_session_control.py, not in anything this PR touches:

test_the_audit_write_does_not_run_on_the_event_loop
test_the_created_agent_name_is_sanitized_before_storage
  SessionControlError: too many sessions created recently; retry shortly

That is #7776"test isolation: the global session-create rate limiter makes
unrelated test shards fail intermittently"
— filed earlier today and already
labelled auto-fixable / claimed. The shared limiter carries state across tests in
a shard, so an unrelated shard trips it depending on ordering.

For the record, the 15 files in this PR are feishu/*, config/sections.py,
config/loader.py, config-baseline.json, the messaging module spec, NOTICE,
THIRD-PARTY-NOTICES, and four test files. Nothing under dashboard/, and no path
that session_control imports. The same Windows shard's Python 3.10 and 3.12
counterparts are green on this commit, which is the ordering-dependence #7776
describes rather than a platform problem in this change.

I cannot re-run it from here — gh run rerun answers cannot be rerun for a fork
contributor. A maintainer re-run should clear it; happy to rebase once #7776 lands if
you would rather see it green from a clean run.

Also worth flagging from this run, since it was caught and fixed rather than argued
with: the SAST scan flagged the CardKit urlopen call. Rather than suppress it, the
Open Platform origin is now asserted to be https before the request is built — it
comes from config, and urllib honours file:// — and only then is the audit rule
suppressed, matching the two existing precedents in this repo
(ops_mission_control/backend/providers/http.py, design_tweak/backend/dev_preview.py).
The second commit also lifted client.py 77% → 96% and renderer.py 72% → 98%, both
of which were under the 80 per-file floor after the first commit.

@JiaDe-Wu

JiaDe-Wu commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Cross-link rather than a review: I filed #7846 for the Feishu proactive-send gap (supports_proactive_send=False, the only channel where it is), and it touches every file this PR does — feishu/transport.py, client.py, renderer.py, transport_dispatch.py. This one should land first. It is far larger, and rebasing a small addressing change onto it is cheap where the reverse is not. I have not claimed the other work and would rebase onto whatever lands here.

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:

FEISHU_STREAMING_CAPABILITIES built with dataclasses.replace — keeping one TransportCapabilities(...) literal in the module so the AST-walking outbound-authz test still reads the real declaration — is the right shape for adding a capability to this channel without disturbing the base. I proposed following it rather than inventing a second pattern.

Pushing cumulative text to push() rather than accumulating deltas inside the card is also worth naming, because the alternative has a specific failure mode. On another project I recently fixed a streaming path that appended each delta to a flat list and joined it with the block separator: a delta is routinely a fragment of a single word, so the separator welded itself into the middle of words (" div" + "iding"" div\n\niding"), 88 characters the model never emitted in one 45-delta reply, and the live display looked perfect the whole time because the raw chunk went to the renderer while only the stored copy was corrupted. Cumulative text cannot express that bug. The _ANTI_STUTTER_MAX_CHARS bound reads like it was written by someone who had already thought about the "nothing schedules a retry" consequence, which is the part that usually gets missed.

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: CARDKIT_THROTTLE_S = 0.10 permits up to ten patches per second per card. ERR_RATE_LIMITED (230020) is handled by dropping the frame and staying live, which is the right policy — but if Lark's published ceiling for the CardKit streaming update is below 10/s, then dropping becomes the normal path rather than the exception, and a stream that is silently shedding most of its frames looks the same in the code as one that is not. Did you get to measure the observed limit against a real app, or is 0.10 chosen as a conservative guess? If it is a guess, a one-line comment saying so would save the next person the same question.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 3, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • This PR is OVERLAPPING with PR #8167. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7823: REBASE. The only blocker is a trivial append-at-EOF collision in one test file; both classes belong in the file and no code hunk conflicts. Files: test/test_feishu_dispatch.py.
  • This PR is OVERLAPPING with PR #8228. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7823: REBASE. Complementary Feishu improvements in opposite directions with no textual or semantic conflict; neither needs to wait on the other. Files: src/kiro_crew/feishu/client.py, src/kiro_crew/feishu/transport.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@dwu96 dwu96 added the needs-pr-triage PR scanner: awaiting automated triage label Sep 7, 2026
@NicholasRBowers NicholasRBowers added drive-to-green PR claimed by drive-to-green pipeline and removed needs-pr-triage PR scanner: awaiting automated triage labels Sep 7, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 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 test/test_feishu_dispatch.py vs merged #8167 (keep both test classes), and the only CI red is the known Windows session-control rate-limiter flake (#7776). A rebase mints a new head, re-runs CI, and re-fires the skipped fork review lanes.

If you'd prefer I don't touch this PR, add the pr-no-autofix label.

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%.
@bolichen97
bolichen97 force-pushed the feat/feishu-streaming-card branch from 7cfe41b to 51c64cb Compare September 8, 2026 18:20
@bolichen97

Copy link
Copy Markdown
Collaborator

Rebased onto main 1192049a by a maintainer as part of the 2026-09-08 open-PR audit (was 7cfe41b0, now 51c64cbe).

Conflicts resolved (both were textual, union-kept, no behaviour change):

  • src/kiro_crew/feishu/client.py: main's _WS_CLIENT_READY_TIMEOUT_SECS and this PR's FEISHU_OPEN_BASE / _CARD_HTTP_TIMEOUT_S / CardApiError both kept.
  • test/test_feishu_dispatch.py: append-at-EOF collision with merged fix(channels): gate /compact on backend capability (#8156) #8167; both TestCompactCapabilityGate and TestStreamingModeSelection kept.

config-baseline.json auto-merged and matches scripts/generate_config_baseline.py output exactly.

Gates run locally on changed files: black, isort, flake8 clean; pytest test/test_feishu_{client,dispatch,renderer,streaming_card}.py 193 passed; test/test_config_baseline.py 10 passed.

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.

@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 8, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 51c64cbeb7bd6f684234277e6d829883c025932f via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The streaming/edit capability flags are declarative-only ("do NOT write code that assumes they are enforced" in transport.py:126), so the per-turn declaration swap is safe. The client already requires lark-oapi (built in __init__), the messaging spec is updated in the same commit, attribution for the ported code is handled in NOTICE/THIRD-PARTY-NOTICES, and the failure policy consistently degrades to the pre-existing buffered reply. I've checked the design gate questions; the review follows.

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

  • The hand-rolled urllib + private tenant-token cache in client.py duplicates auth/transport the lark-oapi client already maintains; if the pinned lark-oapi>=1.4,<2 range exposes its generic raw-request door (client.request with a BaseRequest), routing the CardKit calls through it would delete the second token cache, the https-scheme guard, and the envelope re-parsing — the stated rationale (typed CardKit modules absent from older builds) doesn't rule that door out, so worth one version check before this plumbing calcifies.

[DESIGN-REVIEWED] 51c64cb

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — 🔴 changes requested (blocking)

Reviewed 51c64cbeb7bd6f684234277e6d829883c025932f via the fork AI-review pipeline; updated in place on each push.

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

        await self._flush(force=True)
        if self._shown:
            self._delivered = True
        if not self.live:
            return self._delivered

A multi-chunk DM turn lands an early content push (_shown = a partial body), then the forced final push in finish() fails with any retiring error (transport RuntimeError, a 5xx, an unrecognised code, or a second ERR_CARD_CONSTRAINT); _classify sets _retired=True and leaves _shown at the stale partial, so if self._shown: sets _delivered=True and if not self.live: return self._delivered returns True before step 4/5 can repair the frame → on_done sees delivered and skips send_reply, so the card is frozen in streaming mode on the truncated partial, the tail is lost, and the buffered fallback that would have delivered the whole answer is suppressed while history records the turn as delivered.
Fix: gate the flag on the full body actually landing — if self._shown == prepare_card_text(text, demote_tables=self._demote_tables): self._delivered = True — so a failed final push falls through to the text reply instead of claiming delivery (this also cures the mirror case where every push is rate-limited and only the step-5 replace succeeds, which otherwise returns delivered=False and double-sends).

[BLOCK-MERGE] 51c64cb
[OPUS-REVIEWED] 51c64cb

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 51c64cbeb7bd6f684234277e6d829883c025932f via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 TransportCapabilities(...) call (justifying the dataclasses.replace form), (c) channels.py imports the Feishu renderer chain at module top-level (so the boot claim is mechanically true), and (d) the base LarkClient builds the lark-oapi SDK client with no domain override. Findings below.

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

  1. Missing-module fallback — oversized: the module ships in the same wheel as its importer; the "update that overwrites src/" premise has no provenance you can point at.

What this change ships

Intent: let a Feishu DM user watch the answer appear live instead of staring at a silent bot until the turn ends — an ADDITION.

  1. DM replies stream into a live card that fills in as produced (opt-in) — justified
  2. New config toggle feishu.streaming, off by default — justified
  3. Group chats always keep the buffered reply, flag on or off — justified
  4. Card text adapted to Feishu's renderer (headings clamped, URLs linkified, non-Feishu images dropped, excess tables demoted) — justified
  5. Tool calls now show live as a card footer mid-turn — justified
  6. Any card failure falls back to the buffered reply; a recalled inbound message now gets no reply at all — justified
  7. Client gains a second, raw-HTTP Open Platform path with its own token cache (card_api, send_card_reply, CardApiError) — justified
  8. A missing streaming_card module degrades to buffered with a warning instead of failing boot — oversized: guards a partial-install scenario with no provenance
  9. Half-arrived [OPTIONS markers withheld from live frames; final reply unchanged — justified
  10. NOTICE/THIRD-PARTY-NOTICES carry the ported plugins' MIT attribution — justified

Watch

  • Item 7 rests on "they are absent from older lark-oapi builds" (client.py comment). The supported floor is lark-oapi>=1.4,<2 (base client.py:11). If 1.4 already ships the CardKit verbs or a raw request method, ~100 lines of token cache + urllib plumbing duplicate the SDK client already built in LarkClient.__init__ (base client.py:174-180).
    Clears when: the author confirms lark-oapi 1.4 lacks both.
  • "Domain follows the SDK's own constant... so a Lark tenant is not sent to feishu.cn" — lark_oapi.FEISHU_DOMAIN is the fixed Chinese-domain constant (LARK_DOMAIN is the international one), and nothing in this channel configures a domain (base client builds the SDK client without one), so the branch cannot do what its comment claims.
    Clears when: verified on a larksuite tenant, or the getattr is dropped for the plain constant.

Subtractions

  • Delete the try/except ImportError around streaming_card in renderer.py and the StreamingCardSession is None branch in _open_card — a real packaging fault fails the suite loudly; take the hard import.
  • Drop the getattr(self._lark_mod, "FEISHU_DOMAIN", "") lookup in LarkClient.__init__ — take FEISHU_OPEN_BASE directly; identical behaviour on every real build.

[FIRST-PRINCIPLES-REVIEWED] 51c64cb

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ⚠️ review incomplete

Reviewed 51c64cbeb7bd6f684234277e6d829883c025932f via the fork AI-review pipeline; updated in place on each push.

No completed GPT verdict for this commit; see the Fork GPT 5.6 Review job logs.

Captured review output -- no verdict stamp for this commit, so this is NOT a verdict

Incomplete review: pass(es) 1 did not complete.

The pass-2 verdict was not published because both calls are required. Re-run the workflow for a complete review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drive-to-green PR claimed by drive-to-green pipeline fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants