feat: implement supervised permission mode for Claude provider#277
Merged
feat: implement supervised permission mode for Claude provider#277
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 a supervised permission-request workflow: providers create and track pending permission requests and emit provider events; the server exposes RPCs and push channels to list/respond and forwards provider events; the frontend stores, displays, and resolves permission requests via transport, store, and UI components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Frontend App
participant Store as Thread Store
participant Transport as WS Transport
participant Server as Server
participant Provider as Claude Provider
autonumber
rect rgba(100,150,200,0.5)
note over User,Provider: Permission request creation
User->>Provider: Invoke tool (provider checks canUseTool)
Provider->>Provider: Create pending request (requestId, threadId, tool, input)
Provider->>Server: Emit permission_request
Server->>Transport: broadcast permission.request
Server->>Transport: portPush.send permission.request
Transport->>Frontend: deliver permission.request
Frontend->>Store: addPermissionRequest(request)
Store->>Frontend: state update -> render PermissionRequestCard
end
rect rgba(150,100,200,0.5)
note over User,Provider: Permission resolution
User->>Frontend: Click Allow / Allow-Session / Deny
Frontend->>Transport: respondToPermission(requestId, decision)
Transport->>Server: RPC permission.respond
Server->>Server: AgentService.respondToPermission(...)
Server->>Provider: provider.resolvePermission(requestId, decision)
Provider->>Provider: resolve pending request, apply session rule if needed
Provider->>Server: Emit permission_resolved
Server->>Transport: broadcast permission.resolved
Server->>Transport: portPush.send permission.resolved
Transport->>Frontend: deliver permission.resolved
Frontend->>Store: resolvePermissionRequest(requestId, decision)
Store->>Frontend: state update -> collapse to settled badge
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/web/src/components/chat/MessageList.tsx (1)
149-152: Minor: redundant variable assignment.
currentThreadIdis assigned fromactiveThreadIdbut adds no clarity. Consider usingactiveThreadIddirectly.🔧 Proposed simplification
- const currentThreadId = activeThreadId; const permissions = useThreadStore( - useShallow((s) => currentThreadId ? (s.permissionsByThread[currentThreadId] ?? []) : []), + useShallow((s) => activeThreadId ? (s.permissionsByThread[activeThreadId] ?? []) : []), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/MessageList.tsx` around lines 149 - 152, The variable currentThreadId is an unnecessary alias of activeThreadId; remove currentThreadId and update the useThreadStore call to reference activeThreadId directly (keep the same selector logic that accesses permissionsByThread[activeThreadId] and falls back to []), ensuring symbols touched are currentThreadId, activeThreadId, permissions, useThreadStore and useShallow are updated accordingly to avoid unused-variable lint warnings.apps/server/src/services/agent-service.ts (1)
687-698: Consider returning success status fromrespondToPermission.The method logs a warning when no provider holds the
requestId, but the caller (ws-router.ts) receivesvoidand cannot communicate failure to the client. If the permission request expired or was already resolved, the user gets no feedback.This may be intentional per the PR design (no timeout, agent waits indefinitely), but consider returning a boolean so the router can optionally surface a user-friendly error.
♻️ Optional: return success indicator
- respondToPermission(requestId: string, decision: PermissionDecision): void { + respondToPermission(requestId: string, decision: PermissionDecision): boolean { for (const provider of this.providerRegistry.resolveAll()) { if (provider.resolvePermission?.(requestId, decision)) { - return; + return true; } } logger.warn("permission.respond: no provider holds requestId %s", requestId); + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/services/agent-service.ts` around lines 687 - 698, Change respondToPermission to return a boolean success indicator: update its signature from respondToPermission(requestId: string, decision: PermissionDecision): void to returning boolean, then iterate providers from providerRegistry.resolveAll() and if provider.resolvePermission?.(requestId, decision) returns true return true immediately; after the loop log the warning and return false. Update type annotations and any callers (e.g., the ws-router.ts call site) to check the boolean result and surface a client-friendly error when false.apps/web/src/components/chat/PermissionRequestCard.tsx (2)
81-95: Consider guarding against setState on unmounted component.If the component unmounts while the
respondToPermissioncall is in-flight (e.g., user navigates away), thefinallyblock will attempt to callsetResponding(false)on an unmounted component. While React 18+ no longer warns about this, it's still a no-op that could be avoided.♻️ Optional fix using an abort ref
+ const mountedRef = useRef(true); + useEffect(() => { + return () => { mountedRef.current = false; }; + }, []); + const respond = useCallback( async (d: PermissionDecision) => { setResponding(true); setDropdownOpen(false); try { setError(null); await getTransport().respondToPermission(requestId, d); } catch { - setError("Failed to send response. Please try again."); + if (mountedRef.current) setError("Failed to send response. Please try again."); } finally { - setResponding(false); + if (mountedRef.current) setResponding(false); } }, [requestId], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/PermissionRequestCard.tsx` around lines 81 - 95, The respond callback can call setResponding/setError after the component unmounts; fix by adding an isMounted/abort ref (e.g., abortRef or isMountedRef) that is set true in useEffect and flipped to false on cleanup, then inside respond (the async function that calls getTransport().respondToPermission(requestId, d)) check the ref before calling setError or setResponding in both catch and finally so state updates are skipped when unmounted; ensure the ref is referenced in the cleanup and not recreated on every render so respond's closure can read it.
154-187: Consider adding keyboard navigation for accessibility.The dropdown has proper ARIA attributes (
role="menu",role="menuitem",aria-expanded), but lacks keyboard handling:
- Escape key to close
- Arrow keys to navigate
- Focus management when opening/closing
This is a minor accessibility gap for users navigating via keyboard.
♻️ Example keyboard handler
+ // Close dropdown on Escape + useEffect(() => { + if (!dropdownOpen) return; + const handleKeyDown = (e: KeyboardEvent) => { + if (e.key === "Escape") setDropdownOpen(false); + }; + document.addEventListener("keydown", handleKeyDown); + return () => document.removeEventListener("keydown", handleKeyDown); + }, [dropdownOpen]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/PermissionRequestCard.tsx` around lines 154 - 187, The dropdown in PermissionRequestCard lacks keyboard handlers; add keyboard navigation by wiring an onKeyDown and focus management around dropdownOpen and setDropdownOpen: when opening (dropdownOpen true) move focus to the first menu item (use refs for the menu container and menu items), handle Escape to close and return focus to the toggle button, handle ArrowDown/ArrowUp to cycle focus between menu items, and ensure disabled/responding items are skipped; also ensure any global listeners are cleaned up on unmount and that respond("allow-session") is still invoked on Enter/Space when a menuitem is focused.
🤖 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/web/src/components/chat/virtual-items.ts`:
- Around line 112-126: Settled permission requests are never removed and will
accumulate; update resolvePermissionRequest to mark the request settled as you
do now but then schedule a short removal (e.g. setTimeout for the collapse
animation duration, configurable N ms) that removes settled entries from
permissionsByThread and dispatches the store update so
MessageList/buildVolatileItems no longer render them; ensure the delay matches
the UI collapse animation and only removes requests with settled === true for
the specific thread/requestId to avoid races.
In `@packages/contracts/src/models/permission.ts`:
- Around line 14-21: The PermissionRequestSchema Zod object should be wrapped
with lazySchema to defer construction; locate the exported
PermissionRequestSchema constant and replace its direct z.object(...) definition
with a lazySchema(() => z.object({...})) wrapper (keeping all fields: requestId,
threadId, toolName, input, optional title) so the schema is only constructed on
first use and adheres to the non-trivial schema guideline.
In `@packages/contracts/src/ws/channels.ts`:
- Around line 40-46: Wrap the non-trivial Zod schemas PermissionRequestSchema
and PermissionDecisionSchema with lazySchema where they are defined (replace
their direct exports with lazySchema(() => /* original schema */) in their
source), then update all call sites (e.g., in channels.ts and methods.ts where
PermissionRequestSchema and PermissionDecisionSchema are referenced) to invoke
the lazy wrappers as functions (PermissionRequestSchema() and
PermissionDecisionSchema()) so the deferred construction is used; ensure imports
remain the same names but now refer to the lazy-wrapped values.
---
Nitpick comments:
In `@apps/server/src/services/agent-service.ts`:
- Around line 687-698: Change respondToPermission to return a boolean success
indicator: update its signature from respondToPermission(requestId: string,
decision: PermissionDecision): void to returning boolean, then iterate providers
from providerRegistry.resolveAll() and if
provider.resolvePermission?.(requestId, decision) returns true return true
immediately; after the loop log the warning and return false. Update type
annotations and any callers (e.g., the ws-router.ts call site) to check the
boolean result and surface a client-friendly error when false.
In `@apps/web/src/components/chat/MessageList.tsx`:
- Around line 149-152: The variable currentThreadId is an unnecessary alias of
activeThreadId; remove currentThreadId and update the useThreadStore call to
reference activeThreadId directly (keep the same selector logic that accesses
permissionsByThread[activeThreadId] and falls back to []), ensuring symbols
touched are currentThreadId, activeThreadId, permissions, useThreadStore and
useShallow are updated accordingly to avoid unused-variable lint warnings.
In `@apps/web/src/components/chat/PermissionRequestCard.tsx`:
- Around line 81-95: The respond callback can call setResponding/setError after
the component unmounts; fix by adding an isMounted/abort ref (e.g., abortRef or
isMountedRef) that is set true in useEffect and flipped to false on cleanup,
then inside respond (the async function that calls
getTransport().respondToPermission(requestId, d)) check the ref before calling
setError or setResponding in both catch and finally so state updates are skipped
when unmounted; ensure the ref is referenced in the cleanup and not recreated on
every render so respond's closure can read it.
- Around line 154-187: The dropdown in PermissionRequestCard lacks keyboard
handlers; add keyboard navigation by wiring an onKeyDown and focus management
around dropdownOpen and setDropdownOpen: when opening (dropdownOpen true) move
focus to the first menu item (use refs for the menu container and menu items),
handle Escape to close and return focus to the toggle button, handle
ArrowDown/ArrowUp to cycle focus between menu items, and ensure
disabled/responding items are skipped; also ensure any global listeners are
cleaned up on unmount and that respond("allow-session") is still invoked on
Enter/Space when a menuitem is focused.
🪄 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: 13ccd00f-0487-41ca-8188-443f8b7d81de
📒 Files selected for processing (21)
apps/server/src/index.tsapps/server/src/providers/claude/claude-provider.tsapps/server/src/services/agent-service.tsapps/server/src/transport/ws-router.tsapps/web/src/__tests__/mocks/transport.tsapps/web/src/__tests__/thread-status.test.tsapps/web/src/components/chat/MessageList.tsxapps/web/src/components/chat/PermissionRequestCard.tsxapps/web/src/components/chat/virtual-items.tsapps/web/src/components/sidebar/ProjectTree.test.tsxapps/web/src/components/sidebar/ProjectTree.tsxapps/web/src/lib/thread-status.tsapps/web/src/stores/threadStore.tsapps/web/src/transport/types.tsapps/web/src/transport/ws-events.tsapps/web/src/transport/ws-transport.tspackages/contracts/src/index.tspackages/contracts/src/models/permission.tspackages/contracts/src/providers/interfaces.tspackages/contracts/src/ws/channels.tspackages/contracts/src/ws/methods.ts
Comment on lines
+40
to
+46
| /** A tool permission request awaiting user decision. */ | ||
| "permission.request": PermissionRequestSchema, | ||
| /** Notification that a permission request has been settled. */ | ||
| "permission.resolved": z.object({ | ||
| requestId: z.string(), | ||
| decision: PermissionDecisionSchema, | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check permission schemas and channel declarations:"
rg -n "export const Permission(Request|Decision)Schema" packages/contracts/src/models/permission.ts
rg -n -A4 -B2 '"permission\.(request|resolved)"' packages/contracts/src/ws/channels.ts
rg -n "lazySchema\(" packages/contracts/src/models/permission.ts packages/contracts/src/ws/channels.tsRepository: Mzeey-Emipre/mcode
Length of output: 544
🏁 Script executed:
cat -n packages/contracts/src/models/permission.tsRepository: Mzeey-Emipre/mcode
Length of output: 1000
🏁 Script executed:
rg "PermissionRequestSchema|PermissionDecisionSchema" packages/contracts/src/ -nRepository: Mzeey-Emipre/mcode
Length of output: 1227
🏁 Script executed:
sed -n '200,220p' packages/contracts/src/ws/methods.tsRepository: Mzeey-Emipre/mcode
Length of output: 670
🏁 Script executed:
rg -B2 -A2 "lazySchema" packages/contracts/src/ --type tsRepository: Mzeey-Emipre/mcode
Length of output: 7800
Wrap permission schemas with lazySchema at their source, then invoke as functions at call sites.
Per coding guidelines, all non-trivial Zod schemas in packages/contracts/src/**/*.ts must be wrapped with lazySchema to defer construction until first use. The PermissionRequestSchema and PermissionDecisionSchema in packages/contracts/src/models/permission.ts are non-trivial and should be wrapped at their source. Call sites in channels.ts and methods.ts then invoke them as functions.
The proposed fix as written would fail because PermissionRequestSchema is not currently wrapped with lazySchema—it cannot be invoked as a function. The schemas must first be wrapped at their source definition in permission.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/contracts/src/ws/channels.ts` around lines 40 - 46, Wrap the
non-trivial Zod schemas PermissionRequestSchema and PermissionDecisionSchema
with lazySchema where they are defined (replace their direct exports with
lazySchema(() => /* original schema */) in their source), then update all call
sites (e.g., in channels.ts and methods.ts where PermissionRequestSchema and
PermissionDecisionSchema are referenced) to invoke the lazy wrappers as
functions (PermissionRequestSchema() and PermissionDecisionSchema()) so the
deferred construction is used; ensure imports remain the same names but now
refer to the lazy-wrapped values.
chuks-qua
force-pushed
the
feat/can-use-tool
branch
from
1d812e9 to
b53c752
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/server/src/providers/claude/claude-provider.ts`:
- Around line 166-176: Pending permission promises stored in pendingPermissions
(used by canUseTool()) aren't resolved on all teardown paths; ensure every
session-abort path (including evictIdleSessions(), unexpected stream shutdown
handlers, and anywhere a session/query is closed) drains pendingPermissions
entries for that session/threadId and resolves each stored resolve callback with
a terminal PermissionDecision (e.g., denied/cancelled) before removing them.
Update evictIdleSessions(), the stream shutdown/error handlers, and any other
session cleanup logic to mirror the cleanup currently in stopSession() and
shutdown(): iterate pendingPermissions, filter by threadId (or session id), call
the stored resolve with the chosen PermissionDecision, and delete the map
entries so UI cards and provider promises are not left orphaned.
In `@apps/web/src/components/chat/MessageList.tsx`:
- Around line 149-152: The new permission cards live in volatileItems and are
not being included in the “new content” affordance logic, so update the
MessageList selectors/effects that determine auto-scroll/highlight to also watch
volatileItems and per-thread permission entries: modify the useThreadStore
selection (the one using useShallow and currentThreadId) and any other selector
at the similar spot (lines referencing permissions/volatileItems) to return a
combined list (e.g., [...(s.messagesByThread[currentThreadId] ?? []),
...(s.toolCallsByThread[currentThreadId] ?? []),
...(s.streamingByThread[currentThreadId] ?? []),
...(s.volatileItems[currentThreadId] ?? []),
...(s.permissionsByThread[currentThreadId] ?? [])]) so the same change-detection
triggers apply to permission cards; ensure the effect that triggers
scroll/highlight (the logic watching these arrays) uses this combined selector
so permission requests will auto-scroll and highlight just like messages/tool
calls.
In `@apps/web/src/components/chat/PermissionRequestCard.tsx`:
- Around line 90-91: The component computes inputPreview from an unknown input
which can make JSON.stringify throw (circular refs, BigInt, etc.); wrap the
stringify call in a try-catch inside PermissionRequestCard where inputPreview is
created (variable inputPreview) and on failure fallback to a safe string (e.g.,
"[unserializable input]" or use String(input) / a truncated util.inspect result)
so the render never crashes; ensure the catch handles any error and still
returns a stable string for rendering.
In `@apps/web/src/stores/threadStore.ts`:
- Around line 349-359: The current rehydration unconditionally replaces
permissionsByThread[threadId] and can drop concurrent updates from
permission.request / permission.resolved; instead, when handling
getTransport().listPendingPermissions(threadId) merge by requestId: build a map
of existing entries from s.permissionsByThread[threadId] keyed by requestId,
then produce a merged array that includes all unique requestIds (union of
existing and pending) where for each pending entry from listPendingPermissions
you preserve existing.settled and existing.decision if present (otherwise set
settled:false), and also include any existing-only entries that are newer than
the pending list; finally call set((s) => ({ permissionsByThread: {
...s.permissionsByThread, [threadId]: mergedArray } })) so you do not overwrite
concurrent updates.
🪄 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: ff1f129e-7f61-44c0-8b71-35f72e888331
📒 Files selected for processing (21)
apps/server/src/index.tsapps/server/src/providers/claude/claude-provider.tsapps/server/src/services/agent-service.tsapps/server/src/transport/ws-router.tsapps/web/src/__tests__/mocks/transport.tsapps/web/src/__tests__/thread-status.test.tsapps/web/src/components/chat/MessageList.tsxapps/web/src/components/chat/PermissionRequestCard.tsxapps/web/src/components/chat/virtual-items.tsapps/web/src/components/sidebar/ProjectTree.test.tsxapps/web/src/components/sidebar/ProjectTree.tsxapps/web/src/lib/thread-status.tsapps/web/src/stores/threadStore.tsapps/web/src/transport/types.tsapps/web/src/transport/ws-events.tsapps/web/src/transport/ws-transport.tspackages/contracts/src/index.tspackages/contracts/src/models/permission.tspackages/contracts/src/providers/interfaces.tspackages/contracts/src/ws/channels.tspackages/contracts/src/ws/methods.ts
✅ Files skipped from review due to trivial changes (3)
- apps/web/src/components/sidebar/ProjectTree.test.tsx
- packages/contracts/src/index.ts
- packages/contracts/src/models/permission.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/contracts/src/ws/channels.ts
- apps/web/src/tests/thread-status.test.ts
- apps/server/src/transport/ws-router.ts
- apps/web/src/transport/ws-events.ts
- apps/web/src/transport/types.ts
- apps/web/src/components/sidebar/ProjectTree.tsx
- apps/web/src/transport/ws-transport.ts
- packages/contracts/src/ws/methods.ts
- apps/server/src/services/agent-service.ts
- apps/web/src/lib/thread-status.ts
- apps/web/src/components/chat/virtual-items.ts
Comment on lines
+90
to
+91
| const inputPreview = | ||
| typeof input === "string" ? input : JSON.stringify(input, null, 2); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "PermissionRequestCard.tsx" -type fRepository: Mzeey-Emipre/mcode
Length of output: 118
🏁 Script executed:
cat -n apps/web/src/components/chat/PermissionRequestCard.tsxRepository: Mzeey-Emipre/mcode
Length of output: 8128
🏁 Script executed:
# Check if there are type definitions for PermissionRequest
rg "PermissionRequest" -A 5 -B 5 --type ts --type tsx | head -100Repository: Mzeey-Emipre/mcode
Length of output: 89
🏁 Script executed:
# Check for similar patterns of unsafe JSON.stringify usage in the codebase
rg "JSON\.stringify.*unknown|typeof.*string.*JSON\.stringify" -A 3 -B 3 --type ts --type tsx | head -80Repository: Mzeey-Emipre/mcode
Length of output: 89
🏁 Script executed:
# Check for error boundaries or similar in parent components
rg "ErrorBoundary|try.*JSON\.stringify|catch.*JSON" -B 2 -A 2 --type ts --type tsxRepository: Mzeey-Emipre/mcode
Length of output: 89
🏁 Script executed:
# Check what creates PermissionRequestCard to understand data sources
rg "PermissionRequestCard" -B 3 -A 3 --type ts --type tsxRepository: Mzeey-Emipre/mcode
Length of output: 89
🏁 Script executed:
# Look for how tool inputs are generated/passed to understand if circular refs are possible
rg "tool.*input|PermissionRequest.*input" -B 2 -A 2 --type ts --type tsx | head -60Repository: Mzeey-Emipre/mcode
Length of output: 89
🏁 Script executed:
# Search without tsx type specification
rg "ErrorBoundary|try.*JSON\.stringify|catch.*JSON" -B 2 -A 2Repository: Mzeey-Emipre/mcode
Length of output: 2234
🏁 Script executed:
# Find where PermissionRequestCard is used
rg "PermissionRequestCard" -B 3 -A 3Repository: Mzeey-Emipre/mcode
Length of output: 3099
🏁 Script executed:
# Check contracts to understand PermissionRequest shape
find . -name "*contract*" -o -name "*types*" | grep -i permission | head -10Repository: Mzeey-Emipre/mcode
Length of output: 44
🏁 Script executed:
# Look for PermissionRequest type definition
rg "type PermissionRequest|interface PermissionRequest" -B 2 -A 10Repository: Mzeey-Emipre/mcode
Length of output: 1528
🏁 Script executed:
# Check the PermissionRequest schema to understand input structure
cat packages/contracts/src/models/permission.tsRepository: Mzeey-Emipre/mcode
Length of output: 839
🏁 Script executed:
# Verify if PermissionRequestCard is always used within ToolCallWrapper/error boundary
rg "PermissionRequestCard" -B 5 -A 5 apps/web/src/components/chat/MessageList.tsxRepository: Mzeey-Emipre/mcode
Length of output: 790
🏁 Script executed:
# Check how MessageList renders items and if it has error boundaries
rg "class.*ErrorBoundary|error.*boundary|Error.*Boundary" apps/web/src/components/chat/MessageList.tsxRepository: Mzeey-Emipre/mcode
Length of output: 44
🏁 Script executed:
# Double-check if MessageList has any error handling wrapper at the call site
cat apps/web/src/components/chat/MessageList.tsx | head -50Repository: Mzeey-Emipre/mcode
Length of output: 1915
🏁 Script executed:
# Search for how items are built and if they go through any error handling
rg "buildVolatileItems|buildStableItems" -A 10 apps/web/src/components/chat/MessageList.tsx | head -40Repository: Mzeey-Emipre/mcode
Length of output: 1101
🏁 Script executed:
# Check if VirtualItemRenderer or MessageList has error boundaries
rg "try.*catch|error.*catch" apps/web/src/components/chat/MessageList.tsxRepository: Mzeey-Emipre/mcode
Length of output: 44
🏁 Script executed:
# Final verification: check if PermissionRequestCard is ever wrapped in error boundary at usage site
rg "PermissionRequestCard" -B 10 -A 10 apps/web/src/components/chat/MessageList.tsx | grep -E "(ErrorBoundary|try|catch|PermissionRequestCard)"Repository: Mzeey-Emipre/mcode
Length of output: 157
Wrap JSON.stringify in error handling for unknown tool inputs.
Lines 91-92: input is unknown, so JSON.stringify can throw on circular references, BigInt values, or other non-serializable types and crash the component render. Add a try-catch fallback to prevent unprotected render failures.
Suggested fix
const inputPreview =
- typeof input === "string" ? input : JSON.stringify(input, null, 2);
+ (() => {
+ if (typeof input === "string") return input;
+ try {
+ return JSON.stringify(input, null, 2) ?? String(input);
+ } catch {
+ return String(input);
+ }
+ })();📝 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.
Suggested change
| const inputPreview = | |
| typeof input === "string" ? input : JSON.stringify(input, null, 2); | |
| const inputPreview = | |
| (() => { | |
| if (typeof input === "string") return input; | |
| try { | |
| return JSON.stringify(input, null, 2) ?? String(input); | |
| } catch { | |
| return String(input); | |
| } | |
| })(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/chat/PermissionRequestCard.tsx` around lines 90 - 91,
The component computes inputPreview from an unknown input which can make
JSON.stringify throw (circular refs, BigInt, etc.); wrap the stringify call in a
try-catch inside PermissionRequestCard where inputPreview is created (variable
inputPreview) and on failure fallback to a safe string (e.g., "[unserializable
input]" or use String(input) / a truncated util.inspect result) so the render
never crashes; ensure the catch handles any error and still returns a stable
string for rendering.
Comment on lines
+349
to
+359
| // Re-hydrate pending permissions (covers reconnect and thread switch) | ||
| void getTransport() | ||
| .listPendingPermissions(threadId) | ||
| .then((pending) => { | ||
| if (pending.length > 0) { | ||
| set((s) => ({ | ||
| permissionsByThread: { | ||
| ...s.permissionsByThread, | ||
| [threadId]: pending.map((p) => ({ ...p, settled: false })), | ||
| }, | ||
| })); |
There was a problem hiding this comment.
Merge rehydrated permissions instead of replacing the thread bucket.
This assignment can race with permission.request / permission.resolved pushes and drop newer requests or revert a just-settled card back to pending. Rehydrate by requestId and preserve any existing settled/decision state instead of overwriting the whole array.
Suggested fix
- set((s) => ({
- permissionsByThread: {
- ...s.permissionsByThread,
- [threadId]: pending.map((p) => ({ ...p, settled: false })),
- },
- }));
+ set((s) => {
+ const current = s.permissionsByThread[threadId] ?? [];
+ const byId = new Map(current.map((p) => [p.requestId, p]));
+ for (const p of pending) {
+ const existing = byId.get(p.requestId);
+ byId.set(p.requestId, {
+ ...p,
+ settled: existing?.settled ?? false,
+ decision: existing?.decision,
+ });
+ }
+ return {
+ permissionsByThread: {
+ ...s.permissionsByThread,
+ [threadId]: Array.from(byId.values()),
+ },
+ };
+ });📝 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.
Suggested change
| // Re-hydrate pending permissions (covers reconnect and thread switch) | |
| void getTransport() | |
| .listPendingPermissions(threadId) | |
| .then((pending) => { | |
| if (pending.length > 0) { | |
| set((s) => ({ | |
| permissionsByThread: { | |
| ...s.permissionsByThread, | |
| [threadId]: pending.map((p) => ({ ...p, settled: false })), | |
| }, | |
| })); | |
| // Re-hydrate pending permissions (covers reconnect and thread switch) | |
| void getTransport() | |
| .listPendingPermissions(threadId) | |
| .then((pending) => { | |
| if (pending.length > 0) { | |
| set((s) => { | |
| const current = s.permissionsByThread[threadId] ?? []; | |
| const byId = new Map(current.map((p) => [p.requestId, p])); | |
| for (const p of pending) { | |
| const existing = byId.get(p.requestId); | |
| byId.set(p.requestId, { | |
| ...p, | |
| settled: existing?.settled ?? false, | |
| decision: existing?.decision, | |
| }); | |
| } | |
| return { | |
| permissionsByThread: { | |
| ...s.permissionsByThread, | |
| [threadId]: Array.from(byId.values()), | |
| }, | |
| }; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/stores/threadStore.ts` around lines 349 - 359, The current
rehydration unconditionally replaces permissionsByThread[threadId] and can drop
concurrent updates from permission.request / permission.resolved; instead, when
handling getTransport().listPendingPermissions(threadId) merge by requestId:
build a map of existing entries from s.permissionsByThread[threadId] keyed by
requestId, then produce a merged array that includes all unique requestIds
(union of existing and pending) where for each pending entry from
listPendingPermissions you preserve existing.settled and existing.decision if
present (otherwise set settled:false), and also include any existing-only
entries that are newer than the pending list; finally call set((s) => ({
permissionsByThread: { ...s.permissionsByThread, [threadId]: mergedArray } }))
so you do not overwrite concurrent updates.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/providers/claude/claude-provider.ts (1)
967-980:⚠️ Potential issue | 🟠 MajorPending permission promises orphaned on unexpected stream termination.
The
finallyblock instartStreamLoop(lines 967-980) deletes the session but does not resolve pending permission requests for that thread. If the SDK stream errors out or terminates unexpectedly, the UI will show stale approval cards and the provider-side promises will never settle.Consider adding permission cleanup here similar to
stopSession:🐛 Proposed fix to drain pending permissions on stream end
} finally { const current = this.sessions.get(sessionId); if (current?.query === q) { this.sessions.delete(sessionId); } + // Drain pending permission requests so promises settle and UI clears. + const tid = sessionId.startsWith("mcode-") ? sessionId.slice(6) : sessionId; + for (const [requestId, entry] of this.pendingPermissions) { + if (entry.threadId === tid) { + this.pendingPermissions.delete(requestId); + entry.resolve("cancelled"); + this.emit("permission_resolved", { requestId, decision: "cancelled" }); + } + } logger.info("Session stream ended", { sessionId });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/claude/claude-provider.ts` around lines 967 - 980, In startStreamLoop's finally block (around the sessions cleanup) add the same pending-permission draining used by stopSession so any pending permission promises for the threadId are settled when a stream terminates unexpectedly; specifically, after deleting this.sessions.get(sessionId) and before emitting Ended, invoke the permission-cleanup routine used in stopSession (or inline the same logic) to resolve/reject and remove any entries for threadId so approval cards and provider-side promises are not left orphaned (refer to startStreamLoop, stopSession, sessionId, threadId, sessions, suppressEnded).
♻️ Duplicate comments (1)
apps/web/src/components/chat/PermissionRequestCard.tsx (1)
92-93:⚠️ Potential issue | 🟡 MinorWrap JSON.stringify in error handling for unknown tool inputs.
inputisunknown, soJSON.stringifycan throw on circular references,BigIntvalues, or other non-serializable types. This would crash the component render.🛡️ Proposed fix with try-catch
const inputPreview = - typeof input === "string" ? input : JSON.stringify(input, null, 2); + typeof input === "string" + ? input + : (() => { + try { + return JSON.stringify(input, null, 2); + } catch { + return "[Unable to display input]"; + } + })();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/PermissionRequestCard.tsx` around lines 92 - 93, The current computation of inputPreview in the PermissionRequestCard component uses JSON.stringify directly on the unknown input, which can throw for circular refs or non-serializable types; wrap the JSON.stringify call in a try/catch (or extract a small safeStringify helper) to return a fallback string when stringify fails (e.g., String(input) or a fixed "[unserializable input]" message), and use that safe value for the inputPreview variable so renders won't crash when input contains BigInt/circular structures.
🧹 Nitpick comments (2)
apps/server/src/providers/claude/claude-provider.ts (2)
1167-1182: Add proper JSDoc format for exported method.Same as
resolvePermission— convert to multi-line JSDoc for consistency.📝 Proposed JSDoc format
- /** Returns all pending permission requests for the given thread, including tool input and optional title for display. Used by the frontend to re-hydrate cards after a WebSocket reconnect. */ + /** + * Returns all pending permission requests for the given thread. + * + * Includes tool input and optional title for display. Used by the frontend + * to re-hydrate cards after a WebSocket reconnect. + * + * `@param` threadId - The thread UUID to filter by. + * `@returns` Array of pending PermissionRequest objects for the thread. + */ listPendingPermissions(threadId: string): PermissionRequest[] {As per coding guidelines: "Always add JSDoc/TSDoc docstrings to all exported functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/claude/claude-provider.ts` around lines 1167 - 1182, Update the single-line comment above listPendingPermissions to a proper multi-line JSDoc/TSDoc block like the existing style used for resolvePermission: add /** on its own line, include a brief description, `@param` {string} threadId with description, and `@returns` {PermissionRequest[]} describing the returned array; ensure the JSDoc is immediately above the exported method name listPendingPermissions so tooling and docs pick it up.
1150-1165: Add proper JSDoc format for exported method.The inline comment should be converted to JSDoc format per coding guidelines for exported functions.
📝 Proposed JSDoc format
- /** Resolves a pending permission request by ID. Deletes the entry before calling resolve to prevent re-entrant calls. Returns false if the requestId is unknown. */ + /** + * Resolves a pending permission request by ID. + * + * Deletes the entry before calling resolve to prevent re-entrant calls. + * Resets the session's idle timer so eviction is deferred from the response time. + * + * `@param` requestId - The unique identifier of the permission request. + * `@param` decision - The user's decision (allow, allow-session, deny, cancelled). + * `@returns` `true` if the request was found and resolved, `false` if unknown. + */ resolvePermission(requestId: string, decision: PermissionDecision): boolean {As per coding guidelines: "Always add JSDoc/TSDoc docstrings to all exported functions... At minimum include a one-line summary of what the symbol does."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/providers/claude/claude-provider.ts` around lines 1150 - 1165, Add a JSDoc/TSDoc block above the exported method resolvePermission describing its purpose in one line and documenting parameters and return value; include `@param` for requestId (string) and decision (PermissionDecision), a short note about side effects (deletes pendingPermissions entry, resets session lastUsedAt via sessions map, calls entry.resolve and emits "permission_resolved"), and an `@returns` {boolean} describing the false/true cases. Ensure the JSDoc sits immediately above the resolvePermission method declaration and references resolvePermission, pendingPermissions, sessions, and the emitted event name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/server/src/providers/claude/claude-provider.ts`:
- Around line 967-980: In startStreamLoop's finally block (around the sessions
cleanup) add the same pending-permission draining used by stopSession so any
pending permission promises for the threadId are settled when a stream
terminates unexpectedly; specifically, after deleting
this.sessions.get(sessionId) and before emitting Ended, invoke the
permission-cleanup routine used in stopSession (or inline the same logic) to
resolve/reject and remove any entries for threadId so approval cards and
provider-side promises are not left orphaned (refer to startStreamLoop,
stopSession, sessionId, threadId, sessions, suppressEnded).
---
Duplicate comments:
In `@apps/web/src/components/chat/PermissionRequestCard.tsx`:
- Around line 92-93: The current computation of inputPreview in the
PermissionRequestCard component uses JSON.stringify directly on the unknown
input, which can throw for circular refs or non-serializable types; wrap the
JSON.stringify call in a try/catch (or extract a small safeStringify helper) to
return a fallback string when stringify fails (e.g., String(input) or a fixed
"[unserializable input]" message), and use that safe value for the inputPreview
variable so renders won't crash when input contains BigInt/circular structures.
---
Nitpick comments:
In `@apps/server/src/providers/claude/claude-provider.ts`:
- Around line 1167-1182: Update the single-line comment above
listPendingPermissions to a proper multi-line JSDoc/TSDoc block like the
existing style used for resolvePermission: add /** on its own line, include a
brief description, `@param` {string} threadId with description, and `@returns`
{PermissionRequest[]} describing the returned array; ensure the JSDoc is
immediately above the exported method name listPendingPermissions so tooling and
docs pick it up.
- Around line 1150-1165: Add a JSDoc/TSDoc block above the exported method
resolvePermission describing its purpose in one line and documenting parameters
and return value; include `@param` for requestId (string) and decision
(PermissionDecision), a short note about side effects (deletes
pendingPermissions entry, resets session lastUsedAt via sessions map, calls
entry.resolve and emits "permission_resolved"), and an `@returns` {boolean}
describing the false/true cases. Ensure the JSDoc sits immediately above the
resolvePermission method declaration and references resolvePermission,
pendingPermissions, sessions, and the emitted event name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8559f91f-4e21-42e2-80fd-7cdac691c9d4
📒 Files selected for processing (5)
apps/server/src/providers/claude/claude-provider.tsapps/web/src/components/chat/PermissionRequestCard.tsxapps/web/src/components/chat/virtual-items.tsapps/web/src/lib/thread-status.tsapps/web/src/stores/threadStore.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/lib/thread-status.ts
- apps/web/src/stores/threadStore.ts
…adcast permission.resolved
… response, and show amber dot unconditionally
…lassification and error guard to canUseTool
- allow-session now returns options.suggestions as updatedPermissions instead of hand-crafting a PermissionUpdate (root cause of validation errors for WebSearch and other tools) - volatile list only renders unsettled permission cards - permission cards cleared from store when agent run completes - split button reflects active allow mode with label and icon update
Log permissionMode/isBypass at every doSendMessage, canUseTool entry/ decision/return, signal abort state, and resolvePermission resolution. ws-router logs permission.respond receipt. These entries will pinpoint where allow decisions are lost.
…CLI Zod validation error
…settle feedback - Wrap PermissionRequestSchema in lazySchema so channels.ts and methods.ts can call it as PermissionRequestSchema() — without this the committed call sites throw TypeError at runtime - Add 600ms ready-guard on permission card buttons to prevent accidental clicks from layout shifts (card appearing under cursor) - Show settled permission cards in volatile section so users see "Allowed once" / "Denied" badge as visual confirmation; cards are cleared from state when the agent turn ends so they never trail below persisted messages
… up debug logging - Add updatedInput to canUseTool allow responses (required by CLI's runtime Zod schema despite being optional in SDK TypeScript types) - Remove duplicate permission.resolved broadcast from ws-router (the provider's emit already triggers broadcast via index.ts listener) - Listen to options.signal abort to auto-cancel leaked pending permissions when the SDK aborts a tool call - Downgrade diagnostic logger.info to logger.debug in permission flow - Remove console.log debug statements from PermissionRequestCard - Memoize JSON.stringify in PermissionRequestCard to avoid re-computing on every render - Fix thread-status test to match intentional behavior (amber dot shows regardless of running state)
chuks-qua
force-pushed
the
feat/can-use-tool
branch
from
b57f5e7 to
c0e6802
Compare
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.
What
Replace the binary bypass/default permission model with a proper
canUseToolcallback that routes tool permission requests to the mcode frontend via WebSocket, rendering an inline approval card in the chat UI.Why
In the current model,
"default"mode is unusable in mcode because permission prompts render inside the invisible Claude Code subprocess. Users either run fully bypassed or get stuck. This implements a third mode — supervised — where the user approves or denies each tool call interactively from the chat.Key Changes
PermissionRequest/PermissionDecisionschemas,permission.request/permission.resolvedpush channels,permission.respond/permission.listPendingRPC methodscanUseToolcallback with pending promise map;resolvePermission,listPendingPermissionsmethods; cleanup onstopSessionandshutdownAgentServicedelegation,ws-routerdispatch casespermissionsByThreadstate with add/resolve actions, deduplication guard, re-hydration on thread load/reconnectPermissionRequestCard: Inline chat card with split Allow/Allow-in-session button and Deny; settles to a collapsed badge when resolvedpermission-requestitem type rendered after active tool callsDesign decisions
allow-sessionuses SDK'supdatedPermissionswithdestination: "session"— no mcode-side storageCloses #156 / Related to #276
Summary by CodeRabbit