Add inline permission grant for Claude tool errors #289
Conversation
WalkthroughAdds a reversible Claude tool-permission approval flow: detects permission suggestions from Claude errors, surfaces per-message grant UI, persists remembered approvals in localStorage, and wires client grant actions to server-side in-flight approval resolution via new WebSocket messages and server APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClientUI as ChatInterface / MessageComponent
participant ServerWS as server/index.js
participant ClaudeSDK as server/claude-sdk.js
participant localStorage
User->>ClientUI: Send request invoking tool
ClientUI->>ClaudeSDK: Execute tool via server (via ServerWS)
ClaudeSDK-->>ClientUI: Tool-error with permission hint (via ServerWS)
ClientUI->>ClientUI: getClaudePermissionSuggestion(error, provider)
ClientUI->>MessageComponent: Render suggestion + "Grant" button
User->>MessageComponent: Click "Grant"
MessageComponent->>ClientUI: onGrantToolPermission(entry)
ClientUI->>localStorage: grantClaudeToolPermission(entry) (optionally remember)
localStorage-->>ClientUI: Confirm stored
ClientUI->>ServerWS: Send claude-permission-response (requestId, allow/remember)
ServerWS->>ClaudeSDK: resolveToolApproval(requestId, payload)
ClaudeSDK-->>ServerWS: Approval resolved -> resume tool execution
ServerWS-->>ClientUI: Update UI with granted status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/components/ChatInterface.jsx:
- Around line 290-298: The current logic splits command with split(/\s+/) which
yields [''] for a whitespace-only string and leads to a malformed permission
like 'Bash(:*)'; fix by trimming and validating the command before splitting:
compute a trimmed string (e.g., const trimmed = command.trim()), if trimmed is
empty return toolName, then build tokens from trimmed.split(/\s+/) and proceed
with the existing git and default Bash(...) branches so tokens[0] is never an
empty string.
🧹 Nitpick comments (3)
src/components/ChatInterface.jsx (3)
252-280: Consider reusingsafeJsonParseinsidegetClaudeSettings.
getClaudeSettingsuses its owntry/catchfor JSON parsing (line 264), butsafeJsonParse(defined above) already handles this. This creates redundant error-handling patterns.♻️ Suggested simplification
function getClaudeSettings() { const raw = safeLocalStorage.getItem(CLAUDE_SETTINGS_KEY); if (!raw) { return { allowedTools: [], disallowedTools: [], skipPermissions: false, projectSortOrder: 'name' }; } - try { - const parsed = JSON.parse(raw); + const parsed = safeJsonParse(raw); + if (!parsed) { return { - ...parsed, - allowedTools: Array.isArray(parsed.allowedTools) ? parsed.allowedTools : [], - disallowedTools: Array.isArray(parsed.disallowedTools) ? parsed.disallowedTools : [], - skipPermissions: Boolean(parsed.skipPermissions), - projectSortOrder: parsed.projectSortOrder || 'name' - }; - } catch { - return { allowedTools: [], disallowedTools: [], skipPermissions: false, projectSortOrder: 'name' }; } + return { + ...parsed, + allowedTools: Array.isArray(parsed.allowedTools) ? parsed.allowedTools : [], + disallowedTools: Array.isArray(parsed.disallowedTools) ? parsed.disallowedTools : [], + skipPermissions: Boolean(parsed.skipPermissions), + projectSortOrder: parsed.projectSortOrder || 'name' + }; }
1489-1510: Consider guarding against double-clicks on the grant button.The button is disabled when
permissionGrantState === 'granted', but the state update is asynchronous. A rapid double-click could invokeonGrantToolPermissiontwice before the state updates.♻️ Suggested guard
onClick={() => { if (!onGrantToolPermission) return; + if (permissionGrantState !== 'idle') return; const result = onGrantToolPermission(permissionSuggestion); if (result?.success) { setPermissionGrantState('granted');
1532-1536: Consider addingaria-livefor screen reader accessibility.The success message "Permission saved. Retry the request to use the tool." appears dynamically but may not be announced to screen reader users.
♿ Suggested improvement
{(permissionSuggestion.isAllowed || permissionGrantState === 'granted') && ( - <div className="mt-2 text-xs text-green-700 dark:text-green-200"> + <div className="mt-2 text-xs text-green-700 dark:text-green-200" aria-live="polite"> Permission saved. Retry the request to use the tool. </div> )}
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ChatInterface.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ChatInterface.jsx (2)
src/components/GitPanel.jsx (1)
provider(40-42)server/claude-sdk.js (1)
settings(45-49)
🔇 Additional comments (3)
src/components/ChatInterface.jsx (3)
485-490: LGTM!The permission suggestion computation and state reset logic are correctly implemented. The
useEffectproperly resetspermissionGrantStatewhen the permission entry or tool ID changes.
4280-4285: LGTM!The
handleGrantToolPermissioncallback is correctly memoized with theproviderdependency, and appropriately gates the permission grant to Claude-only.
4863-4868: LGTM!The new
onGrantToolPermissionandproviderprops are correctly passed toMessageComponent.
| const tokens = command.split(/\s+/); | ||
| if (tokens.length === 0) return toolName; | ||
|
|
||
| // For Bash, allow the command family instead of every Bash invocation. | ||
| if (tokens[0] === 'git' && tokens[1]) { | ||
| return `Bash(${tokens[0]} ${tokens[1]}:*)`; | ||
| } | ||
| return `Bash(${tokens[0]}:*)`; | ||
| } |
There was a problem hiding this comment.
Edge case: empty command token produces malformed permission entry.
If command contains only whitespace (e.g., ' '), split(/\s+/) returns ['']. The check tokens.length === 0 passes, but tokens[0] is an empty string, producing 'Bash(:*)' which is likely invalid.
🐛 Suggested fix
const tokens = command.split(/\s+/);
- if (tokens.length === 0) return toolName;
+ if (tokens.length === 0 || !tokens[0]) return toolName;
// For Bash, allow the command family instead of every Bash invocation.
if (tokens[0] === 'git' && tokens[1]) {🤖 Prompt for AI Agents
In @src/components/ChatInterface.jsx around lines 290 - 298, The current logic
splits command with split(/\s+/) which yields [''] for a whitespace-only string
and leads to a malformed permission like 'Bash(:*)'; fix by trimming and
validating the command before splitting: compute a trimmed string (e.g., const
trimmed = command.trim()), if trimmed is empty return toolName, then build
tokens from trimmed.split(/\s+/) and proceed with the existing git and default
Bash(...) branches so tokens[0] is never an empty string.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/ChatInterface.jsx (3)
239-280: Claude settings helpers and grant flow look correct; consider minor cleanupThe
CLAUDE_SETTINGS_KEY+getClaudeSettings+grantClaudeToolPermissiontrio correctly:
- Reads/writes the same
claude-settingskey used elsewhere.- Normalizes
allowedTools/disallowedToolsarrays and safely tolerates malformed or missing JSON.- Updates
allowedToolsand prunesdisallowedToolsin a single write, which matches the server-sidetoolsSettingsshape.Two small, non-blocking refinements you might consider:
getClaudeSettingsreimplements JSON parsing instead of usingsafeJsonParse; not a bug, but you could reusesafeJsonParsehere to centralize error handling.- The literal
'claude-settings'still appears ingetToolsSettingsinsidehandleSubmit; if you ever rename this key, it would be easy to miss one place. Pulling that usage throughCLAUDE_SETTINGS_KEYwould remove this drift risk.Also applies to: 339-355
282-308: Bash permission scoping logic is solid; a couple of tiny robustness nitsThe Bash handling does what the PR promises:
toolInputis parsed viasafeJsonParse, andparsed.command.trim()plus the!commandguard neatly handles empty/whitespace-only inputs (addressing the earlier edge case).- Scoping entries to
Bash(git <subcmd>:*)for git, andBash(<cmd>:*)otherwise, keeps permissions narrow.Minor optional tweaks:
- After the
trim()+!commandguard,tokens.length === 0is effectively unreachable and could be dropped.- If you expect patterns like
sudo git log ..., you might later want to strip a small set of known wrappers (sudo,time, etc.) before thegitspecial-case so that both the entry and label reflect the underlying command family instead ofsudo. Not required for correctness today, just future-proofing.
310-337: Permission suggestion + handler wiring matches requirements; behavior is consistentThe end-to-end flow looks coherent:
getClaudePermissionSuggestion:
- Correctly gates on
provider === 'claude'andmessage.toolResult.isError, so you won’t show the UI on non-Claude providers or non-error results.- Uses the regex match first, then falls back to
message.toolName/Bash approval text, and only returns a suggestion when a concreteentryis built.- Checks
allowedTools.includes(entry)so pre‑allowed entries render as “Permission added” with a disabled button.
handleGrantToolPermission:
- Is scoped to Claude via the provider check and delegates to
grantClaudeToolPermission, which updatesclaude-settingsin localStorage in the same shape thatgetToolsSettingslater reads for tool invocations.
MessageComponent:
- Derives
permissionSuggestionfrom the message + provider and keeps a localpermissionGrantStateso the UI can show “Permission added” / error messaging without re-parsing content.- Resets
permissionGrantStatewhenpermissionSuggestion.entryormessage.toolIdchanges, which handles retried tool calls cleanly.- Disables the button once permission is already allowed or just granted, and shows the “Permission saved. Retry the request to use the tool.” text, which aligns with the PR’s UX spec.
I don’t see functional issues here; the string-pattern detection is understandably a bit coupled to backend error phrasing, but that’s acceptable given both ends live in this repo.
Also applies to: 4314-4319, 4899-4905
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ChatInterface.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ChatInterface.jsx (2)
src/components/GitPanel.jsx (1)
provider(40-42)server/claude-sdk.js (1)
settings(45-49)
🔇 Additional comments (1)
src/components/ChatInterface.jsx (1)
1486-1537: Inline permission UI copy and state transitions are clearThe inline block in the tool error card:
- Clearly states what will be added (
{permissionSuggestion.entry}) and when the permission has been saved.- Handles the three interesting states (initial, granted, error) without letting the user double‑submit.
- Provides a direct link into Settings via
onShowSettings, which is a nice escape hatch.No changes needed from a correctness standpoint.
…https://github.com/siteboon/claudecodeui into feat/show-grant-permission-button-in-chat-for-claude
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/ChatInterface.jsx (2)
250-278: Consider reusingsafeJsonParsefor consistency.The function uses
JSON.parse(raw)directly at line 262, butsafeJsonParsewas just defined above. While the outer try-catch handles failures, usingsafeJsonParsewould be more consistent.♻️ Suggested refactor
function getClaudeSettings() { const raw = safeLocalStorage.getItem(CLAUDE_SETTINGS_KEY); if (!raw) { return { allowedTools: [], disallowedTools: [], skipPermissions: false, projectSortOrder: 'name' }; } - try { - const parsed = JSON.parse(raw); + const parsed = safeJsonParse(raw); + if (!parsed) { return { - ...parsed, - allowedTools: Array.isArray(parsed.allowedTools) ? parsed.allowedTools : [], - disallowedTools: Array.isArray(parsed.disallowedTools) ? parsed.disallowedTools : [], - skipPermissions: Boolean(parsed.skipPermissions), - projectSortOrder: parsed.projectSortOrder || 'name' + allowedTools: [], + disallowedTools: [], + skipPermissions: false, + projectSortOrder: 'name' }; - } catch { - return { - allowedTools: [], - disallowedTools: [], - skipPermissions: false, - projectSortOrder: 'name' - }; } + + return { + ...parsed, + allowedTools: Array.isArray(parsed.allowedTools) ? parsed.allowedTools : [], + disallowedTools: Array.isArray(parsed.disallowedTools) ? parsed.disallowedTools : [], + skipPermissions: Boolean(parsed.skipPermissions), + projectSortOrder: parsed.projectSortOrder || 'name' + }; }
458-463: MemoizepermissionSuggestionto avoid repeated localStorage reads.
getClaudePermissionSuggestioncallsgetClaudeSettings()which reads from localStorage on every render. For a component that may re-render frequently during message streaming, this is unnecessary overhead.♻️ Suggested refactor using useMemo
- const permissionSuggestion = getClaudePermissionSuggestion(message, provider); + const permissionSuggestion = React.useMemo( + () => getClaudePermissionSuggestion(message, provider), + [message?.toolResult?.isError, message?.toolName, message?.toolInput, provider] + );
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ChatInterface.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ChatInterface.jsx (2)
src/components/GitPanel.jsx (1)
provider(40-42)server/claude-sdk.js (1)
settings(45-49)
🔇 Additional comments (6)
src/components/ChatInterface.jsx (6)
239-248: LGTM on the utility functions.The
safeJsonParsehelper is a good defensive pattern for handling potentially malformed JSON from localStorage.
280-296: LGTM on the permission entry builder.The Bash command scoping logic correctly extracts the command family (e.g.,
Bash(git log:*)for git commands with subcommands). The defensive parsing withsafeJsonParseand optional chaining handles edge cases well.One minor note: Line 289's check
if (tokens.length === 0)is unreachable since line 286 already returns whencommandis empty (andsplit(/\s+/)on an empty string produces[""], not[]). Consider removing the redundant check.
312-328: LGTM on the permission grant function.The function correctly:
- Prevents duplicate entries in
allowedTools- Removes the entry from
disallowedToolsto avoid conflicts- Adds a
lastUpdatedtimestamp for tracking- Returns meaningful status for UI feedback
1459-1511: Well-designed permission UI with clear state feedback.The implementation provides:
- Clear visual distinction between idle, granted, and error states
- Transparency by showing the exact permission entry being added
- Quick access to settings for verification
- Helpful guidance text for retry after granting
Good UX design.
4287-4292: LGTM on the callback handler.The guard clause for
provider !== 'claude'is defensive (sincegetClaudePermissionSuggestionalready filters by provider), but adds an extra layer of safety.
4872-4877: LGTM on props wiring.The
onGrantToolPermissionandproviderprops are correctly passed toMessageComponent, enabling the permission flow.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/components/ChatInterface.jsx:
- Around line 3435-3457: The permission prompt can appear across sessions
because claude-permission-request stores sessionId as latestMessage.sessionId ||
null; update the handling in the case 'claude-permission-request' inside
setPendingPermissionRequests to attach a best-effort sessionId (use the current
selectedSessionId or equivalent UI state) when latestMessage.sessionId is
missing, so the stored object uses either latestMessage.sessionId or
selectedSessionId instead of null; ensure you still preserve existing fields
(requestId, toolName, input, context, receivedAt) and keep the duplicate-check
logic based on requestId unchanged.
- Around line 318-334: grantClaudeToolPermission currently assumes the call to
safeLocalStorage.setItem(CLAUDE_SETTINGS_KEY, ...) always succeeds; change it to
attempt the write inside try/catch, then read back with
safeLocalStorage.getItem(CLAUDE_SETTINGS_KEY) and JSON.parse to verify the
stored settings match updatedSettings (comparing serialized or key-wise), and
only return { success: true, alreadyAllowed, updatedSettings } if the
verification passes; on parse errors, read mismatch, or thrown exceptions return
{ success: false, error: <brief message> } and do not report “Permission saved”;
apply the same read-back verification pattern to the analogous code around the
other occurrence (lines ~1506-1515) that writes CLAUDE_SETTINGS_KEY.
- Around line 304-316: Update getClaudePermissionSuggestion to only show the
grant-permission UI when the tool error is a permission-denied error: return
null unless message.toolResult indicates permission denial by checking common
markers (e.g. message.toolResult.permissionDenied === true,
message.toolResult.errorCode === 'PERMISSION_DENIED', message.toolResult.status
=== 403, or message.toolResult.message includes "permission" / "not
authorized"); also handle missing toolName by falling back to
message.toolResult?.toolName or deriving it from message?.metadata or
message.toolInput before calling buildClaudeToolPermissionEntry; keep using
buildClaudeToolPermissionEntry and getClaudeSettings to compute entry/isAllowed,
and apply the same guard to the other occurrence referenced (around lines
1465-1517).
🧹 Nitpick comments (3)
server/index.js (1)
807-818: LGTM! Consider adding a warning log for missingrequestId.The handler correctly relays UI approval decisions to the SDK. The
Boolean(data.allow)coercion safely handles undefined/null values.One optional improvement: logging when
requestIdis missing could aid debugging if the UI sends malformed messages.🔧 Optional: Add debug logging for missing requestId
} else if (data.type === 'claude-permission-response') { // Relay UI approval decisions back into the SDK control flow. // This does not persist permissions; it only resolves the in-flight request, // introduced so the SDK can resume once the user clicks Allow/Deny. if (data.requestId) { resolveToolApproval(data.requestId, { allow: Boolean(data.allow), updatedInput: data.updatedInput, message: data.message, rememberEntry: data.rememberEntry }); + } else { + console.warn('[WARN] claude-permission-response received without requestId'); }src/components/ChatInterface.jsx (2)
273-289: HardenbuildClaudeToolPermissionEntry()against odd Bash inputs (sanitization + common prefixes).
Todaytokens[0]/tokens[1]can produce surprising allow rules for inputs likesudo git ..., quoted commands, or commands containing characters that may not be valid in your allowlist grammar.Possible tweak (strip prefixes + sanitize token used in allow rule)
function buildClaudeToolPermissionEntry(toolName, toolInput) { if (!toolName) return null; if (toolName !== 'Bash') return toolName; const parsed = safeJsonParse(toolInput); const command = typeof parsed?.command === 'string' ? parsed.command.trim() : ''; if (!command) return toolName; const tokens = command.split(/\s+/); if (tokens.length === 0) return toolName; + + const stripPrefixes = (toks) => { + const prefixes = new Set(['sudo', 'env', 'command']); + while (toks.length && prefixes.has(toks[0])) toks = toks.slice(1); + return toks; + }; + const sanitize = (s) => String(s).replace(/[^A-Za-z0-9_.-]/g, ''); + + const normalized = stripPrefixes(tokens); + if (normalized.length === 0) return toolName; + const cmd0 = sanitize(normalized[0]); + const cmd1 = normalized[1] ? sanitize(normalized[1]) : ''; + if (!cmd0) return toolName; // For Bash, allow the command family instead of every Bash invocation. - if (tokens[0] === 'git' && tokens[1]) { - return `Bash(${tokens[0]} ${tokens[1]}:*)`; + if (cmd0 === 'git' && cmd1) { + return `Bash(${cmd0} ${cmd1}:*)`; } - return `Bash(${tokens[0]}:*)`; + return `Bash(${cmd0}:*)`; }
5055-5078: Optional: reduce repeated localStorage parses + O(n²) batching computation in the banner.
Inside.map(), each row callsgetClaudeSettings()and recomputesmatchingRequestIdsby scanningpendingPermissionRequestsand re-parsing tool inputs repeatedly. Consider memoizingsettingsonce per render, and precomputingrequestId[]buckets bypermissionEntry.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/claude-sdk.jsserver/index.jssrc/components/ChatInterface.jsxsrc/lib/utils.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ChatInterface.jsx (1)
src/lib/utils.js (1)
safeJsonParse(8-15)
🔇 Additional comments (11)
src/lib/utils.js (1)
8-15: LGTM!Clean utility function with proper input validation and error handling. The optional catch binding (
catch {}) is valid ES2019+ syntax and keeps the code concise.server/index.js (1)
61-61: LGTM!Import addition aligns with the new
resolveToolApprovalexport fromclaude-sdk.js.server/claude-sdk.js (9)
16-18: LGTM!Standard Node.js crypto import for UUID generation. The comment clearly documents that this is for collision avoidance, not cryptographic security.
26-34: LGTM!The 55-second default timeout appropriately stays under the SDK's 60-second control timeout. Environment variable override provides operational flexibility.
36-44: LGTM!Good fallback pattern for Node.js version compatibility. Both
randomUUID()andrandomBytes(16)provide sufficient uniqueness for in-process request tracking.
46-98: LGTM!Solid implementation with proper cleanup to prevent memory leaks. The
settledflag correctly prevents race conditions between timeout, cancellation, and UI response paths. Signal handling properly checkssignal.abortedbefore adding the listener.
100-108: LGTM!Lightweight relay function. Silent no-op for missing requests is appropriate since the request may have already timed out or been cancelled.
110-142: LGTM!Clean implementation that correctly handles both exact tool matches and the
Bash(command:*)shorthand. The prefix-based command matching aligns with the UI's permission entry format. Properly handles both string and object input formats for Bash commands.
175-196: LGTM!Explicitly setting
allowedToolsanddisallowedToolsarrays prevents the SDK from interpreting undefined as "all tools allowed." The plan mode tools are correctly added only when not already present.
492-560: Well-structured permission flow.The hook correctly prioritizes: bypass mode → disallowed check → allowed check → UI approval. The in-memory
rememberEntryaddition prevents repeated prompts within the same session while delegating persistence to the UI.One observation: the
disallowedToolsfilter at line 553 uses exact string matching (entry !== decision.rememberEntry), whilematchesToolPermissionuses pattern matching. IfdisallowedToolscontains a broad entry like"Bash"andrememberEntryis"Bash(git:*)", the broad entry won't be removed. This appears intentional—the UI is responsible for building appropriaterememberEntryvalues—but worth noting for future maintainers.
718-719: LGTM!Export addition aligns with the import in
server/index.js.
| function getClaudePermissionSuggestion(message, provider) { | ||
| if (provider !== 'claude') return null; | ||
| if (!message?.toolResult?.isError) return null; | ||
|
|
||
| const toolName = message?.toolName; | ||
| const entry = buildClaudeToolPermissionEntry(toolName, message.toolInput); | ||
|
|
||
| if (!entry) return null; | ||
|
|
||
| const settings = getClaudeSettings(); | ||
| const isAllowed = settings.allowedTools.includes(entry); | ||
| return { toolName, entry, isAllowed }; | ||
| } |
There was a problem hiding this comment.
Gate the inline “Grant permission” UI to permission-denied errors (right now it shows for any tool error).
getClaudePermissionSuggestion() only checks message.toolResult.isError, so the button/banner will appear on all tool failures (network errors, tool crashes, bad inputs), which is misleading and increases the chance of users granting overly broad permissions.
Proposed fix (add a permission-denied detector + optional toolName fallback)
+function isClaudePermissionDeniedError(toolResultContent) {
+ const text = String(toolResultContent ?? '');
+ // TODO: tighten to Claude's exact denial wording/codes once confirmed.
+ return /permission|not allowed|denied|unauthorized/i.test(text);
+}
+
+function parseToolNameFromPermissionError(toolResultContent) {
+ const text = String(toolResultContent ?? '');
+ // Example fallback: "You don't have permission to use tool: WebSearch"
+ const match = text.match(/tool:\s*([A-Za-z0-9_:-]+)/i);
+ return match?.[1] || null;
+}
+
function getClaudePermissionSuggestion(message, provider) {
if (provider !== 'claude') return null;
if (!message?.toolResult?.isError) return null;
- const toolName = message?.toolName;
+ if (!isClaudePermissionDeniedError(message.toolResult?.content)) return null;
+
+ const toolName = message?.toolName || parseToolNameFromPermissionError(message.toolResult?.content);
const entry = buildClaudeToolPermissionEntry(toolName, message.toolInput);
if (!entry) return null;
const settings = getClaudeSettings();
const isAllowed = settings.allowedTools.includes(entry);
return { toolName, entry, isAllowed };
}Also applies to: 1465-1517
🤖 Prompt for AI Agents
In @src/components/ChatInterface.jsx around lines 304 - 316, Update
getClaudePermissionSuggestion to only show the grant-permission UI when the tool
error is a permission-denied error: return null unless message.toolResult
indicates permission denial by checking common markers (e.g.
message.toolResult.permissionDenied === true, message.toolResult.errorCode ===
'PERMISSION_DENIED', message.toolResult.status === 403, or
message.toolResult.message includes "permission" / "not authorized"); also
handle missing toolName by falling back to message.toolResult?.toolName or
deriving it from message?.metadata or message.toolInput before calling
buildClaudeToolPermissionEntry; keep using buildClaudeToolPermissionEntry and
getClaudeSettings to compute entry/isAllowed, and apply the same guard to the
other occurrence referenced (around lines 1465-1517).
| function grantClaudeToolPermission(entry) { | ||
| if (!entry) return { success: false }; | ||
|
|
||
| const settings = getClaudeSettings(); | ||
| const alreadyAllowed = settings.allowedTools.includes(entry); | ||
| const nextAllowed = alreadyAllowed ? settings.allowedTools : [...settings.allowedTools, entry]; | ||
| const nextDisallowed = settings.disallowedTools.filter(tool => tool !== entry); | ||
| const updatedSettings = { | ||
| ...settings, | ||
| allowedTools: nextAllowed, | ||
| disallowedTools: nextDisallowed, | ||
| lastUpdated: new Date().toISOString() | ||
| }; | ||
|
|
||
| safeLocalStorage.setItem(CLAUDE_SETTINGS_KEY, JSON.stringify(updatedSettings)); | ||
| return { success: true, alreadyAllowed, updatedSettings }; | ||
| } |
There was a problem hiding this comment.
Don’t report “Permission saved” unless the localStorage write actually stuck.
grantClaudeToolPermission() always returns { success: true } after calling safeLocalStorage.setItem(...), even if the write failed or was dropped; the UI then says “Permission saved”.
Proposed fix (read-back verification)
function grantClaudeToolPermission(entry) {
if (!entry) return { success: false };
const settings = getClaudeSettings();
const alreadyAllowed = settings.allowedTools.includes(entry);
const nextAllowed = alreadyAllowed ? settings.allowedTools : [...settings.allowedTools, entry];
const nextDisallowed = settings.disallowedTools.filter(tool => tool !== entry);
const updatedSettings = {
...settings,
allowedTools: nextAllowed,
disallowedTools: nextDisallowed,
lastUpdated: new Date().toISOString()
};
safeLocalStorage.setItem(CLAUDE_SETTINGS_KEY, JSON.stringify(updatedSettings));
- return { success: true, alreadyAllowed, updatedSettings };
+ const verify = getClaudeSettings();
+ const persisted = Array.isArray(verify.allowedTools) && verify.allowedTools.includes(entry);
+ return { success: persisted, alreadyAllowed, updatedSettings };
}Also applies to: 1506-1515
🤖 Prompt for AI Agents
In @src/components/ChatInterface.jsx around lines 318 - 334,
grantClaudeToolPermission currently assumes the call to
safeLocalStorage.setItem(CLAUDE_SETTINGS_KEY, ...) always succeeds; change it to
attempt the write inside try/catch, then read back with
safeLocalStorage.getItem(CLAUDE_SETTINGS_KEY) and JSON.parse to verify the
stored settings match updatedSettings (comparing serialized or key-wise), and
only return { success: true, alreadyAllowed, updatedSettings } if the
verification passes; on parse errors, read mismatch, or thrown exceptions return
{ success: false, error: <brief message> } and do not report “Permission saved”;
apply the same read-back verification pattern to the analogous code around the
other occurrence (lines ~1506-1515) that writes CLAUDE_SETTINGS_KEY.
| case 'claude-permission-request': { | ||
| // Receive a tool approval request from the backend and surface it in the UI. | ||
| // This does not approve anything automatically; it only queues a prompt, | ||
| // introduced so the user can decide before the SDK continues. | ||
| if (provider !== 'claude' || !latestMessage.requestId) { | ||
| break; | ||
| } | ||
|
|
||
| setPendingPermissionRequests(prev => { | ||
| if (prev.some(req => req.requestId === latestMessage.requestId)) { | ||
| return prev; | ||
| } | ||
| return [ | ||
| ...prev, | ||
| { | ||
| requestId: latestMessage.requestId, | ||
| toolName: latestMessage.toolName || 'UnknownTool', | ||
| input: latestMessage.input, | ||
| context: latestMessage.context, | ||
| sessionId: latestMessage.sessionId || null, | ||
| receivedAt: new Date() | ||
| } | ||
| ]; |
There was a problem hiding this comment.
Avoid cross-session permission prompts when sessionId is missing.
claude-permission-request stores sessionId: latestMessage.sessionId || null, and the session filter keeps !req.sessionId prompts for any selected session (Line 1936), so banners can appear in unrelated sessions if backend ever omits sessionId.
Proposed fix (attach a best-effort sessionId on receipt)
setPendingPermissionRequests(prev => {
if (prev.some(req => req.requestId === latestMessage.requestId)) {
return prev;
}
return [
...prev,
{
requestId: latestMessage.requestId,
toolName: latestMessage.toolName || 'UnknownTool',
input: latestMessage.input,
context: latestMessage.context,
- sessionId: latestMessage.sessionId || null,
+ sessionId: latestMessage.sessionId || currentSessionId || selectedSession?.id || null,
receivedAt: new Date()
}
];
});Also applies to: 1932-1937
🤖 Prompt for AI Agents
In @src/components/ChatInterface.jsx around lines 3435 - 3457, The permission
prompt can appear across sessions because claude-permission-request stores
sessionId as latestMessage.sessionId || null; update the handling in the case
'claude-permission-request' inside setPendingPermissionRequests to attach a
best-effort sessionId (use the current selectedSessionId or equivalent UI state)
when latestMessage.sessionId is missing, so the stored object uses either
latestMessage.sessionId or selectedSessionId instead of null; ensure you still
preserve existing fields (requestId, toolName, input, context, receivedAt) and
keep the duplicate-check logic based on requestId unchanged.
| type="button" | ||
| onClick={() => { | ||
| if (permissionEntry && !alreadyAllowed) { | ||
| handleGrantToolPermission({ entry: permissionEntry, toolName: request.toolName }); | ||
| } | ||
| handlePermissionDecision(matchingRequestIds, { allow: true, rememberEntry: permissionEntry }); | ||
| }} | ||
| className={`inline-flex items-center gap-2 rounded-md text-xs font-medium px-3 py-1.5 border transition-colors ${ | ||
| permissionEntry | ||
| ? 'border-amber-300 text-amber-800 hover:bg-amber-100 dark:border-amber-700 dark:text-amber-100 dark:hover:bg-amber-900/30' | ||
| : 'border-gray-300 text-gray-400 cursor-not-allowed' | ||
| }`} | ||
| disabled={!permissionEntry} | ||
| > | ||
| {rememberLabel} | ||
| </button> |
There was a problem hiding this comment.
“Allow & remember” should only send rememberEntry if the local grant succeeded (and surface failure).
Currently the banner will: (1) attempt local grant, (2) always send rememberEntry to the backend, even if local persistence failed. That can desync UI vs backend expectations and mislead users.
Proposed fix (gate rememberEntry on grant result)
<button
type="button"
onClick={() => {
- if (permissionEntry && !alreadyAllowed) {
- handleGrantToolPermission({ entry: permissionEntry, toolName: request.toolName });
- }
- handlePermissionDecision(matchingRequestIds, { allow: true, rememberEntry: permissionEntry });
+ let grantOk = true;
+ if (permissionEntry && !alreadyAllowed) {
+ const res = handleGrantToolPermission({ entry: permissionEntry, toolName: request.toolName });
+ grantOk = Boolean(res?.success);
+ }
+ handlePermissionDecision(matchingRequestIds, {
+ allow: true,
+ rememberEntry: grantOk ? permissionEntry : undefined
+ });
}}Also applies to: 4387-4415
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server/claude-sdk.js (3)
39-45: Consider documenting Node.js version requirements.The fallback logic correctly handles older Node.js versions that lack
crypto.randomUUID(), but the comment could be more specific about which versions require the fallback (Node.js < 14.17.0 or < 15.6.0).
497-561: Consider defensive checks for WebSocket validity.The
canUseToolhook callsws.send()at lines 517 and 530 without checking ifwsis valid. Whilewsis a required parameter inqueryClaudeSDK, adding defensive checks would prevent runtime errors if the function is called with an invalid WebSocket instance.🛡️ Proposed defensive check
const requestId = createRequestId(); +if (!ws || typeof ws.send !== 'function') { + return { behavior: 'deny', message: 'WebSocket unavailable for permission request' }; +} ws.send({ type: 'claude-permission-request', requestId,
31-34: Consider validating timeout against SDK limits.While the default 55s timeout is correctly under the SDK's 60s limit, the environment variable
CLAUDE_TOOL_APPROVAL_TIMEOUT_MScould be misconfigured to exceed this limit. Adding validation or a warning would improve operational clarity.📋 Proposed validation
const TOOL_APPROVAL_TIMEOUT_MS = parseInt(process.env.CLAUDE_TOOL_APPROVAL_TIMEOUT_MS, 10) || 55000; +if (TOOL_APPROVAL_TIMEOUT_MS >= 60000) { + console.warn(`CLAUDE_TOOL_APPROVAL_TIMEOUT_MS (${TOOL_APPROVAL_TIMEOUT_MS}ms) exceeds SDK's 60s timeout. Using 55000ms instead.`); + TOOL_APPROVAL_TIMEOUT_MS = 55000; +}
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/claude-sdk.js
🔇 Additional comments (5)
server/claude-sdk.js (5)
47-99: LGTM: Robust approval waiting mechanism.The implementation correctly handles timeout, cancellation, and cleanup with proper safeguards against double-resolution and resource leaks. The use of the
settledflag prevents race conditions between timeout, abort, and UI response.
111-143: LGTM: Permission matching logic is correct.The function correctly handles exact tool name matching and the Bash(command:*) shorthand format. Input type handling (string vs object) is appropriate, and the regex pattern is safe from injection.
178-197: LGTM: Explicit initialization prevents implicit SDK defaults.The changes correctly initialize
allowedToolsanddisallowedToolsas explicit arrays to avoid the SDK treatingundefinedas permissive. The plan mode tool additions are properly deduplicated.
546-558: LGTM: In-memory allowlist updates are correct.The logic correctly updates the session's in-memory permission lists to prevent repeated prompts for the same tool. The defensive type checks and the comment clarifying that persistence is UI-handled are appropriate.
15-15: No action needed — SDK version 0.1.29 supports thecanUseToolcallback.The
@anthropic-ai/claude-agent-sdkv0.1.29 includes thecanUseTooloption, and the implementation correctly uses the callback with the expected signature(toolName, input, context)and return value structure withbehavior,updatedInput, andmessageproperties.
#285
Summary
claude-settingsso it appears in the Permissions UI and is used on the next run.Bash(git log:*)) to avoid over-broad allows.Changes
allowedTools, removes fromdisallowedToolsif present).How to Test
WebSearchwithout it in Allowed Tools).Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.