feat(agent): implement live mode with LLM tool execution#126
feat(agent): implement live mode with LLM tool execution#126KhairallahA wants to merge 15 commits intokeep-starknet-strange:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds live streaming agent support with tool-call parsing/execution: SSE parser emits tool_call chunks, runtime types and provider accept tools, a new live chat hook orchestrates streaming and tool execution, demo state and UI updated to render streaming, tool-call cards, and live-mode errors. Changes
Sequence DiagramsequenceDiagram
participant UI as Agent UI
participant Hook as useAgentChatLive
participant Provider as OpenAI Provider
participant Tools as Tool Executor
participant State as Chat State
UI->>Hook: sendMessage(userMessage)
activate Hook
Hook->>State: set isResponding, append user message
Hook->>Provider: stream(initialMessages, tools)
activate Provider
Provider-->>Hook: tool_call (id,name,args)
Hook->>State: add toolCall entry
Hook->>Tools: executeToolCall(toolCall)
activate Tools
Tools-->>Hook: toolResult
deactivate Tools
Hook->>State: append tool result as message
Provider-->>Hook: text delta
Hook->>State: update assistant message (isStreaming=true)
Provider-->>Hook: done
deactivate Provider
Hook->>Provider: stream(messagesWithToolResults, tools)
activate Provider
Provider-->>Hook: final response deltas
Provider-->>Hook: done
deactivate Provider
Hook->>State: set isResponding(false)
deactivate Hook
UI->>State: read messages, toolCalls, error
UI->>UI: render MessageBubble(isStreaming), ToolCallCard, error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds live LLM chat with tool execution to the Agent tab, introducing However, the tool-call flow is non-functional due to several compounding bugs:
Confidence Score: 1/5
|
| Filename | Overview |
|---|---|
| apps/mobile/lib/agent-runtime/openai-adapter.ts | Critical: tools never included in API request body (dead code); tool message mapping strips required tool_call_id; streaming tool call argument accumulation is incomplete — only the first chunk is parsed. |
| apps/mobile/lib/agent/use-agent-chat.ts | User messages duplicated on every streaming delta due to slice-and-append logic; stale closure captures state.messages for LLM context; unused toolCallBuffer variable. |
| apps/mobile/lib/agent-runtime/types.ts | Clean type additions: tool role, ParsedToolCall, toolCallId on ChatMessage, and tools on StreamOptions. No issues found. |
| apps/mobile/app/(tabs)/agent.tsx | UI integration is mostly clean — hooks called unconditionally, mode switching works. Minor: (m as any).isStreaming bypasses types; quick prompts still use actions.sendAgentMessage instead of chatActions.sendMessage even in live mode. |
Sequence Diagram
sequenceDiagram
participant User
participant AgentUI as Agent Tab UI
participant Hook as useAgentChatLive
participant Adapter as OpenAI Adapter
participant API as OpenAI API
participant Registry as Tool Registry
User->>AgentUI: Type message & send
AgentUI->>Hook: sendMessage(text)
Hook->>Hook: Add user msg to state
Hook->>Adapter: streamChat(messages, tools)
Note over Adapter: BUG: tools not included in body
Adapter->>API: POST /chat/completions (SSE)
API-->>Adapter: delta chunks (text)
Adapter-->>Hook: StreamChunk{type: "delta"}
Hook->>Hook: Update assistant message
Note over Hook: BUG: user msg duplicated each delta
API-->>Adapter: tool_call chunks (streamed)
Note over Adapter: BUG: only first chunk parsed
Adapter-->>Hook: StreamChunk{type: "tool_call"}
Hook->>Registry: executeTool(name, args)
Registry-->>Hook: ToolResult
Hook->>Adapter: streamChat(msgs + tool results)
Note over Adapter: BUG: tool_call_id stripped
Adapter->>API: POST /chat/completions
API-->>Adapter: delta chunks (final answer)
Adapter-->>Hook: StreamChunk{type: "delta"}
Hook->>AgentUI: Update state (messages, toolCalls)
AgentUI->>User: Render conversation + ToolCallCards
Last reviewed commit: f2b15b8
Additional Comments (2)
The
This mapping strips |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/mobile/lib/agent-runtime/openai-adapter.ts (1)
192-198:⚠️ Potential issue | 🔴 CriticalTools are not passed to OpenAI API — tool calling will not work.
The
opts.toolsfield is defined inStreamOptionsbut never included in the request body. OpenAI requires thetoolsparameter for function calling to work.🐛 Proposed fix to include tools in request body
const body: Record<string, unknown> = { model, messages, stream: true, }; if (opts.maxTokens != null) body.max_tokens = opts.maxTokens; if (opts.temperature != null) body.temperature = opts.temperature; +if (opts.tools && opts.tools.length > 0) body.tools = opts.tools;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/lib/agent-runtime/openai-adapter.ts` around lines 192 - 198, The request body built in openai-adapter.ts for the streaming call omits the tools parameter, so function/tool calling won't work; update the code that populates the body (the const body: Record<string, unknown> = {...} block) to include opts.tools when present (e.g., if (opts.tools != null) body.tools = opts.tools) and ensure the shape matches StreamOptions.tools so the OpenAI API receives the tools array for function calling; reference the variables body and opts.tools in your change and keep the existing conditional pattern used for max_tokens and temperature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/mobile/app/`(tabs)/agent.tsx:
- Around line 317-319: The code casts (m as any).isStreaming when rendering
MessageBubble because state.agent.messages lacks isStreaming while
chatState.messages (live) includes it; replace the unsafe cast by unifying or
narrowing the message type: update the shared message type (e.g., make Message
have optional isStreaming?: boolean) or create a discriminated union/ type guard
(e.g., isLiveMessage(m): m is LiveChatMessage) and use it in the map so you can
pass m.isStreaming safely to MessageBubble; update the definitions used by
state.agent.messages and chatState.messages or add a type guard function and use
it in the map where isLive is checked to ensure type-safe access to isStreaming.
In `@apps/mobile/lib/agent-runtime/openai-adapter.ts`:
- Around line 46-64: The loop over toolCalls currently returns on the first
match which drops subsequent tool calls; update the logic in the function that
processes toolCalls to produce all tool_call chunks instead of returning
immediately: iterate over toolCalls, for each tc with tc?.id and
tc?.function?.name parse tc.function.arguments with a try/catch (defaulting to
{} on parse error) and push a tool_call object ({ type: "tool_call", toolCall: {
id: tc.id, name: tc.function.name, arguments: args } }) into a results array,
then return that array (or change the function signature to stream/emit each
chunk individually) so every tool call in the same SSE event is handled rather
than only the first.
- Around line 48-53: The streaming handler currently JSON.parse's each
tc.function.arguments fragment into args (letting failures fall back to {}),
which breaks tool execution for incremental fragments; instead accumulate
fragments per tool_calls[index] (use the existing toolCallBuffer from
use-agent-chat.ts keyed by tool call index), concatenate incoming
tc.function.arguments pieces, attempt JSON.parse only when the combined string
forms valid JSON (or when tc.finish_reason indicates completion), and emit the
tool call with the parsed args only once the arguments are complete; update the
code paths that reference args, tc.function.arguments, and finish_reason so
parsing/validation happens after accumulation rather than per-chunk.
In `@apps/mobile/lib/agent-runtime/types.ts`:
- Around line 44-45: Replace the weak tools?: unknown[] with a concrete shared
type by extracting a ToolDefinition (or similar) interface that matches the tool
schema used in use-agent-chat.ts (the tool schema defined in use-agent-chat.ts
lines ~171-232); then import or declare that ToolDefinition in
apps/mobile/lib/agent-runtime/types.ts and change the property to tools?:
ToolDefinition[] (or readonly ToolDefinition[] | undefined) so callers get
proper typing for tool name, inputs, and metadata.
In `@apps/mobile/lib/agent/use-agent-chat.ts`:
- Around line 267-274: The toolCallEntry creation hardcodes operationType to
"write", causing read-only tools like get_balances to be misclassified; update
the logic around the ToolCall construction (toolCallEntry) to derive
operationType from the tool metadata or name (e.g., inspect tc.name or a
tc.metadata/flags field if available) and set operationType to "read" for
read-only tools (like "get_balances") and "write" otherwise, ensuring existing
fields id, toolName, params, timestamp, status remain unchanged; if a canonical
source of operation type exists (ToolCall type or tc metadata), use that instead
of name-based heuristics.
- Around line 247-260: The streaming update logic currently slices off the last
message and re-inserts userMsg which causes duplicate user messages; instead,
generate and store the assistant message id outside of setState (e.g., const
assistantId = `msg-${Date.now()}-assistant`) then in setState update only the
existing assistant message by mapping s.messages and replacing the message with
id === assistantId (updating text, isStreaming, createdAt) rather than slicing
and re-pushing userMsg; apply this same pattern to both streaming loops (the
first setState that builds messages with userMsg/fullText and the second
streaming loop that performs similar slicing) so updates modify the assistant
message in-place without duplicating userMsg.
---
Outside diff comments:
In `@apps/mobile/lib/agent-runtime/openai-adapter.ts`:
- Around line 192-198: The request body built in openai-adapter.ts for the
streaming call omits the tools parameter, so function/tool calling won't work;
update the code that populates the body (the const body: Record<string, unknown>
= {...} block) to include opts.tools when present (e.g., if (opts.tools != null)
body.tools = opts.tools) and ensure the shape matches StreamOptions.tools so the
OpenAI API receives the tools array for function calling; reference the
variables body and opts.tools in your change and keep the existing conditional
pattern used for max_tokens and temperature.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/mobile/app/(tabs)/agent.tsx (1)
392-398: 🧹 Nitpick | 🔵 TrivialConsider adding error handling for async send operations.
chatActions.sendMessage(text)is awaited but has no try/catch. If the LLM request fails, the error would be unhandled at this level. WhilechatState.errormay capture it internally, a local catch could provide haptic feedback on failure for better UX consistency (similar to the approval flow above).♻️ Optional: Add error handling with haptic feedback
} else if (isLive) { // Live mode: send to LLM chat - await chatActions.sendMessage(text); + try { + await chatActions.sendMessage(text); + } catch { + await haptic("warn"); + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/`(tabs)/agent.tsx around lines 392 - 398, The live-path awaits chatActions.sendMessage(text) without local error handling; wrap that await in a try/catch inside the isLive branch (where chatActions.sendMessage is called) and on catch trigger the same haptic/error UI behavior used in the approval flow (e.g., call the existing haptic/error helper or emit a failure feedback and set a local error state) so failures are surfaced to the user while non-live paths continue to use actions.sendAgentMessage(text).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/mobile/app/`(tabs)/agent.tsx:
- Around line 429-436: The UI is directly rendering
JSON.stringify(toolCall.params) and toolCall.result which may leak sensitive
wallet data; update the rendering in the component that shows toolCall (the
Muted lines referencing toolCall.params and toolCall.result) to call a sanitizer
before displaying: implement a helper like sanitizeToolCall or
redactSensitiveFields that accepts toolCall.params/result, removes or masks keys
such as private keys, addresses, accountIds, balances, txHashes, or any
configured blacklist (or alternatively whitelist safe keys), and returns a
truncated string for display; replace direct JSON.stringify/String usage with
the sanitized, truncated output to ensure secrets are filtered before rendering.
- Around line 321-327: The catch-all error handler currently puts raw exception
text into the UI via setState (setting isResponding and error), which can leak
secrets; replace that assignment so the UI error string is a generic,
non-sensitive message (e.g., "An unexpected error occurred. Please try again.")
while still marking isResponding false, and send the raw err to a secure logger
(or console/error reporting) instead of the UI. Update the catch block that
calls setState(...) (the same state that feeds chatState.error) so only the
sanitized message is stored and rendered, and ensure the original err is logged
only to an internal logger/console.error for troubleshooting.
---
Outside diff comments:
In `@apps/mobile/app/`(tabs)/agent.tsx:
- Around line 392-398: The live-path awaits chatActions.sendMessage(text)
without local error handling; wrap that await in a try/catch inside the isLive
branch (where chatActions.sendMessage is called) and on catch trigger the same
haptic/error UI behavior used in the approval flow (e.g., call the existing
haptic/error helper or emit a failure feedback and set a local error state) so
failures are surfaced to the user while non-live paths continue to use
actions.sendAgentMessage(text).
---
Duplicate comments:
In `@apps/mobile/app/`(tabs)/agent.tsx:
- Around line 317-319: The map currently passes m.isStreaming to <MessageBubble>
but DemoAgentMessage doesn't define isStreaming; add a type guard when rendering
messages in the isLive ? chatState.messages : state.agent.messages mapping to
check whether the message object has the isStreaming property (e.g., typeof (m
as any).isStreaming !== "undefined" or a user-defined isStreamingMessage(m)
predicate) and only pass isStreaming to MessageBubble when present, otherwise
pass false/omit the prop, updating the mapping around the MessageBubble
instantiation to use that guard so TypeScript no longer errors.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/mobile/app/`(tabs)/agent.tsx:
- Around line 39-44: The current SENSITIVE_KEYS array wrongly treats
wallet-visible fields ("address", "addresses", "balance", "balances") as
secrets; update the constants to separate true secrets from user-visible wallet
data by creating a new SENSITIVE_SECRET_KEYS list that contains only
privateKey/private_key/secret/apiKey/api_key/key/secretKey/mnemonic/seed/password/token
etc., and remove "address"/"addresses"/"balance"/"balances" from that list; then
update any redaction logic (where SENSITIVE_KEYS is referenced—e.g., redaction
utility or the code path that redacts tool call results like get_balances) to
accept a context flag (e.g., isWalletContext or redactSecretsOnly) and use
SENSITIVE_SECRET_KEYS when in wallet UI so balances and addresses remain visible
while still redacting true secrets.
- Around line 89-92: The current hasIsStreaming implementation is too
permissive; change it into a proper type guard by updating its signature to
return a type predicate (e.g., m is { isStreaming: boolean }) and accept a
broader input type (like unknown) so it can safely narrow arbitrary values;
inside hasIsStreaming, check that m is an object (not null) and that typeof (m
as any).isStreaming === "boolean" before returning true, referencing the
hasIsStreaming function to locate and replace the existing implementation.
In `@apps/mobile/lib/agent/use-agent-chat.ts`:
- Around line 325-336: The variable assistantMsg declared as type LlmMessage
shadows an earlier assistantMsg of type ChatMessage (declared around the prior
block), reducing clarity; rename the LlmMessage variable (and any local uses) to
a distinct name such as toolContextMsg (or toolAssistantMsg) where it's created
and where it's consumed, and update related references like toolResultMessages
mapping and any code that pushes or reads this message so the original
ChatMessage assistantMsg remains unshadowed.
- Around line 177-184: The helper callLlm currently accepts a tools parameter
but ignores it — it always passes toolDefs into provider.streamChat; update
callLlm to use the provided tools when present (e.g., pass tools ?? toolDefs or
tools.length ? tools : toolDefs) so provider.streamChat receives the
caller-supplied tool list, or if callers never need tools remove the unused
tools parameter from callLlm and all its call sites; references: callLlm,
provider.streamChat, tools, toolDefs.
- Around line 254-256: The variable toolCallBuffer is declared but never used;
remove the unused declaration to clean up dead code in use-agent-chat.ts where
fullText, currentToolCalls (type ParsedToolCall[]) and toolCallBuffer are
declared, or if it was meant to accumulate tool-call args implement its usage by
piping token/argument text into toolCallBuffer and flushing into
currentToolCalls where the adapter/argument accumulation logic is handled;
either delete the toolCallBuffer declaration or wire it into the existing
parsing flow (functions/methods that build tool calls and finalize
ParsedToolCall entries) so it is referenced.
---
Duplicate comments:
In `@apps/mobile/lib/agent-runtime/openai-adapter.ts`:
- Around line 48-87: The current loop over toolCalls emits a tool_call for each
chunk even when tc.function.arguments are fragmented; change to accumulate
argument fragments per tool call (use a Map keyed by tc.id or the tool_calls
index) by appending tc.function.arguments fragments into a buffer for that id,
attempt JSON.parse only when the buffer parses successfully, and only push a
single results entry for that id (or mark it emitted) once parse succeeds or
when finish_reason indicates the stream is complete; update the logic around
toolCalls, tc.function.arguments, results and use finish_reason to
force-finalize parsing and emit or surface a parse error instead of emitting
empty args.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/mobile/app/(tabs)/agent.tsx (1)
453-470:⚠️ Potential issue | 🟠 MajorAvoid logging raw chat errors in UI flow.
console.error("Chat send error:", err)can surface sensitive data in logs. Log only a sanitized message (or route to a secure logger).🔒 Proposed fix
- } catch (err) { - await haptic("error"); - console.error("Chat send error:", err); - } + } catch (err) { + await haptic("error"); + console.error("Chat send error"); + }As per coding guidelines, "Keep secrets out of git, logs, crash output, and LLM prompts".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile/app/`(tabs)/agent.tsx around lines 453 - 470, Replace the raw console.error call in the onPress handler that wraps chatActions.sendMessage (inside the isLive branch) with a sanitized logging approach: remove or avoid printing the full err object (do not interpolate err), instead call a secure logger or processLogger with a minimal message like "Chat send failed" and optional non-sensitive context (e.g., user action, request id) or generate an error id to surface, while keeping the haptic("error") and the existing error-handling flow around chatActions.sendMessage so sensitive data from err is not written to stdout/stderr; locate this in the onPress async handler where chatActions.sendMessage is invoked and the current console.error("Chat send error:", err) appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/mobile/app/`(tabs)/agent.tsx:
- Around line 39-95: The sanitizeForDisplay function fails to redact camelCase
keys because SENSITIVE_KEYS/SENSITIVE_SECRET_KEYS contain mixed-case entries and
you compare lowerKey to them; normalize comparisons by lowercasing the
sensitive-key list (e.g., map SENSITIVE_SECRET_KEYS and SENSITIVE_KEYS to
lowercase when building keysToCheck) or lowercase each sk in the .some(...)
check so lowerKey === sk and lowerKey.endsWith(sk)/lowerKey.endsWith(`_${sk}`)
are comparing lowercased strings; update the keysToCheck creation and/or the
isSensitive check in sanitizeForDisplay accordingly to ensure camelCase secrets
like "privateKey" and "apiKey" are redacted.
In `@apps/mobile/lib/agent-runtime/openai-adapter.ts`:
- Around line 126-128: The code currently declares toolCallBuffers twice (an
outer and an inner shadowing declaration), which confuses state and makes the
finally cleanup incorrect; remove the inner declaration so there is a single
per-stream Map<string,string> used throughout (the one originally created as
"toolCallBuffers"), update any references inside the streaming handler to use
that single Map, and ensure the finally block clears or deletes entries from
that single toolCallBuffers instance instead of attempting to clean up a
shadowed variable; look for references to "toolCallBuffers" and the streaming
handler/finally block to apply this change.
- Around line 66-83: The emission gate currently checks Object.keys(args).length
> 0 which skips valid empty-argument tool calls; update the logic around the
parsing block in openai-adapter.ts (the buffer/args/toolCallBuffers parsing for
tcId) to track successful JSON parse (e.g., set a parsed flag when
JSON.parse(buffer) runs and toolCallBuffers.delete(tcId) is called) and change
the emit condition from Object.keys(args).length > 0 || !buffer to use that
parsed flag (e.g., parsed || !buffer) so we emit even when args is an empty
object.
In `@apps/mobile/lib/agent/use-agent-chat.ts`:
- Around line 385-392: In the catch block inside useAgentChat (where setState is
called on error), remove the raw console.error(err) and instead log only a
sanitized message (e.g., processLogger?.warn or console.warn with no sensitive
payload) and update state to clear all streaming flags: set isResponding: false
and isStreaming: false on the root state and also map over state.messages (or
the messages property) to set message.isStreaming = false for any message
currently streaming; keep the user-facing generic error text unchanged.
---
Outside diff comments:
In `@apps/mobile/app/`(tabs)/agent.tsx:
- Around line 453-470: Replace the raw console.error call in the onPress handler
that wraps chatActions.sendMessage (inside the isLive branch) with a sanitized
logging approach: remove or avoid printing the full err object (do not
interpolate err), instead call a secure logger or processLogger with a minimal
message like "Chat send failed" and optional non-sensitive context (e.g., user
action, request id) or generate an error id to surface, while keeping the
haptic("error") and the existing error-handling flow around
chatActions.sendMessage so sensitive data from err is not written to
stdout/stderr; locate this in the onPress async handler where
chatActions.sendMessage is invoked and the current console.error("Chat send
error:", err) appears.
---
Duplicate comments:
In `@apps/mobile/lib/agent-runtime/openai-adapter.ts`:
- Around line 44-65: The current loop over toolCalls drops fragments when later
deltas omit tc.id or tc.function.name; modify the accumulation logic in the
block handling toolCalls so you key buffers by the tool_calls array index (e.g.,
use the loop index) rather than requiring tc.id, and when you first encounter a
fragment record the tc.id and tc.function.name into a stored metadata object
(alongside the buffer) and thereafter always append tc.function.arguments
(newArgsFragment) to the buffer even if id/name are missing; update usages of
toolCallBuffers and the local variables tcId/funcName/newArgsFragment to
read/write from that index-keyed entry and preserve the original id/name when
they first appear.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/mobile/lib/agent-runtime/openai-adapter.ts`:
- Around line 81-91: The tool-call emission logic skips calls when args is {}
even if parsing succeeded because the local buffer still contains the raw
string; introduce a parseSucceeded flag set to true when JSON.parse of buffer
succeeds (in the same scope where args is replaced and the bufferMap entry is
deleted), and change the emission predicate in the block that builds the
tool_call (the check around Object.keys(args).length > 0 || !buffer) to also
emit when parseSucceeded is true (i.e., Object.keys(args).length > 0 || !buffer
|| parseSucceeded). This ensures calls like get_balances with empty {} args are
emitted while preserving existing behavior for non-parsed buffers; update
references to args, buffer, tcId, and funcName accordingly.
Calcutatator
left a comment
Calcutatator
left a comment
There was a problem hiding this comment.
Good architectural direction — the dual-hook pattern (demo + live), SSE streaming parser, and ToolCallCard component are well-conceived. However there are several critical issues that need fixing before merge:
Critical (must fix):
-
sanitizeForDisplaysecret redaction is broken. TheSENSITIVE_SECRET_KEYSarray uses camelCase ("privateKey","apiKey") but the comparison lowercases the key first (lowerKey === sk). Since"privatekey" !== "privateKey", secrets are never redacted. This is a real security defect in a wallet app. Fix: lowercase the entries in the list, or comparelowerKey === sk.toLowerCase(). -
OpenAI tool calling protocol is incomplete. When sending tool results back to the API, the assistant message must include the
tool_callsarray (the raw tool call objects with id/function/arguments), and each tool result message needstool_call_idserialized astool_call_idin the HTTP body. Without these, the follow-up API call will fail or produce incorrect behavior. -
Tool call argument streaming drops subsequent chunks. The SSE parser guard
if (tc?.id && tc?.function?.name)only matches the first chunk per tool call — OpenAI sendsidandfunction.nameonly on the first chunk, with subsequent chunks carrying just argument fragments keyed by index. Arguments will be truncated for any non-trivial tool call. -
No programmatic confirmation gate for
execute_transfer. The system prompt says "always confirm before executing transfers" but this is a soft LLM instruction, not an enforced constraint. A prompt injection via crafted input could bypass this. There should be a hard gate in code — e.g., require explicit user approval in the UI beforeexecuteTool("execute_transfer", ...)runs.
Major (should fix):
-
Stale closure on
state.messagesin thesendMessagecallback.useCallbackcapturesstate.messagesfrom the closure, butsetStatehas already been called. After multiple exchanges, the conversation history sent to the LLM will be incomplete. Use auseReffor messages or derive from functional setState. -
No wallet context passed to tool execution.
executeTool(tc.name, tc.arguments)doesn't include which account to operate on. Tools may not know which wallet to use. -
Raw error objects logged via
console.errorcould contain API keys in network error payloads. Use sanitized logging.
Minor:
- JSON completeness heuristic (
startsWith("{") && endsWith("}")) is fragile —{"a": "{b}"}would false-match mid-accumulation. Prefer try/catchJSON.parse. - Model is hardcoded to
"gpt-4o-mini"— should respect user's configured model. - No activity logging for tool executions — at minimum
execute_transferresults should be logged. - Commits should be squashed before merge ("update 1" through "update 5").
|
@coderabbitai review |
|
@greptile-ai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
|
@greptile-ai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/mobile/app/`(tabs)/agent.tsx:
- Around line 463-470: The code in the isLive branch catches errors from
chatActions.sendMessage and currently logs the raw err, which may expose
secrets; change the catch to log only a safe identifier (e.g., err.name or a
sanitized message) and keep the haptic("error") call; locate the try/catch
around chatActions.sendMessage in the isLive branch of the component
(references: isLive, chatActions.sendMessage, haptic) and replace
console.error("Chat send error:", err) with a console.error call that emits a
non-sensitive value such as err.name or a fixed message combined with err.name.
In `@apps/mobile/lib/agent/use-agent-chat.ts`:
- Around line 214-221: The helper callLlm references the toolDefs variable
declared later, which is confusing; move the toolDefs declaration so it appears
before the callLlm function. Update the file so toolDefs is defined first, then
define callLlm to use toolDefs (keep the same signature and behavior for callLlm
and preserve the fallback logic tools && tools.length > 0 ? tools : toolDefs).
This reordering improves readability and prevents future refactor bugs related
to declaration order.
- Around line 376-391: The assistant message must include the original
tool_calls metadata and the ChatMessage type must support it: add an optional
toolCalls field to the ChatMessage/LlmMessage type in types.ts that matches
OpenAI's schema (array of {id, type, function: {name}, arguments}), then when
building toolContextMsg in use-agent-chat.ts populate toolContextMsg.toolCalls
from currentToolCalls (construct id, type, function.name and arguments) so the
assistant message reflects the executed calls, and finally update the OpenAI
adapter's request serializer to include tool_calls from ChatMessage/toolCalls
into the outgoing request body so callLlm(updatedMessages, ...) sends the
tool_calls array.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/mobile/app/(tabs)/agent.tsxapps/mobile/lib/agent-runtime/openai-adapter.tsapps/mobile/lib/agent/use-agent-chat.ts
|
@coderabbitai review |
|
@greptile-ai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
|
@greptile-ai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
|
@greptile-ai review |
✅ Actions performedReview triggered.
|
Additional Comments (2)
This creates poor UX: the LLM will likely apologize and tell the user to use the Transfer tab, but the Transfer tab integration isn't wired (per CLAUDE.md line 6: "Live Starknet execution libs exist but aren't wired to UI yet"). The agent can't actually help with transfers. Either remove Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/mobile/lib/agent/use-agent-chat.ts
Line: 862-876
Comment:
Authorization check blocks `execute_transfer` but still returns blocked message to LLM and updates UI. The system prompt (line 547) tells the LLM that `execute_transfer` requires manual approval in the Transfer tab, but when the LLM tries to execute it, the tool returns "Blocked: this tool requires manual approval" as an error.
This creates poor UX: the LLM will likely apologize and tell the user to use the Transfer tab, but the Transfer tab integration isn't wired (per CLAUDE.md line 6: "Live Starknet execution libs exist but aren't wired to UI yet"). The agent can't actually help with transfers.
Either remove `execute_transfer` from the tool definitions entirely, or implement proper approval flow integration with the Transfer tab.
How can I resolve this? If you propose a fix, please make it concise.
Add wallet context to tool execution or validate network consistency. Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/mobile/lib/agent/use-agent-chat.ts
Line: 878
Comment:
`executeTool` executes without wallet context validation. The tool registry (core-tools.ts:250-329) calls `executeTransfer` which moves real funds using session keys, but `executeTool` at line 878 has no awareness of:
- Which wallet is active
- Whether the wallet has sufficient balance
- Whether session keys are initialized
- Network mismatch between tool params and wallet
`get_balances` tool requires `network` param (core-tools.ts:74-80) but `execute_transfer` and other tools don't validate that the provided network matches the active wallet's network before execution.
Add wallet context to tool execution or validate network consistency.
How can I resolve this? If you propose a fix, please make it concise. |
|
@greptile-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@greptile-ai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!AUTO_EXECUTABLE_TOOLS.has(tc.name)) { | ||
| const blockedResult = | ||
| "Blocked: this tool requires manual approval. Use the Transfer tab to run write operations."; | ||
| toolResults.push({ | ||
| tool_call_id: tc.id, | ||
| content: blockedResult, | ||
| }); | ||
| setState((s) => ({ | ||
| ...s, | ||
| toolCalls: s.toolCalls.map((t) => | ||
| t.id === tc.id ? { ...t, result: blockedResult, status: "error" } : t | ||
| ), | ||
| })); | ||
| continue; |
There was a problem hiding this comment.
prepare_transfer requires manual approval but should be auto-executable. It's a read-only validation operation that doesn't execute transfers - blocking it cripples the agent's ability to help users plan transactions.
Per CLAUDE.md:54: "Don't let the model construct raw calldata unsafely" - prepare_transfer doesn't construct calldata for execution, it only validates parameters. The actual execution in execute_transfer is correctly blocked.
Only execute_transfer needs manual approval. Move prepare_transfer to AUTO_EXECUTABLE_TOOLS:
| if (!AUTO_EXECUTABLE_TOOLS.has(tc.name)) { | |
| const blockedResult = | |
| "Blocked: this tool requires manual approval. Use the Transfer tab to run write operations."; | |
| toolResults.push({ | |
| tool_call_id: tc.id, | |
| content: blockedResult, | |
| }); | |
| setState((s) => ({ | |
| ...s, | |
| toolCalls: s.toolCalls.map((t) => | |
| t.id === tc.id ? { ...t, result: blockedResult, status: "error" } : t | |
| ), | |
| })); | |
| continue; | |
| for (const tc of currentToolCalls) { | |
| if (!AUTO_EXECUTABLE_TOOLS.has(tc.name)) { |
And update line 68:
const AUTO_EXECUTABLE_TOOLS = new Set(["get_balances", "estimate_fee", "prepare_transfer"]);Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/mobile/lib/agent/use-agent-chat.ts
Line: 405-418
Comment:
`prepare_transfer` requires manual approval but should be auto-executable. It's a read-only validation operation that doesn't execute transfers - blocking it cripples the agent's ability to help users plan transactions.
Per CLAUDE.md:54: "Don't let the model construct raw calldata unsafely" - `prepare_transfer` doesn't construct calldata for execution, it only validates parameters. The actual execution in `execute_transfer` is correctly blocked.
Only `execute_transfer` needs manual approval. Move `prepare_transfer` to `AUTO_EXECUTABLE_TOOLS`:
```suggestion
for (const tc of currentToolCalls) {
if (!AUTO_EXECUTABLE_TOOLS.has(tc.name)) {
```
And update line 68:
```typescript
const AUTO_EXECUTABLE_TOOLS = new Set(["get_balances", "estimate_fee", "prepare_transfer"]);
```
How can I resolve this? If you propose a fix, please make it concise.3 participants
feat(agent): implement live mode with LLM tool execution
tool_callsfrom OpenAI SSE streaming responseget_balances,prepare_transfer,estimate_fee,execute_transfer) and send results back to LLMToolCallCardcomponent for displaying tool execution statusTypeScript types updated:
tool_callchunk type and tool role toChatMessageStreamOptionsSummary by CodeRabbit
New Features
Improvements
Greptile Summary
This PR implements live mode LLM streaming with tool execution for the agent chat interface. The implementation correctly addresses most security concerns from previous reviews:
What Changed:
useAgentChatLivehook with real OpenAI streaming and tool executionprepare_transferandexecute_transferKey Security Controls:
get_balancesandestimate_feeare auto-executableexecute_transfer) correctly blocked with error messageIssues Found:
prepare_transferis classified as requiring manual approval, but it's a read-only validation operation that doesn't execute transfers. This makes the agent unable to help users plan transactions effectively, severely limiting live mode UX compared to demo mode.Architecture Quality:
The streaming implementation is well-architected with proper buffering, clean state management, and correct message flow. Tool call arguments are accumulated across SSE chunks and flushed at terminal events. The dual LLM call pattern (initial request → tool execution → follow-up with results) is correctly implemented.
Confidence Score: 4/5
prepare_transferis overly restricted - it's a read-only validation tool that should be auto-executable for agent usability, but blocking it doesn't create a security risk, only a UX limitation.prepare_transfershould be unblocked for better agent UXImportant Files Changed
prepare_transfershould be auto-executable for UXSequence Diagram
sequenceDiagram participant User participant AgentUI as Agent UI participant LiveChat as useAgentChatLive participant OpenAI as OpenAI Adapter participant LLM as OpenAI API participant Registry as Tool Registry participant Tools as Core Tools User->>AgentUI: Send message AgentUI->>LiveChat: sendMessage(text) LiveChat->>LiveChat: Add user message to state LiveChat->>OpenAI: streamChat(messages, tools) OpenAI->>LLM: POST /chat/completions (SSE) loop Streaming Response LLM-->>OpenAI: SSE chunk (delta/tool_call) OpenAI->>OpenAI: Buffer tool call arguments OpenAI-->>LiveChat: StreamChunk alt Text Delta LiveChat->>LiveChat: Append to assistant message LiveChat->>AgentUI: Update UI (streaming) else Tool Call LiveChat->>LiveChat: Add to currentToolCalls[] end end LLM-->>OpenAI: [DONE] OpenAI->>OpenAI: Flush pending tool calls OpenAI-->>LiveChat: done chunk alt Tool Calls Present loop For each tool call LiveChat->>LiveChat: Check AUTO_EXECUTABLE_TOOLS alt Auto-executable LiveChat->>Registry: executeTool(name, args) Registry->>Registry: Validate args schema Registry->>Tools: handler(args) Tools-->>Registry: ToolResult Registry->>Registry: Append audit log Registry-->>LiveChat: ToolResult else Blocked LiveChat->>LiveChat: Return "Blocked" message end LiveChat->>AgentUI: Update tool call status end LiveChat->>LiveChat: Build LLM messages with tool results LiveChat->>OpenAI: streamChat(messages + tool results) OpenAI->>LLM: POST /chat/completions (SSE) loop Final Response LLM-->>OpenAI: SSE chunk OpenAI-->>LiveChat: StreamChunk LiveChat->>AgentUI: Update final assistant message end end LiveChat->>LiveChat: Mark streaming complete LiveChat->>AgentUI: Update UI (complete)Last reviewed commit: bae8287