-
Notifications
You must be signed in to change notification settings - Fork 45
Feat/tool execution approval #147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThese changes implement a tool approval workflow with configurable auto-run modes. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Chat UI
participant Chat Handler
participant Tool Context
participant Terminal Tool
participant Tool Approval UI
User->>Chat UI: Send message with terminal command
Chat UI->>Chat Handler: Route request with autoRunMode
Chat Handler->>Tool Context: Provide autoRunMode + sandboxPreference
Tool Context->>Terminal Tool: Create tool with context
Terminal Tool->>Terminal Tool: Compute needsApproval flag
alt needsApproval = true
Terminal Tool->>Chat Handler: Include needsApproval in response
Chat Handler->>Chat UI: Stream approval-requested state
Chat UI->>Tool Approval UI: Render command preview + mode selector
User->>Tool Approval UI: Enter/Click "Run" to approve
Tool Approval UI->>Chat UI: Call addToolApprovalResponse (approved)
Chat UI->>Chat Handler: Detect approval continuation
Chat Handler->>Chat Handler: Auto-continue with approved state
Chat Handler->>Terminal Tool: Re-execute command
Terminal Tool->>Chat Handler: Return command output
Chat Handler->>Chat UI: Stream approval-responded + output
Chat UI->>Tool Approval UI: Render execution status/result
else needsApproval = false
Terminal Tool->>Chat Handler: Execute immediately (no approval)
Chat Handler->>Chat UI: Stream result directly
end
Chat UI->>User: Display final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (5)
types/agent.ts (1)
46-47: Use theAutoRunModetype instead of duplicating the literal union.Line 46 duplicates the string literals instead of referencing the
AutoRunModetype defined on lines 30-33. This creates a maintenance risk if the type is updated.Proposed fix
- autoRunMode?: "ask-every-time" | "auto-run-sandbox" | "run-everything"; + autoRunMode?: AutoRunMode;lib/db/actions.ts (1)
337-360: Consider reusingisApprovalContinuationfromlib/utils/approval-detection.ts.The approval detection logic here (lines 339-349) duplicates similar checks from the new
approval-detection.tsutility. Consider extracting a shared helper to avoid drift between the two implementations.Also note: if
approvalMessage.iddoesn't match any existing message, the.map()on line 353 silently returnsexistingMessagesunchanged. Verify this is the intended behavior for new approval messages.app/components/AgentsTab.tsx (1)
251-258: Consider adding a confirmation dialog for "Run Everything" mode.The "Run Everything (Unsandboxed)" option bypasses all safety checks. Consider adding a confirmation dialog when selecting this option, similar to other dangerous operations in the app (e.g., unshare confirmations). This would prevent accidental selection of the most permissive mode.
app/contexts/GlobalState.tsx (1)
257-264: Consider validating localStorage value.The localStorage value is cast directly to
AutoRunModewithout validation. If localStorage contains an invalid value (e.g., from a previous version or tampering), it could cause unexpected behavior.🔎 Suggested validation
const [autoRunMode, setAutoRunMode] = useState<AutoRunMode>(() => { if (typeof window === "undefined") return "auto-run-sandbox"; - return ( - (localStorage.getItem("auto-run-mode") as AutoRunMode) || - "auto-run-sandbox" - ); + const stored = localStorage.getItem("auto-run-mode"); + const validModes = ["ask-every-time", "auto-run-sandbox", "run-everything"]; + return validModes.includes(stored || "") + ? (stored as AutoRunMode) + : "auto-run-sandbox"; });lib/api/chat-handler.ts (1)
248-249: ValidateautoRunModebefore casting.The
autoRunModecomes from the client request body as a string and is cast directly toAutoRunMode. A malicious or buggy client could send an invalid value.🔎 Suggested validation
+ const validAutoRunModes = ["ask-every-time", "auto-run-sandbox", "run-everything"]; + const validatedAutoRunMode = validAutoRunModes.includes(autoRunMode || "") + ? (autoRunMode as AutoRunMode) + : undefined; const { tools, // ... } = createTools( // ... - autoRunMode as AutoRunMode | undefined, + validatedAutoRunMode, );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
app/components/AgentsTab.tsxapp/components/MessagePartHandler.tsxapp/components/Messages.tsxapp/components/SharedLinksTab.tsxapp/components/chat.tsxapp/components/tools/TerminalCommandApproval.tsxapp/components/tools/TerminalToolHandler.tsxapp/contexts/GlobalState.tsxapp/hooks/useFileUrlCache.tsconvex/messages.tslib/ai/tools/index.tslib/ai/tools/run-terminal-cmd.tslib/api/chat-handler.tslib/db/actions.tslib/utils/approval-detection.tstypes/agent.ts
🧰 Additional context used
📓 Path-based instructions (1)
convex/**/*.{ts,js}
📄 CodeRabbit inference engine (.cursor/rules/convex_rules.mdc)
convex/**/*.{ts,js}: ALWAYS use the new function syntax for Convex functions withquery,mutation,actiondecorators that includeargs,returns, andhandlerproperties
Use array validators withv.array()to validate array arguments and schema fields
Use discriminated union types withv.union()andv.literal()for schema validation
Always use thev.null()validator when a function returns a null value
Usev.id(tableName)validator for document ID validation andId<'tableName'>TypeScript type for ID fields
v.bigint()is deprecated; usev.int64()instead for representing signed 64-bit integers
Usev.record()for defining record types;v.map()andv.set()are not supported
Index fields must be queried in the same order they are defined; create separate indexes if you need different query orders
UseinternalQuery,internalMutation, andinternalActionfor private functions that should not be part of the public API
Usequery,mutation, andactionfor public functions that are exposed to the public API
ALWAYS include argument and return validators for all Convex functions includingquery,internalQuery,mutation,internalMutation,action, andinternalAction
Functions that don't return anything should includereturns: v.null()as the output validator
Functions implicitly returnnullin JavaScript if they have no return statement
Usectx.runQueryto call a query from a query, mutation, or action
Usectx.runMutationto call a mutation from a mutation or action
Usectx.runActionto call an action from an action
Only call an action from another action if you need to cross runtimes (e.g., V8 to Node); otherwise extract shared code into a helper async function
Use as few calls from actions to queries and mutations as possible to avoid race conditions from split transaction logic
Pass aFunctionReferencetoctx.runQuery,ctx.runMutation, orctx.runAction; do not pass the callee function directly
When usi...
Files:
convex/messages.ts
🧠 Learnings (11)
📚 Learning: 2025-11-28T01:16:26.536Z
Learnt from: fkesheh
Repo: hackerai-tech/hackerai PR: 114
File: lib/ai/tools/utils/convex-sandbox.ts:95-145
Timestamp: 2025-11-28T01:16:26.536Z
Learning: In the local sandbox architecture (lib/ai/tools/utils/convex-sandbox.ts), background command execution semantics are handled by the local sandbox client (packages/local/src/index.ts), not by the ConvexSandbox class. When background: true, the local client immediately submits a result with empty output and PID after spawning the process, causing waitForResult() to resolve immediately and maintaining E2B-compatible non-blocking behavior.
Applied to files:
types/agent.tslib/ai/tools/run-terminal-cmd.tslib/api/chat-handler.ts
📚 Learning: 2025-12-24T18:09:08.561Z
Learnt from: CR
Repo: hackerai-tech/hackerai PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-24T18:09:08.561Z
Learning: Applies to convex/**/*.{ts,js} : Use `ctx.db.patch` to shallow merge updates into an existing document; throws an error if the document doesn't exist
Applied to files:
convex/messages.ts
📚 Learning: 2025-12-24T18:09:08.561Z
Learnt from: CR
Repo: hackerai-tech/hackerai PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-24T18:09:08.561Z
Learning: Applies to convex/**/*.{ts,js} : Use `ctx.db.replace` to fully replace an existing document; throws an error if the document doesn't exist
Applied to files:
convex/messages.ts
📚 Learning: 2025-12-24T18:09:08.561Z
Learnt from: CR
Repo: hackerai-tech/hackerai PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-24T18:09:08.561Z
Learning: Applies to convex/**/*.{ts,js} : Use `query`, `mutation`, and `action` for public functions that are exposed to the public API
Applied to files:
app/contexts/GlobalState.tsx
📚 Learning: 2025-12-24T18:09:08.561Z
Learnt from: CR
Repo: hackerai-tech/hackerai PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-24T18:09:08.561Z
Learning: Applies to convex/**/*.{ts,js} : Use `internalQuery`, `internalMutation`, and `internalAction` for private functions that should not be part of the public API
Applied to files:
app/contexts/GlobalState.tsx
📚 Learning: 2025-12-24T18:09:08.561Z
Learnt from: CR
Repo: hackerai-tech/hackerai PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-24T18:09:08.561Z
Learning: Applies to convex/**/*.{ts,js} : Use `ctx.runQuery` to call a query from a query, mutation, or action
Applied to files:
app/contexts/GlobalState.tsx
📚 Learning: 2025-12-24T18:09:08.561Z
Learnt from: CR
Repo: hackerai-tech/hackerai PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-24T18:09:08.561Z
Learning: Applies to convex/**/*.{ts,js} : Use as few calls from actions to queries and mutations as possible to avoid race conditions from split transaction logic
Applied to files:
app/contexts/GlobalState.tsx
📚 Learning: 2025-12-24T18:09:08.561Z
Learnt from: CR
Repo: hackerai-tech/hackerai PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-24T18:09:08.561Z
Learning: Applies to convex/**/*.{ts,js} : Use `ctx.runMutation` to call a mutation from a mutation or action
Applied to files:
app/contexts/GlobalState.tsx
📚 Learning: 2025-12-24T18:09:08.561Z
Learnt from: CR
Repo: hackerai-tech/hackerai PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-24T18:09:08.561Z
Learning: Applies to convex/**/*.{ts,js} : When using `ctx.runQuery`, `ctx.runMutation`, or `ctx.runAction` to call a function in the same file, add a type annotation on the return value to work around TypeScript circularity limitations
Applied to files:
app/contexts/GlobalState.tsx
📚 Learning: 2025-12-24T18:09:08.561Z
Learnt from: CR
Repo: hackerai-tech/hackerai PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-24T18:09:08.561Z
Learning: Applies to convex/**/*.{ts,js} : Thoughtfully organize files with public query, mutation, or action functions within the `convex/` directory for file-based routing
Applied to files:
app/contexts/GlobalState.tsx
📚 Learning: 2025-12-24T18:09:08.561Z
Learnt from: CR
Repo: hackerai-tech/hackerai PR: 0
File: .cursor/rules/convex_rules.mdc:0-0
Timestamp: 2025-12-24T18:09:08.561Z
Learning: Applies to convex/**/*.{ts,js} : Use `ctx.runAction` to call an action from an action
Applied to files:
app/components/AgentsTab.tsx
🧬 Code graph analysis (10)
app/components/SharedLinksTab.tsx (1)
types/chat.ts (1)
SharedChat(137-144)
app/components/Messages.tsx (1)
types/chat.ts (1)
ChatMessage(89-92)
convex/messages.ts (1)
convex/_generated/dataModel.d.ts (1)
Id(48-49)
app/components/AgentsTab.tsx (2)
components/ui/select.tsx (5)
Select(150-150)SelectTrigger(153-153)SelectValue(152-152)SelectContent(154-154)SelectItem(156-156)types/agent.ts (1)
AutoRunMode(30-33)
app/components/MessagePartHandler.tsx (2)
types/chat.ts (1)
ChatMessage(89-92)app/components/tools/TerminalToolHandler.tsx (1)
TerminalToolHandler(18-179)
app/components/tools/TerminalCommandApproval.tsx (5)
types/chat.ts (1)
ChatMessage(89-92)__mocks__/react-hotkeys-hook.ts (1)
useHotkeys(1-1)components/ui/button.tsx (1)
Button(59-59)components/ui/select.tsx (5)
Select(150-150)SelectTrigger(153-153)SelectValue(152-152)SelectContent(154-154)SelectItem(156-156)components/ui/tooltip.tsx (3)
Tooltip(61-61)TooltipTrigger(61-61)TooltipContent(61-61)
app/components/tools/TerminalToolHandler.tsx (3)
types/chat.ts (2)
ChatStatus(81-81)ChatMessage(89-92)app/contexts/GlobalState.tsx (1)
useGlobalState(776-782)app/components/tools/TerminalCommandApproval.tsx (1)
TerminalCommandApproval(42-216)
lib/api/chat-handler.ts (6)
lib/utils/approval-detection.ts (1)
isApprovalContinuation(11-23)lib/rate-limit.ts (1)
checkRateLimit(7-125)types/agent.ts (1)
AutoRunMode(30-33)convex/chats.ts (1)
updateChat(194-282)convex/messages.ts (1)
saveMessage(94-224)convex/tempStreams.ts (1)
deleteTempStreamForBackend(108-124)
app/components/chat.tsx (2)
types/chat.ts (1)
ChatMessage(89-92)lib/utils/approval-detection.ts (1)
isApprovalContinuation(11-23)
lib/ai/tools/index.ts (1)
types/agent.ts (1)
AutoRunMode(30-33)
🔇 Additional comments (21)
app/hooks/useFileUrlCache.ts (1)
125-127: LGTM!Formatting-only change to the type annotation. The loop logic remains correct.
app/components/SharedLinksTab.tsx (1)
201-203: LGTM!Formatting-only change to the
findcallback. The unshare confirmation logic is unchanged.lib/utils/approval-detection.ts (1)
11-23: LGTM!Clean implementation with proper null/empty handling. The
as anycast is acceptable given the dynamic structure of message parts.convex/messages.ts (1)
119-168: LGTM! Efficient conditional update logic.The changes correctly batch updates and only persist when there are actual changes beyond
update_time. The per-file error handling is appropriately resilient.One minor caveat:
JSON.stringifycomparison on line 131 may return false negatives if object key order differs betweenexistingMessage.partsandargs.parts(though this is unlikely with consistent serialization).app/components/MessagePartHandler.tsx (2)
20-29: LGTM!Clean addition of the
addToolApprovalResponseprop with proper typing fromUseChatHelpers. The prop is correctly optional and appropriately passed only toTerminalToolHandler.
86-95: LGTM!The
addToolApprovalResponseprop is correctly forwarded toTerminalToolHandlerfor bothdata-terminalandtool-run_terminal_cmdcases.app/components/AgentsTab.tsx (1)
219-261: LGTM! Well-structured Tool Approval settings UI.The implementation follows the existing patterns for other settings. Good UX with descriptive text for each option.
app/components/chat.tsx (2)
182-196: Auto-continue logic is well-implemented.The
sendAutomaticallyWhencallback correctly identifies when to auto-continue after tool approval by checking forapproval-respondedstate withapproved === true. This prevents auto-continuation for denied tools, which aligns with the PR's design intent.
245-253: Message selection handles approval flow correctly.The three-way conditional for
messagesToSendproperly handles:
- Temporary chats: all messages (not persisted)
- Approval continuations: all normalized messages (contains approval state)
- Normal requests: just the last user message
lib/ai/tools/index.ts (2)
50-51: Parameter propagation looks good.The optional
autoRunModeparameter is correctly added at the end of the parameter list, maintaining backward compatibility. It's properly passed to theToolContextobject.
89-91: Context object extended appropriately.Both
autoRunModeandsandboxPreferenceare added to the tool context, enabling downstream tools (likerun-terminal-cmd) to access these values for approval logic.app/contexts/GlobalState.tsx (1)
764-767: Context value extension is correct.The
autoRunModeandsetAutoRunModeare properly exposed in the context value, following the established pattern for other state fields.app/components/Messages.tsx (1)
64-65: Prop threading is consistent.The
addToolApprovalResponseprop is correctly typed usingUseChatHelpers<ChatMessage>["addToolApprovalResponse"]and consistently passed through to allMessagePartHandlerinstances across user messages, assistant messages, and file-only assistant messages.lib/ai/tools/run-terminal-cmd.ts (2)
33-46: Approval logic correctly implements the three modes.The
needsApprovalcomputation properly handles:
run-everything: Never asks (needsApproval = false)ask-every-time: Always asks (needsApproval = true)auto-run-sandbox(default): Only asks for local sandboxes (when preference is set and not "e2b")
113-113: Conditional tool property is idiomatic.Using spread with conditional object
...(needsApproval ? { needsApproval: true } : {})cleanly adds the property only when needed, avoiding sending unnecessary data to the AI SDK.app/components/tools/TerminalCommandApproval.tsx (2)
66-78: Verify Enter hotkey doesn't conflict with chat input.The
useHotkeyswithenableOnFormTags: trueandenableOnContentEditable: truewill capture Enter keypresses even in the chat textarea. This could prevent users from typing multi-line messages when an approval is pending.Consider adding a check to ensure the chat input doesn't have focus, or scope the hotkey more narrowly.
42-215: Component structure is clean and well-organized.The switch-case pattern for handling different approval states is clear and maintainable. Each state renders appropriate UI with proper accessibility attributes (aria-label on copy button).
app/components/tools/TerminalToolHandler.tsx (1)
122-138: Approval state routing is correctly implemented.The switch cases for
approval-requested,approval-responded, andoutput-deniedproperly delegate toTerminalCommandApprovalwith all required props, including handlers for sidebar interaction.lib/api/chat-handler.ts (3)
143-157: Approval flow handling prevents duplicate saves and charges.The logic correctly:
- Detects approval continuations using
isApprovalContinuation- Skips
handleInitialChatAndUserMessagefor approval flows (frontend already saved)- Prevents double-charging by bypassing rate limit check
This is a well-designed approach to handle the approval continuation pattern.
476-491: Pending approval detection clears finish reason correctly.When
streamFinishReason === "tool-calls"and there are pending approvals (approval-requestedstate), the finish reason is cleared. This prevents showing a confusing "max steps reached" notice when the stream simply paused for user approval.
174-179: Assistant message ID reuse for approval flows is correct.For approval continuations, reusing the existing assistant message ID ensures the approval state updates are applied to the same message rather than creating a new one. The fallback to
uuidv4()handles edge cases where no assistant message exists.
| <SelectContent> | ||
| <SelectItem value="ask-every-time">Ask Every Time</SelectItem> | ||
| <SelectItem value="auto-run-sandbox">Run Everything</SelectItem> | ||
| <SelectItem value="run-everything"> | ||
| Run Everything (Unsandboxed) | ||
| </SelectItem> | ||
| </SelectContent> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Select option labels may confuse users.
The labels don't clearly communicate the behavior:
"auto-run-sandbox"shows "Run Everything" but actually only auto-runs in E2B sandbox"run-everything"shows "Run Everything (Unsandboxed)" but runs everywhere without asking
Consider more descriptive labels to match the types/agent.ts comments.
🔎 Suggested label improvements
<SelectContent>
<SelectItem value="ask-every-time">Ask Every Time</SelectItem>
- <SelectItem value="auto-run-sandbox">Run Everything</SelectItem>
- <SelectItem value="run-everything">
- Run Everything (Unsandboxed)
- </SelectItem>
+ <SelectItem value="auto-run-sandbox">Auto-run in Cloud Sandbox</SelectItem>
+ <SelectItem value="run-everything">Auto-run Everything</SelectItem>
</SelectContent>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <SelectContent> | |
| <SelectItem value="ask-every-time">Ask Every Time</SelectItem> | |
| <SelectItem value="auto-run-sandbox">Run Everything</SelectItem> | |
| <SelectItem value="run-everything"> | |
| Run Everything (Unsandboxed) | |
| </SelectItem> | |
| </SelectContent> | |
| <SelectContent> | |
| <SelectItem value="ask-every-time">Ask Every Time</SelectItem> | |
| <SelectItem value="auto-run-sandbox">Auto-run in Cloud Sandbox</SelectItem> | |
| <SelectItem value="run-everything"> | |
| Auto-run Everything | |
| </SelectItem> | |
| </SelectContent> |
🤖 Prompt for AI Agents
In app/components/tools/TerminalCommandApproval.tsx around lines 130 to 136, the
SelectItem labels are ambiguous: "Run Everything" is used for the
auto-run-in-sandbox mode and "Run Everything (Unsandboxed)" is used for the
global auto-run mode. Update the labels to clearly reflect each mode: rename the
value "ask-every-time" label to "Ask Every Time (Prompt Before Run)", change
"auto-run-sandbox" label to "Auto-Run in E2B Sandbox", and change
"run-everything" label to "Auto-Run Everywhere (Unsandboxed)" so the UI matches
the behavior described in types/agent.ts; keep the SelectItem values unchanged.
| autoRunMode={autoRunMode || "auto-run-sandbox"} | ||
| setAutoRunMode={setAutoRunMode!} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-null assertion on setAutoRunMode could cause runtime error.
Using setAutoRunMode! assumes the function is always defined, but it comes from useGlobalState() which could theoretically return undefined in edge cases. Consider providing a fallback or handling the undefined case.
🔎 Suggested safer approach
- setAutoRunMode={setAutoRunMode!}
+ setAutoRunMode={setAutoRunMode ?? (() => {})}Or add a guard before rendering:
if (!setAutoRunMode) return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| autoRunMode={autoRunMode || "auto-run-sandbox"} | |
| setAutoRunMode={setAutoRunMode!} | |
| autoRunMode={autoRunMode || "auto-run-sandbox"} | |
| setAutoRunMode={setAutoRunMode ?? (() => {})} |
🤖 Prompt for AI Agents
In app/components/tools/TerminalToolHandler.tsx around lines 130 to 131, the
code uses a non-null assertion on setAutoRunMode (setAutoRunMode!) which can
cause a runtime error if useGlobalState() returns undefined; remove the non-null
assertion and either: 1) guard before rendering (e.g., if (!setAutoRunMode)
return null or render a disabled/readonly UI) or 2) provide a safe fallback
handler (e.g., const safeSetAutoRunMode = setAutoRunMode ?? (()=>{}) and pass
that), and adjust prop typing if necessary so the component accepts an optional
setter without asserting it exists.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.