Skip to content

Keep Feishu card reasoning payloads#498

Merged
evandance merged 2 commits into
larksuite:mainfrom
zhulijin1991:codex/lark-stream-reasoning-payloads
May 15, 2026
Merged

Keep Feishu card reasoning payloads#498
evandance merged 2 commits into
larksuite:mainfrom
zhulijin1991:codex/lark-stream-reasoning-payloads

Conversation

@zhulijin1991
Copy link
Copy Markdown
Contributor

@zhulijin1991 zhulijin1991 commented May 11, 2026

Summary

  • Preserve reasoning payload text for the streaming card controller.
  • Route payloads marked isReasoning: true through the reasoning stream path instead of the visible answer delivery path.
  • Add coverage for both prefixed and bare reasoning payloads.

Trigger scenario

This came from the Feishu CardKit streaming path where reasoning chunks reached the dispatcher deliver branch. The previous isReasoning ? "" : ... filter dropped those chunks before the streaming card controller could render the reasoning panel.

Validation

  • pnpm vitest run tests/reply-dispatcher-tool-use.test.ts
  • git diff --check

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 11, 2026

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Collaborator

@evandance evandance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — the underlying issue is real (the dispatcher's isReasoning === true ? '' : ... filter was silently dropping reasoning payloads). The fix works for the test's payload shape: splitReasoningText (src/card/builder.ts:80-103) recognizes the 'Reasoning:\n' prefix and controller.onDeliver (streaming-card-controller.ts:465-473) routes through the pure-reasoning branch, so content lands in the reasoning panel as expected.

The concern is that the controller decides reasoning vs answer by text format (the prefix or <think> tags), not by payload.isReasoning. If a payload arrives with isReasoning: true but bare reasoning text — no prefix, no tags, possible from a future model variant or a different streaming format — splitReasoningText falls to its default branch and returns {answerText: text}. That gets appended to text.completedText and renders as the visible answer.

Two ways to harden:

  1. Route on the flag in the dispatcher: when payload.isReasoning === true, call controller.onReasoningStream(payload) instead of controller.onDeliver(...). onReasoningStream (line 494) already handles bare-text reasoning, and the dispatcher already exposes it on line 422.
  2. Make controller.onDeliver treat payload.isReasoning === true as pure reasoning up front, before falling through to splitReasoningText.

Either way, add a regression test for the bare format — { text: 'checking context', isReasoning: true } should still land in the reasoning area, not the answer body.

If you can also paste the trigger scenario (what SDK version / model / config causes reasoning to reach deliver instead of onReasoningStream) into the PR description, that'll tell us whether the fix belongs here or upstream.

@evandance evandance added feature request New feature or request messaging src/messaging/ + src/card/ — message rendering, cards, streaming changes requested Need do changes labels May 12, 2026
@zhulijin1991
Copy link
Copy Markdown
Contributor Author

Thanks, agreed. The current patch preserves the reasoning payload, but it still leaves reasoning-vs-answer classification to the text shape, which is fragile for bare reasoning chunks.

I will harden this by routing payload.isReasoning === true through the reasoning path in the dispatcher, rather than sending it through onDeliver and relying on splitReasoningText. I will also add the bare-text regression you suggested ({ text: "checking context", isReasoning: true }) so it lands in the reasoning area and never the visible answer body.

Trigger scenario: this came from the Feishu CardKit streaming path where reasoning chunks reached the dispatcher deliver branch, so the previous isReasoning ? "" : ... filter dropped them before the streaming card controller could render the reasoning panel. I will add that context to the PR description while updating the patch.

Copy link
Copy Markdown
Collaborator

@evandance evandance left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the rework — this is exactly the right shape. The dispatcher now routes reasoning payloads by the isReasoning flag rather than by text classification, and the two new tests pin both the prefix format and the bare-text case (checking context) to the reasoning lane with explicit onDeliver not called assertions. The trigger scenario from the Feishu CardKit streaming path is documented in the thread too. All concerns from the prior review are addressed.

LGTM, approving.

@evandance evandance removed the changes requested Need do changes label May 15, 2026
@evandance evandance merged commit 4ef2b08 into larksuite:main May 15, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature request New feature or request messaging src/messaging/ + src/card/ — message rendering, cards, streaming

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants