Conversation
Contributor
📝 WalkthroughWalkthroughImplements structured pending tool interactions for Telegram and Feishu: adds spec/docs, new pending-interaction types and parsing, runner APIs to expose/respond to pending interactions, platform prompt builders (cards/buttons and fallbacks), router/poller/binding-store changes for callback tokens and gating, and tests covering the flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router
participant Runner
participant BindingStore
participant Client as Telegram/Feishu Client
Note over Router,Runner: New pending-interaction flow
User->>Router: Message or /pending
Router->>Runner: getPendingInteraction(endpointKey)
Runner-->>Router: RemotePendingInteraction or null
alt Pending exists
Router->>BindingStore: createPendingInteractionState(endpoint, {messageId, toolCallId})
Router->>Client: send pending prompt (card/buttons + fallback)
Client->>User: Display prompt
User->>Client: Button callback or text reply
Client->>Router: Callback or message with token/text
Router->>BindingStore: getPendingInteractionState(token)
BindingStore-->>Router: state or null
alt Token valid
Router->>Runner: respondToPendingInteraction(endpointKey, parsedResponse)
Runner->>Runner: respondToolInteraction()/resume execution
Runner-->>Router: continuation execution (maybe deferred)
Router->>Client: edit/send resolved text and subsequent content
Client->>User: Show resolution and continue
else Token expired or missing
Router->>Runner: (no response) request refresh -> re-send prompt
Router->>Client: re-send refreshed prompt
end
else No pending
Router->>Runner: handle message/command normally
Runner-->>Router: execution
Router->>Client: send replies
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts (1)
163-189: Tighten thesendCardpayload contract.
sendCard()acceptsFeishuInteractiveCardPayload, but that type declares all fields optional insrc/main/presenter/remoteControlPresenter/types.ts:364-370, allowing malformed cards to pass the compiler. ThecreateCardPayload()helper (line 21) only stringifies without validation. The real builderbuildFeishuPendingInteractionCard()always emits a complete card withconfig,header, andelements, yet the test fixture demonstrates the type accepts a header-only partial. Narrow the type to match what the builder actually produces, or add runtime validation at the boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts` around lines 163 - 189, The sendCard boundary currently accepts FeishuInteractiveCardPayload (which has all-optional fields) and blindly stringifies via createCardPayload; tighten this by either (A) updating the FeishuInteractiveCardPayload type to require the fields the builder emits (config, header, elements) so callers like buildFeishuPendingInteractionCard match the compile-time contract, or (B) add a runtime guard inside sendCard that validates the payload contains non-empty config, header and elements (and throws or returns a clear error) before calling createCardPayload; update sendCard, FeishuInteractiveCardPayload, createCardPayload, and ensure tests/use sites that rely on buildFeishuPendingInteractionCard remain consistent with the new required shape.src/main/presenter/remoteControlPresenter/services/remoteBindingStore.ts (1)
450-464: Weak token generation should use cryptographic randomness for improved defense-in-depth.Token generation via
createTelegramCallbackToken()combinesDate.now().toString(36)with only 6 random characters fromMath.random().toString(36), creating ~2.1 billion possible suffixes. While the practical exploit risk is limited by multi-factor validation (tokens must match endpoint key, message ID, and tool call ID), short TTL (10 minutes), and in-memory storage, usingcrypto.randomUUID()would provide stronger entropy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/services/remoteBindingStore.ts` around lines 450 - 464, createPendingInteractionState uses a weak createTelegramCallbackToken() relying on Math.random; replace the token with a cryptographically strong value (e.g., crypto.randomUUID() or hex from crypto.randomBytes) to improve entropy. Update the implementation that currently produces tokens (or stop calling createTelegramCallbackToken() from createPendingInteractionState) so the stored token is generated via Node's crypto API, and ensure any callers/validators (lookup code that compares tokens) continue to accept the new format (UUID/hex) in RemoteBindingStore methods such as createPendingInteractionState and any token validation/lookup logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/remote-tool-interactions/spec.md`:
- Around line 1-47: Create two companion artifacts alongside spec.md: a plan.md
and tasks.md under the same docs/specs/remote-tool-interactions/ directory. In
plan.md outline goals, rollout steps, dependencies, migration/compatibility
notes referencing RemoteConversationSnapshot.pendingInteraction, Telegram and
Feishu rendering behaviors, expired callback token refresh, and the blocked
commands list (/new, /use, /model) plus allowed commands (/help, /status, /open,
/pending). In tasks.md enumerate actionable tickets: design API changes,
Electron main integration, Telegram button/callback and text-reply handling,
Feishu card fallback, token refresh logic, /pending re-send, tests, and docs,
each with owner, estimate, and acceptance criteria tied to the spec’s acceptance
criteria. Ensure both files resolve any [NEEDS CLARIFICATION] items before
marking ready.
In `@src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts`:
- Around line 339-349: The current selection logic in the handler (uses
Number.parseInt(normalized, 10) and variable optionIndex) accepts inputs like "2
please" or "2." as valid—change it to require an exact numeric reply: after
computing optionIndex from normalized, verify that normalized trimmed exactly
matches optionIndex.toString() (or use a /^\d+$/ check) before returning the {
kind: 'question_option', optionLabel: question.options[optionIndex - 1].label }
result; otherwise fall through to the non-numeric/custom input handling. Ensure
this check is applied only when question.multiple is false.
- Around line 299-309: The current buildPendingPromptResult returns a card with
fallbackText set to formatPendingTextReplyHint(interaction) which omits the full
pending prompt; change fallbackText to use the full plain-text builder
buildFeishuPendingInteractionText(interaction) so card failures preserve the
user's permission/question details, and add the corresponding import for
buildFeishuPendingInteractionText alongside buildFeishuPendingInteractionCard;
update references in buildPendingPromptResult (FeishuCommandRouteResult,
RemotePendingInteraction) accordingly.
In `@src/main/presenter/remoteControlPresenter/telegram/telegramPoller.ts`:
- Around line 226-229: The poll loop is awaiting routed.deferred inside
handleRawUpdate(), which blocks later updates and can hang stop(); instead, do
not await routed.deferred on-loop—dispatch the continuation off the loop by
launching dispatchRouteResult(target, deferred) as a detached background task
(e.g., spawn via Promise.resolve().then(...) or setImmediate) and ensure errors
are caught inside that task; additionally, if graceful shutdown is needed, race
those detached tasks against loopPromise/stop() using a tracked task list or a
cancellable/race wrapper so stop() can settle without waiting indefinitely for
routed.deferred.
---
Nitpick comments:
In `@src/main/presenter/remoteControlPresenter/feishu/feishuClient.ts`:
- Around line 163-189: The sendCard boundary currently accepts
FeishuInteractiveCardPayload (which has all-optional fields) and blindly
stringifies via createCardPayload; tighten this by either (A) updating the
FeishuInteractiveCardPayload type to require the fields the builder emits
(config, header, elements) so callers like buildFeishuPendingInteractionCard
match the compile-time contract, or (B) add a runtime guard inside sendCard that
validates the payload contains non-empty config, header and elements (and throws
or returns a clear error) before calling createCardPayload; update sendCard,
FeishuInteractiveCardPayload, createCardPayload, and ensure tests/use sites that
rely on buildFeishuPendingInteractionCard remain consistent with the new
required shape.
In `@src/main/presenter/remoteControlPresenter/services/remoteBindingStore.ts`:
- Around line 450-464: createPendingInteractionState uses a weak
createTelegramCallbackToken() relying on Math.random; replace the token with a
cryptographically strong value (e.g., crypto.randomUUID() or hex from
crypto.randomBytes) to improve entropy. Update the implementation that currently
produces tokens (or stop calling createTelegramCallbackToken() from
createPendingInteractionState) so the stored token is generated via Node's
crypto API, and ensure any callers/validators (lookup code that compares tokens)
continue to accept the new format (UUID/hex) in RemoteBindingStore methods such
as createPendingInteractionState and any token validation/lookup logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7391023f-d011-4663-8e55-04727cf43a92
📒 Files selected for processing (22)
docs/specs/remote-tool-interactions/plan.mddocs/specs/remote-tool-interactions/spec.mdpackage.jsonsrc/main/presenter/remoteControlPresenter/feishu/feishuClient.tssrc/main/presenter/remoteControlPresenter/feishu/feishuInteractionPrompt.tssrc/main/presenter/remoteControlPresenter/feishu/feishuRuntime.tssrc/main/presenter/remoteControlPresenter/services/feishuCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/remoteBindingStore.tssrc/main/presenter/remoteControlPresenter/services/remoteCommandRouter.tssrc/main/presenter/remoteControlPresenter/services/remoteConversationRunner.tssrc/main/presenter/remoteControlPresenter/services/remoteInteraction.tssrc/main/presenter/remoteControlPresenter/telegram/telegramInteractionPrompt.tssrc/main/presenter/remoteControlPresenter/telegram/telegramOutbound.tssrc/main/presenter/remoteControlPresenter/telegram/telegramPoller.tssrc/main/presenter/remoteControlPresenter/types.tstest/main/presenter/remoteControlPresenter/feishuCommandRouter.test.tstest/main/presenter/remoteControlPresenter/feishuRuntime.test.tstest/main/presenter/remoteControlPresenter/remoteBindingStore.test.tstest/main/presenter/remoteControlPresenter/remoteCommandRouter.test.tstest/main/presenter/remoteControlPresenter/remoteConversationRunner.test.tstest/main/presenter/remoteControlPresenter/telegramOutbound.test.tstest/main/presenter/remoteControlPresenter/telegramPoller.test.ts
src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts
Show resolved
Hide resolved
src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts (1)
365-370: Minor: Consider simplifying the boolean check.Given that
question.customis typed asboolean,question.custom !== falseis equivalent toquestion.custom. However, the current form is defensive and not incorrect.- if (question.multiple || question.custom !== false) { + if (question.multiple || question.custom) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts` around lines 365 - 370, The boolean check can be simplified: in the branch where you currently test "if (question.multiple || question.custom !== false)" replace the defensive "question.custom !== false" with "question.custom" since question.custom is typed as boolean; update the condition in the function containing this block (the branch that returns { kind: 'question_custom', answerText: normalized }) to use "question.multiple || question.custom" so the logic and returned object remain the same but the expression is clearer.docs/specs/remote-tool-interactions/plan.md (1)
68-68: Minor: Add hyphen for compound adjective."interactive-card style" should be "interactive-card-style" when used as a compound adjective modifying "outbound messages".
- - Pending interactions render as interactive-card style outbound messages when the card API succeeds. + - Pending interactions render as interactive-card-style outbound messages when the card API succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/remote-tool-interactions/plan.md` at line 68, The phrase "interactive-card style" is missing a hyphen for the compound adjective; update the sentence that currently reads "Pending interactions render as interactive-card style outbound messages when the card API succeeds." to use "interactive-card-style" so it reads "Pending interactions render as interactive-card-style outbound messages when the card API succeeds."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/specs/remote-tool-interactions/plan.md`:
- Line 68: The phrase "interactive-card style" is missing a hyphen for the
compound adjective; update the sentence that currently reads "Pending
interactions render as interactive-card style outbound messages when the card
API succeeds." to use "interactive-card-style" so it reads "Pending interactions
render as interactive-card-style outbound messages when the card API succeeds."
In `@src/main/presenter/remoteControlPresenter/services/feishuCommandRouter.ts`:
- Around line 365-370: The boolean check can be simplified: in the branch where
you currently test "if (question.multiple || question.custom !== false)" replace
the defensive "question.custom !== false" with "question.custom" since
question.custom is typed as boolean; update the condition in the function
containing this block (the branch that returns { kind: 'question_custom',
answerText: normalized }) to use "question.multiple || question.custom" so the
logic and returned object remain the same but the expression is clearer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 498653a3-6d12-4758-9c78-b895fc1eb3f6
📒 Files selected for processing (6)
docs/specs/remote-tool-interactions/plan.mddocs/specs/remote-tool-interactions/tasks.mdsrc/main/presenter/remoteControlPresenter/services/feishuCommandRouter.tssrc/main/presenter/remoteControlPresenter/telegram/telegramPoller.tstest/main/presenter/remoteControlPresenter/feishuCommandRouter.test.tstest/main/presenter/remoteControlPresenter/telegramPoller.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/specs/remote-tool-interactions/tasks.md
- test/main/presenter/remoteControlPresenter/feishuCommandRouter.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
/pendingto re-send the current prompt;/statusshows waiting state; unrelated session commands are blocked while waiting.Tests
Chores