feat(lark-bot): swap typing reaction to done after reply#487
Conversation
Replace the immediate "OK" reaction with "Typing" so the user sees the bot working on it; once the reply lands, list the bot's Typing reactions (operator_type=app), delete each, and add "DONE". Uses list-based discovery instead of caching reaction_id on the singleton runner — keeping reaction state on the runner would violate the "no in-process cache as fact source" rule. Filtering on operator_type=app preserves any user-added Typing reactions on the same message. Wired post-reply at TryHandleAgentBuilderAsync (direct slash/card replies) and RunLlmReplyAsync (the deferred LLM path). RunLlmReplyAsync now resolves registration on the relay path too so NyxProviderSlug is available for the swap; the relay reply itself still doesn't need it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af79b5b827
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // it) so the post-reply Lark typing→done reaction swap can find NyxProviderSlug. Skipping | ||
| // resolution here only when HasRelayDelivery would silently disable the swap on the most | ||
| // common production path. Direct path still requires registration to send the reply. | ||
| var registration = await ResolveRegistrationForReplyAsync(reply, ct); |
There was a problem hiding this comment.
This unconditional ResolveRegistrationForReplyAsync call runs before every reply, including relay delivery where registration is not required to send the outbound message. If the registration read model is transiently unavailable (timeout/store error), the exception now aborts RunLlmReplyAsync before SendReplyAsync, which can drop otherwise valid relay replies. The registration fetch for reaction swap should be best-effort on relay paths (catch/log and continue with registration = null).
Useful? React with 👍 / 👎.
| var listResponse = await _nyxClient.ProxyRequestAsync( | ||
| accessToken!, | ||
| providerSlug!, | ||
| $"/open-apis/im/v1/messages/{Uri.EscapeDataString(platformMessageId!)}/reactions?reaction_type={TypingReactionEmojiType}&page_size=50", | ||
| "GET", |
There was a problem hiding this comment.
The swap logic requests only the first reactions page (page_size=50) and immediately proceeds with deletion/addition. The Lark response shape is paginated (has_more/page_token), so in chats with more than 50 Typing reactions, the bot-owned reaction may be on a later page and never deleted, leaving stale Typing alongside DONE. Iterate through all pages before adding the final DONE reaction.
Useful? React with 👍 / 👎.
eanzhao
left a comment
eanzhao
left a comment
There was a problem hiding this comment.
Found two additional issues in the reaction swap path.
| runtimeContext, | ||
| ct); | ||
| if (result.Success) | ||
| _ = TrySwapTypingReactionToDoneAsync(inbound, registration, ct); |
There was a problem hiding this comment.
P1: Cover the default streaming relay completion path
This swap only runs when ConversationGAgent reaches RunLlmReplyAsync, but relay streaming is enabled by default (NyxIdRelayOptions.StreamingRepliesEnabled = true). On successful streamed replies, HandleLlmReplyReadyAsync returns from TryCompleteStreamedReplyAsync after final RunStreamChunkAsync/PersistStreamedCompletionAsync, so this code is never called. That leaves the inbound Typing reaction in place for the normal relay LLM path. The swap needs to be triggered from the streamed completion/final-chunk path as well, or exposed as a runner hook that TryCompleteStreamedReplyAsync can invoke after the final visible reply succeeds.
| registration, | ||
| runtimeContext, | ||
| ct); | ||
| if (result.Success) |
There was a problem hiding this comment.
P2: Avoid racing the fire-and-forget Typing POST on direct replies
RunInboundAsync starts TrySendImmediateLarkReactionAsync without awaiting it, then direct slash/card flows can reach this swap almost immediately. If the list request runs before Lark has persisted the initial Typing reaction, ParseAppReactionIds sees no bot reaction, DONE is added, and the original Typing POST can complete afterwards, leaving both Typing and DONE on the message. Direct-reply paths need to sequence the initial reaction with the swap, or make the swap tolerate this race with a bounded retry/re-list before adding the final DONE.
eanzhao
left a comment
eanzhao
left a comment
There was a problem hiding this comment.
Review Summary
Two real issues, both confirmed by code path tracing:
P1 — Streaming relay path skips the reaction swap entirely. This is the biggest gap. StreamingRepliesEnabled defaults to true, and when streaming succeeds, HandleLlmReplyReadyAsync returns early from TryCompleteStreamedReplyAsync at ConversationGAgent.cs:477, completely bypassing RunLlmReplyAsync (and its swap). The existing test HandleLlmReplyReadyAsync_WhenStreamingSucceeded_PersistsCompletedWithoutInvokingRunLlmReply explicitly asserts runner.LlmReplyCount.ShouldBe(0). So on the most common production path (streamed LLM replies), the Typing reaction is never swapped to DONE.
P2 — Race on direct-reply slash commands. RunInboundAsync fires the Typing POST (line 62) with _ = fire-and-forget, then TryHandleAgentBuilderAsync can complete a fast direct reply and trigger the swap at line 331 before Lark has persisted the Typing reaction. The swap's GET list finds nothing, adds DONE, then the original Typing POST completes, leaving both reactions. Narrow window, but architecturally real. Card actions are safe (gate excludes non-Message types). LLM path is safe (LLM latency >> reaction POST latency).
The non-streaming relay and direct-slash paths that go through RunLlmReplyAsync look correct. The list-filter-delete-add sequence, operator_type=app filtering, error handling, and fire-and-forget exception safety are all well done.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## dev #487 +/- ##
=======================================
Coverage 71.03% 71.04%
=======================================
Files 1230 1230
Lines 89062 89062
Branches 11654 11654
=======================================
+ Hits 63268 63271 +3
+ Misses 21257 21256 -1
+ Partials 4537 4535 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…ly race P1: HandleLlmReplyReadyAsync returns early via TryCompleteStreamedReplyAsync when streaming succeeds (the most common production path), bypassing RunLlmReplyAsync where the swap was wired. Add OnReplyDeliveredAsync to IConversationTurnRunner with a default no-op; ConversationGAgent calls it after streaming completion so the runner can finalize platform-specific post-reply state. ChannelConversationTurnRunner implements it by resolving registration and running the existing typing→done swap helper. P2: Direct-reply slash commands (TryHandleAgentBuilderAsync) can complete faster than the typing POST takes to land in Lark, causing the swap GET to miss the typing reaction and leave both Typing + DONE on the message. Capture the typing-reaction Task in RunInboundAsync, thread it through TryHandleAgentBuilderAsync, and await it (with a 2s cap) before the swap issues its GET. Deferred-LLM and streaming paths skip this guard because their natural latency dwarfs the typing POST. Tests: - OnReplyDeliveredAsync_ShouldRunSwap_WhenStreamingPathInvokesIt - OnReplyDeliveredAsync_ShouldNoOp_WhenActivityIsNotLark - RunInboundAsync_ShouldAwaitTypingReactionBeforeSwap_ForDirectAgentBuilderReply (TypingReactionGateHandler parks typing POST; asserts GET is not issued until typing releases, then confirms POST→GET→DELETE→POST DONE order) - HandleLlmReplyReadyAsync_WhenStreamingSucceeded asserts OnReplyDelivered is called exactly once - HandleLlmReplyReadyAsync_WhenStreamingDisabled asserts OnReplyDelivered is NOT called (would double-fire with the swap inside RunLlmReplyAsync) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
eanzhao
left a comment
eanzhao
left a comment
There was a problem hiding this comment.
Fix Review — LGTM
Both P1 and P2 are addressed correctly.
P1 (streaming path): OnReplyDeliveredAsync hook with default no-op on the interface, called from ConversationGAgent only on the streaming-early-return branch. Non-streaming path goes through RunLlmReplyAsync with its own swap, no double-fire. Tests assert OnReplyDeliveredCount = 1 (streaming) and 0 (non-streaming).
P2 (race condition): TrySendImmediateLarkReactionAsync Task is captured and threaded through to TryHandleAgentBuilderAsync → AwaitTypingReactionThenSwapAsync which awaits it with a 2-second cap before the swap GET. RunLlmReplyAsync skips this guard (LLM latency >> POST latency). TypingReactionGateHandler test is well designed — parks the typing POST, asserts zero swap requests before release, then verifies POST→GET→DELETE→POST DONE ordering.
No issues found. Ship it.
…nate Typing list
Codex review feedback:
P1: ResolveRegistrationForReplyAsync runs on every reply, including the relay
path where the actual reply uses the reply token and never needs registration.
A transient registration-store error then aborted RunLlmReplyAsync and dropped
otherwise-valid relay replies. Branch the lookup: mandatory on the direct path
(unchanged), best-effort on the relay path — caught exceptions log a warning
and degrade the swap to a no-op for that turn while the reply still lands.
P2: TrySwapTypingReactionToDoneAsync only fetched the first 50 Typing
reactions. In chats with more than 50 Typing reactions on a single message
(rare but possible), the bot's own reaction can land on a later page, never
get deleted, and remain alongside DONE. Walk page_token until has_more=false
(safety cap of 10 pages = 500 reactions). Page_token is only followed when
has_more=true so a Lark response that returns a stale token can't loop.
Tests:
- RunLlmReplyAsync_RelayPath_ShouldStillReplyAndSkipSwap_WhenRegistrationLookupThrows
— registration store throws InvalidOperationException; relay reply still
lands via the relay handler; swap is correctly skipped (no nyx calls).
- RunLlmReplyAsync_ShouldPaginate_WhenTypingReactionListSpansMultiplePages
— page 1 returns user-only reaction with has_more=true; page 2 returns
bot reaction with has_more=false; asserts GET → GET (with page_token) →
DELETE → POST DONE in correct order.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
OK→Typing(Lark's keyboard-typing hands) so the user sees the bot working on it.Typingreactions (filteroperator_type=appso user-added Typing is preserved), deletes each, and addsDONE.reaction_idon the singleton runner — keeping cross-turn reaction state on a service-layer dictionary would violate the "禁止中间层进程内缓存作为事实源" rule.TryHandleAgentBuilderAsync(direct slash/card replies) andRunLlmReplyAsync(the deferred LLM path, which is the main message → reply flow).RunLlmReplyAsyncnow resolves registration even on the relay path soNyxProviderSlugis available for the swap; the relay reply itself still doesn't need it.Test plan
dotnet build agents/Aevatar.GAgents.NyxidChat/Aevatar.GAgents.NyxidChat.csproj— cleandotnet test test/Aevatar.GAgents.ChannelRuntime.Tests/Aevatar.GAgents.ChannelRuntime.Tests.csproj— 580/580 pass, including:RunInboundAsync_ShouldSendImmediateLarkReaction_*asserts"emoji_type":"Typing"RunLlmReplyAsync_ShouldSwapTypingReactionToDone_AfterSuccessfulRelayReplyverifies GET → DELETE (bot reaction only) → POST DONE sequence and that user-owned Typing reactions are left aloneRunLlmReplyAsync_ShouldNotSwapReaction_WhenReplyFailsverifies no swap fires on reply failurebash tools/ci/test_stability_guards.sh— clean (TCS-based deterministic wait, no polling)🤖 Generated with Claude Code