Conversation
…tool calls - Add session-scoped permission cache in ToolManager (memory-only) - Support remember=false for session-level permissions - Add preCheckToolPermission() chain: ToolManager -> MCPPresenter -> ToolPresenter -> ToolCallProcessor - Pause execution flow when any tool requires permission, returning all permission requests at once - Update grantPermission signature to support session-only permissions
📝 WalkthroughWalkthroughAdds an edit_file filesystem tool, a background process execution manager, and a comprehensive batch permission pre-check/resume flow with session-scoped permission caching and UI integration; wires pre-check APIs across presenters, updates session/runtime locking, and adds related tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer/UI
participant Presenter as ToolPresenter / McpPresenter
participant ToolMgr as AgentToolManager / ToolManager
participant Session as SessionManager
participant Permission as PermissionHandler
participant Executor as Tool Executor / BackgroundExecSessionManager
UI->>Presenter: request tool / user grants permission
Presenter->>ToolMgr: preCheckToolPermission(MCPToolCall)
ToolMgr-->>Presenter: PermissionRequestPayload or null
alt permission required (batch)
Presenter->>Permission: emit batch permission-required (persist pendingPermissions)
Permission->>Session: addPendingPermission(messageId, toolCallId,...)
Permission->>UI: stream:permission-updated
UI->>Permission: user grants/denies
Permission->>Session: updatePermissionBlocks(granted)
Permission->>Permission: acquire permissionResumeLock(messageId)
Permission->>Executor: execute allowed tool_calls sequentially
Executor-->>Permission: tool result / running session meta
Permission->>Session: flushStreamUpdates(messageId)
Permission->>UI: stream events for tool outcomes
else no permission required
Presenter->>Executor: execute tool immediately
Executor-->>Presenter: result
Presenter->>UI: stream events
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/presenter/mcpPresenter/toolManager.ts (1)
226-226:⚠️ Potential issue | 🟡 MinorChinese comments violate the coding guidelines.
Lines 226, 273, 289, 297, 303, 571, 598, 621, 651–652, 661, 674 contain Chinese comments. As per coding guidelines, all logs and comments in
**/*.{js,ts,tsx,jsx,vue,mjs,cjs}files must be in English.Also applies to: 273-273, 289-289, 297-297, 303-303, 571-571, 598-598, 621-621, 651-652, 661-661, 674-674
src/main/presenter/agentPresenter/acp/agentToolManager.ts (1)
1103-1133:⚠️ Potential issue | 🔴 Critical
edit_filemissing fromcollectWriteTargets— write-permission check bypassed.
assertWritePermissionat line 995 callscollectWriteTargetsforedit_file, butcollectWriteTargetshas nocase 'edit_file'branch and falls through todefault, returning[]. This causes the permission assertion to silently return, allowingedit_fileto modify files outside the allowed directories without triggering a permission prompt.🐛 Add edit_file to collectWriteTargets
case 'text_replace': { const pathArg = args.path return typeof pathArg === 'string' ? [pathArg] : [] } + case 'edit_file': { + const pathArg = args.path + return typeof pathArg === 'string' ? [pathArg] : [] + } case 'move_files': {src/main/presenter/agentPresenter/permission/permissionHandler.ts (1)
861-917:⚠️ Potential issue | 🔴 Critical
restartAgentLoopAfterPermissiondoes not acquire the permission resume lock before delegating toresumeToolExecutionAfterPermissions.
resumeToolExecutionAfterPermissionsvalidates the lock at line 466 and returns early if no lock is held. SincerestartAgentLoopAfterPermissionnever callsacquirePermissionResumeLock, every call to this method will silently bail out at the lock-validation step, making the resume a no-op.Either acquire the lock before calling
resumeToolExecutionAfterPermissions, or make the lock check insideresumeToolExecutionAfterPermissionsconditional.🐛 Acquire lock before delegating
async restartAgentLoopAfterPermission(messageId: string, toolCallId?: string): Promise<void> { console.log('[PermissionHandler] Restarting agent loop after permission', messageId) try { const message = await this.ctx.messageManager.getMessage(messageId) if (!message) { throw new Error(`Message not found (${messageId})`) } + const conversationId = message.conversationId + const lockAcquired = presenter.sessionManager.acquirePermissionResumeLock( + conversationId, + messageId + ) + if (!lockAcquired) { + console.log('[PermissionHandler] Resume already in progress for this message, skipping') + return + } + // ... rest of the method
🤖 Fix all issues with AI agents
In `@docs/specs/edit-file-tool/plan.md`:
- Around line 46-55: The normalization currently builds args with computed
fields first then spreads ...args, which re-overwrites path/oldText/newText with
original undefined values; change the construction so you spread the original
args first and then set the computed fields last (e.g., args = { ...args, path:
args.path ?? args.file_path, oldText: args.oldText ?? args.old_string, newText:
args.newText ?? args.new_string, base_directory: args.base_directory }) so the
computed path/file_path, oldText/old_string and newText/new_string wins and
base_directory is preserved; update the block that checks toolName ===
'edit_file' and assigns to args accordingly.
In `@src/main/presenter/agentPresenter/acp/agentFileSystemHandler.ts`:
- Around line 1108-1114: The pre-check currently uses raw
content.includes(oldText) but the replacement works on
normalizedContent/normalizedOldText, so normalize the file content and the
oldText before the existence check: after reading content from
fs.readFile(validPath, 'utf-8') produce normalizedContent and normalizedOldText
(same normalization used by the replacement logic) and then perform
normalizedContent.includes(normalizedOldText); if not found, throw the same
Error. Update references in the check to normalizedContent and normalizedOldText
so line ending differences don't cause false negatives.
In `@src/main/presenter/agentPresenter/acp/agentToolManager.ts`:
- Around line 69-83: Duplicate Zod schemas exist: private processToolSchema and
fileSystemSchemas.process; remove the duplicate and consolidate by using a
single source. Replace the private field processToolSchema with a reference to
fileSystemSchemas.process (or delete fileSystemSchemas.process and point
fileSystemSchemas.process = this.processToolSchema) and update all usages
(notably callProcessTool and getFileSystemToolDefinitions) to reference the
single canonical schema (fileSystemSchemas.process or processToolSchema
depending on which you keep) so validation and schema lookups use the same
object.
In `@src/main/presenter/agentPresenter/acp/backgroundExecSessionManager.ts`:
- Around line 407-440: The appendOutput method currently uses blocking
fs.appendFileSync/writeFileSync inside stdout/stderr handlers; replace these
sync calls by creating and reusing a per-session fs.WriteStream (e.g., add
session.outputStream) or use fs.promises with a small write-queue so writes are
non-blocking: on first time session.outputFilePath is set, create
session.outputStream = fs.createWriteStream(session.outputFilePath, { flags: 'a'
}); when shouldOffload is true, write via
session.outputStream.write(session.outputBuffer or data) (and clear outputBuffer
after draining) and handle stream 'error' events to fall back to appending to
session.outputBuffer; remove appendFileSync and writeFileSync and ensure
totalOutputLength, outputBuffer and outputFilePath logic in appendOutput and
BackgroundSession are updated accordingly.
- Around line 567-575: The readFromFile implementation currently calls
fs.readFileSync and loads the entire file, causing memory spikes for large
outputs; change it to open the file descriptor (fs.openSync or async equivalent)
and use a positional read (fs.readSync) into a sized Buffer so you only read the
requested range, mirroring the approach used by readLastCharsFromFile; ensure
you handle the offset/limit semantics (clarify/convert between character vs byte
offsets) and close the descriptor after reading, and preserve the existing error
handling/logging in readFromFile.
In `@src/main/presenter/agentPresenter/permission/permissionHandler.ts`:
- Around line 677-691: In executeSingleToolCall, guard against getMessage
returning null before accessing message.content: after calling
this.ctx.messageManager.getMessage(state.message.id) check if message is null
(or undefined) and handle it (e.g., log/warn and return false or otherwise abort
the tool call) so the subsequent cast to AssistantMessageBlock[] and the
permissionBlock lookup are not performed on a null object; update references in
this method (message, message.content, permissionBlock) accordingly to avoid the
TypeError.
- Around line 456-463: The null-guard is ineffective because
presenter.sessionManager.getSession(conversationId) is async—either await the
call (const session = await presenter.sessionManager.getSession(conversationId))
so the if (!session) check works and you only call
presenter.sessionManager.releasePermissionResumeLock(conversationId) on actual
null, or if the intent is to not create a session use the synchronous lookup
method (presenter.sessionManager.getSessionSync(conversationId)) instead; update
the code paths using getSession/getSessionSync accordingly to ensure the lock is
held and released correctly when no session exists.
In `@src/main/presenter/mcpPresenter/toolManager.ts`:
- Around line 651-672: The checkSessionPermission incorrectly treats any server
permission as blanket access; update checkSessionPermission to respect the
hierarchy all > write > read by comparing permission levels instead of blindly
returning true when any `${serverName}:...` key exists: for each permKey in
this.sessionPermissions.get(conversationId) that startsWith `${serverName}:`,
parse the stored permission token (e.g., split on ':' to get
'read'|'write'|'all'), map these to numeric levels (read=1, write=2, all=3) and
only return true if storedLevel >= requestedLevel (map _permissionType
similarly), keep the existing console.log when granting access and return false
otherwise.
In `@test/main/presenter/sessionPresenter/permissionHandler.test.ts`:
- Around line 329-374: The tests call acquirePermissionResumeLock,
getPermissionResumeLock, and releasePermissionResumeLock on
presenter.sessionManager but the test presenter mock lacks those methods,
causing TypeError; update the test setup to add mocked implementations for
presenter.sessionManager (or replace the mock with a real SessionManager
instance) that implement acquirePermissionResumeLock(agentId, messageId),
getPermissionResumeLock(agentId) and releasePermissionResumeLock(agentId) (e.g.,
use jest.fn() or a small in-memory lock map implementation) so the tests in the
"Permission resume lock" describe block can run against those methods.
- Around line 376-424: The test "should update permissions with same or lower
permission level" is incomplete and references out-of-scope variables; fix it by
giving the test its own local mocks and assertions: declare a local messageId
constant and a mocked messageManager (e.g., mock messageManager.getMessage to
return the prepared message content), then call the permission-handling function
under test (the permission handler in sessionPresenter/permissionHandler used by
this suite) to simulate granting 'write' permission, and finally add expect(...)
assertions that both the 'read' and 'write' permission action blocks are updated
to the expected status (e.g., not pending / granted) and that any
persistence/update helpers were called with the correct payloads; alternatively,
if this behavior is tested elsewhere, remove this no-op test to avoid misleading
test counts.
- Around line 279-327: The mock for presenter.sessionManager is missing the
methods used by the new tests (addPendingPermission, getPendingPermissions,
hasPendingPermissions, acquirePermissionResumeLock, getPermissionResumeLock,
releasePermissionResumeLock), causing TypeErrors; fix by updating the test setup
to either replace presenter.sessionManager with a real SessionManager instance
(constructed with mocked dependencies) or extend the mock to implement those
methods with the expected behaviors (append/return pending permission entries
and manage resume-lock state) so that calls to addPendingPermission,
getPendingPermissions, hasPendingPermissions, acquirePermissionResumeLock,
getPermissionResumeLock, and releasePermissionResumeLock succeed in the tests.
🟡 Minor comments (12)
src/renderer/src/stores/chat.ts-2189-2198 (1)
2189-2198:⚠️ Potential issue | 🟡 MinorComment on line 2190 must be in English.
The comment
// 监听权限更新事件，刷新消息以显示最新的权限状态should be written in English per coding guidelines.Proposed fix
- // 监听权限更新事件，刷新消息以显示最新的权限状态 + // Listen for permission update events to refresh messages and display the latest permission stateAs per coding guidelines:
**/*.{ts,tsx}— "All logs and comments must be in English" andsrc/renderer/src/**/*.{ts,tsx,vue}— "Ensure all code comments are in English."docs/specs/process-tool/tasks.md-127-135 (1)
127-135:⚠️ Potential issue | 🟡 MinorFenced code block missing language specifier (MD040).
The dependency diagram code block at line 127 lacks a language identifier. Use
```textor```mermaid(if converted) to satisfy markdownlint.Proposed fix
-``` +```text Task 1.1 ──┬──→ Task 2.1 ──┬──→ Task 2.2src/renderer/src/events.ts-52-53 (1)
52-53:⚠️ Potential issue | 🟡 MinorComment on line 53 must be in English.
The comment
// 权限状态更新，通知前端刷新UIshould be in English per coding guidelines.Proposed fix
- ERROR: 'stream:error', // 替代 stream-error - PERMISSION_UPDATED: 'stream:permission-updated' // 权限状态更新，通知前端刷新UI + ERROR: 'stream:error', // Replaces stream-error + PERMISSION_UPDATED: 'stream:permission-updated' // Permission state updated, notify frontend to refresh UINote: The existing comments on other lines (e.g.,
// 替代 stream-response) also use Chinese, but since those aren't changed lines, flagging only the new addition here. As per coding guidelines:**/*.{js,ts,tsx,jsx,vue,mjs,cjs}— "All logs and comments must be in English."src/main/presenter/agentPresenter/acp/backgroundExecSessionManager.ts-14-18 (1)
14-18:⚠️ Potential issue | 🟡 Minor
OPENCLAW_BASH_PENDING_MAX_OUTPUT_CHARSlooks like a leftover from another project.This environment variable name doesn't match the
PI_BASH_*naming convention used by the other variables. If it's intentional for backward compatibility, a brief comment would help; otherwise, consider removing it.docs/specs/edit-file-tool/tasks.md-102-102 (1)
102-102:⚠️ Potential issue | 🟡 MinorAdd a language identifier to the fenced code block.
Static analysis (markdownlint MD040) flags this code block as missing a language specifier. Use
```textor similar for the commit message block.src/main/presenter/mcpPresenter/toolManager.ts-636-648 (1)
636-648:⚠️ Potential issue | 🟡 Minor
grantPermissionwithremember: falseand noconversationIdis a silent no-op.When
rememberisfalseandconversationIdisundefined, the function only logs and does nothing — the permission is never stored anywhere. Callers likely expect at least a best-effort grant.src/main/presenter/agentPresenter/acp/backgroundExecSessionManager.ts-489-529 (1)
489-529:⚠️ Potential issue | 🟡 Minor
killInternalunconditionally overrides status to'killed'at line 528.If the process happens to exit normally (exit code 0) just as
killInternalfires, theexitevent handler (line 455) setsstatus = 'done', then line 528 overrides it to'killed'. Consider only settingsession.status = 'killed'if it's still'running'.Proposed fix
- session.status = 'killed' + if (session.status === 'running') { + session.status = 'killed' + }src/main/presenter/agentPresenter/acp/agentBashHandler.ts-336-346 (1)
336-346:⚠️ Potential issue | 🟡 MinorInconsistent timestamp capture.
Date.now()is called separately on lines 344 and 345, which may produce slightly different values. The foreground path capturesstartedAtonce and reuses it. Use a single variable for consistency.Proposed fix
+ const startedAt = Date.now() // Emit initial terminal snippet await this.emitTerminalSnippet(conversationId, { id: result.sessionId, status: 'running', command, cwd, output: '', truncated: false, exitCode: null, - startedAt: Date.now(), - timestamp: Date.now() + startedAt, + timestamp: startedAt })src/main/presenter/agentPresenter/acp/agentBashHandler.ts-26-27 (1)
26-27:⚠️ Potential issue | 🟡 Minor
yieldMsis declared in the schema but never consumed.
yieldMsis validated byExecuteCommandArgsSchemabut never destructured or passed to any execution path. Even though the schema description inagentToolManager.tsindicates it's intended for foreground mode, the parameter is not actually used in the foreground execution logic. Remove it from the schema until it's properly wired up to avoid silently ignoring parameters passed by callers.src/main/presenter/agentPresenter/acp/agentToolManager.ts-1338-1345 (1)
1338-1345:⚠️ Potential issue | 🟡 MinorMisleading comment and dead code path for read operations.
The comment on line 1340 says "Check for path in read operations too", but the condition
targets.length === 0 && isWriteOperationonly triggers for write operations. ThereadToolslist (lines 1261–1267) andisReadOperationvariable (line 1313) are computed but never drive any permission check logic — read operations always returnnull.If read pre-checks are intentionally skipped, remove the
readToolslist andisReadOperationvariable, and fix the comment. If read pre-checks should exist, the condition needs to betargets.length === 0 && (isWriteOperation || isReadOperation).src/main/presenter/agentPresenter/session/sessionManager.ts-273-283 (1)
273-283:⚠️ Potential issue | 🟡 MinorLock for a different
messageIdis silently overwritten.If
permissionResumeLockalready exists for a differentmessageId, this method overwrites it without warning and returnstrue. This could mask a scenario where a previous resume flow hasn't completed yet. Consider either returningfalse(or logging a warning) when overwriting a lock held by another message.🛡️ Suggested guard against overwriting a different message's lock
acquirePermissionResumeLock(agentId: string, messageId: string): boolean { const session = this.sessions.get(agentId) if (!session) return false const runtime = this.ensureRuntime(session) if (runtime.permissionResumeLock?.messageId === messageId) { return false } + if (runtime.permissionResumeLock) { + console.warn( + `[SessionManager] Overwriting existing lock for message ${runtime.permissionResumeLock.messageId} with ${messageId}` + ) + } runtime.permissionResumeLock = { messageId, startedAt: Date.now() } session.updatedAt = Date.now() return true }src/main/presenter/agentPresenter/acp/agentToolManager.ts-1007-1013 (1)
1007-1013:⚠️ Potential issue | 🟡 MinorSwitch case variable declaration leaks to other cases.
The
const commandResultdeclared inside theexecute_commandcase is not block-scoped, which Biome/ESLint correctly flags asnoSwitchDeclarations.🔧 Wrap in a block
- case 'execute_command': + case 'execute_command': { if (!this.bashHandler) { throw new Error('Bash handler not initialized for execute_command tool') } const commandResult = await this.bashHandler.executeCommand(parsedArgs, { conversationId }) return { content: typeof commandResult === 'string' ? commandResult : JSON.stringify(commandResult) } + }
🧹 Nitpick comments (17)
src/renderer/src/stores/chat.ts (1)
2191-2198: ThemessageIdfrom the payload is only logged, not used for a targeted refresh.Currently
bumpMessageCacheVersion()refreshes the entire message list. If the intent is to eventually support targeted message refreshes, consider usinghandleMessageEdited(payload.messageId)instead, which would fetch and enrich only the affected message. If the current broad refresh is intentional (e.g., multiple permission blocks may change), this is fine as-is.src/main/presenter/agentPresenter/loop/toolCallHandler.ts (1)
515-517: Emptyifblock — dead code that should be removed.Lines 516-517 contain an empty conditional body that appears to be a leftover from debugging or an incomplete implementation.
Proposed fix
- const lastBlock = state.message.content[state.message.content.length - 1] - if (lastBlock && lastBlock.type === 'tool_call' && lastBlock.tool_call) { - } - this.finalizeLastBlock(state)src/main/presenter/agentPresenter/session/sessionContext.ts (1)
44-46: Consider deprecatingpendingPermissionin favor ofpendingPermissions.Both
pendingPermission(singular) andpendingPermissions(array) coexist, where the singular is always synced topendingPermissions[0]inSessionManager. This dual-field pattern adds maintenance burden and risks drift. If the singular field exists purely for backward compatibility, consider marking it with a@deprecatedJSDoc annotation so future contributors know to migrate away from it.docs/specs/process-tool/plan.md (1)
7-30: Add language identifiers to fenced code blocks.Multiple fenced code blocks use bare triple-backticks without a language specifier, triggering MD040. For ASCII diagrams and plaintext flows, use
```textor```plaintext.Also applies to: 89-96, 100-106, 110-114, 118-123, 127-136, 140-155
docs/specs/permission-flow-stabilization/plan.md (1)
21-27: Add a language identifier to the fenced code block.The state machine diagram should specify a language (e.g.,
```text) to satisfy MD040.src/main/presenter/toolPresenter/index.ts (2)
210-230: Extract duplicated argument-parsing logic into a shared helper.The JSON-parse-with-jsonrepair-fallback block (lines 210–230) is a near-exact copy of lines 139–156 in
callTool. Consider extracting a private method likeparseToolArguments(argsString: string): Record<string, unknown>to eliminate the duplication.♻️ Proposed helper extraction
+ private parseToolArguments(argsString: string): Record<string, unknown> { + if (!argsString.trim().length) return {} + try { + return JSON.parse(argsString) as Record<string, unknown> + } catch (error) { + console.warn('[ToolPresenter] Failed to parse tool arguments, trying jsonrepair:', error) + try { + return JSON.parse(jsonrepair(argsString)) as Record<string, unknown> + } catch (error) { + console.warn('[ToolPresenter] Failed to repair tool arguments, using empty args.', error) + return {} + } + } + }Then in both
callToolandpreCheckToolPermission:- let args: Record<string, unknown> = {} - const argsString = request.function.arguments || '' - if (argsString.trim().length > 0) { - try { - args = JSON.parse(argsString) as Record<string, unknown> - } catch (error) { - ... - } - } + const args = this.parseToolArguments(request.function.arguments || '')
204-208: Add a warning log whenagentToolManageris null for an agent-sourced tool.Line 200 logs a warning when the tool source is missing, but lines 206–208 silently return
nullwhenagentToolManagerhasn't been initialized. This makes debugging harder since an agent tool would silently skip permission checks.Proposed fix
if (source === 'agent') { // Agent tools: delegate to AgentToolManager for pre-check if (!this.agentToolManager) { + console.warn(`[ToolPresenter] Agent tool manager not initialized for permission check of '${toolName}'`) return null }src/main/presenter/mcpPresenter/toolManager.ts (1)
740-743:sessionPermissionsis not cleaned up indestroy().Event listeners are removed in
destroy(), butsessionPermissionsis never cleared, which could lead to stale data if aToolManagerinstance is reused or references linger.Proposed fix
public destroy(): void { eventBus.off(MCP_EVENTS.CLIENT_LIST_UPDATED, this.handleServerListUpdate) eventBus.off(MCP_EVENTS.CONFIG_CHANGED, this.handleConfigChange) + this.sessionPermissions.clear() }src/main/presenter/agentPresenter/acp/agentBashHandler.ts (1)
298-349: Duplicated permission-check-and-throw block between foreground and background paths.The permission check +
CommandPermissionRequiredErrorthrow pattern inexecuteCommandBackground(lines 310–328) is nearly identical to lines 65–85. A shared helper would reduce duplication.♻️ Possible helper
+ private assertCommandPermission(command: string, conversationId?: string): void { + if (!this.commandPermissionHandler) return + const permissionCheck = this.commandPermissionHandler.checkPermission(conversationId, command) + if (!permissionCheck.allowed) { + const commandInfo = this.commandPermissionHandler.buildCommandInfo(command) + throw new CommandPermissionRequiredError( + 'components.messageBlockPermissionRequest.description.commandWithRisk', + { + toolName: 'execute_command', + serverName: 'agent-filesystem', + permissionType: 'command', + description: 'Execute command requires approval.', + command, + commandSignature: commandInfo.signature, + commandInfo, + conversationId, + } + ) + } + }Then replace both blocks with
this.assertCommandPermission(command, options.conversationId).Also applies to: 65-85
src/main/presenter/agentPresenter/loop/toolCallProcessor.ts (2)
62-66:ToolCallinterface duplicates existing inline type.
ToolCallExecutionContext.toolCallsis already typed asArray<{ id: string; name: string; arguments: string }>, which is exactly the same shape. Consider reusing that type or at least usingToolCallconsistently throughout the file (including inToolCallExecutionContext).
133-142:isBatchPermissionandtotalInBatchare untyped additions topermission_request.These fields are emitted in the batch path but aren't part of the existing permission_request type used in the per-tool path (line 350). The
LLMAgentEventdatais likely loosely typed, but this ad-hoc shape divergence could confuse consumers. Consider documenting the contract or aligning the types.src/main/presenter/agentPresenter/session/sessionManager.ts (1)
165-172: Misleading variable namehasExistingLock.
acquirePermissionResumeLockreturnstrueon successful acquisition andfalsewhen the lock already exists. The variable namehasExistingLockreads as the inverse of what it represents, making theif (!hasExistingLock)guard confusing.🔧 Suggested rename for clarity
- const hasExistingLock = this.acquirePermissionResumeLock(agentId, messageId) - if (!hasExistingLock) { + const lockAcquired = this.acquirePermissionResumeLock(agentId, messageId) + if (!lockAcquired) {src/main/presenter/agentPresenter/acp/agentToolManager.ts (2)
881-884: Unreachable process-tool guard incallFileSystemTool.
callTool(line 418) already dispatchesprocessbefore reachingcallFileSystemTool. This early-return block is dead code in the normal flow. If it's intentional as a safety net, a brief comment would help; otherwise, consider removing it.
895-903: Alias normalization foredit_fileis pragmatic but loosely typed.Mapping
file_path → path,old_string → oldText,new_string → newTextsilently accepts non-standard parameter names. This is presumably for LLM compatibility. Consider logging when aliases are actually used so misuse is visible during debugging.src/main/presenter/agentPresenter/permission/permissionHandler.ts (3)
36-70:canBatchUpdatenow only matches the exact same tool call, making the "batch" concept a single-item update.Lines 50–52 enforce
targetPermission.tool_call?.id === grantedPermission.tool_call?.id, so only the block whosetool_call.idmatches the granted one will pass. The secondary guard at lines 59–67 is therefore redundant — it re-checks the same tool_call ID equality that line 52 already guarantees.If single-block-at-a-time is intentional (matching the "CRITICAL FIX" comment), consider simplifying the function and renaming it (e.g.,
shouldUpdateBlock) to avoid confusion about "batching."
632-637:denied: trueproperty added toPendingToolCallis never read.The
deniedflag is set here via spread + cast, butexecuteSingleToolCallre-checkspermissionBlock?.status === 'denied'from the database instead of reading this flag. The castas PendingToolCallalso hides the type mismatch sincePendingToolCallhas nodeniedfield.♻️ Remove unused property
} else if (permissionBlock.status === 'denied') { - // Denied - will generate error response later const pendingCall = this.buildPendingToolCallFromBlock(block) - if (pendingCall) { - toolCalls.push({ ...pendingCall, denied: true } as PendingToolCall) - } + if (pendingCall) toolCalls.push(pendingCall)
496-502: Lock is released at multiple "SINGLE EXIT POINT" locations — comments are misleading.The lock is released at lines 522, 569, 577, and 592. While each path is correct, the repeated "SINGLE EXIT POINT" comments are inaccurate and could mislead future maintainers. Consider using a
try/finallypattern to guarantee release at one true exit point.Also applies to: 546-548, 516-523, 562-572, 574-577, 578-593
- Extract unified PermissionRequestPayload interface for type consistency - Fix batch permission requests to include extended fields (paths, providerId, etc.) - Change resume logic to process all resolved tool calls, skipping completed blocks - Normalize line endings before text matching in editFile - Optimize frontend generation state to maintain working status during user interactions - Add test cases for batch permission and resume behavior
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/agentPresenter/permission/permissionHandler.ts (1)
864-920:⚠️ Potential issue | 🔴 Critical
restartAgentLoopAfterPermissionmust acquire the resume lock before callingresumeToolExecutionAfterPermissions, or it will early-exit at the lock validation check.
resumeToolExecutionAfterPermissionschecks for an active lock at line 466–478. Without an acquired lock, the method returns early (line 478) without executing any tool calls. Compare this tohandlePermissionResponse(line 270–282), which correctly acquires the lock before calling this method. The commented-outconversationIdat line 877 suggests this was left incomplete.🐛 Proposed fix: Acquire lock before calling resume
// UNIFIED FLOW: Use resumeToolExecutionAfterPermissions for all cases // This method handles: // 1. Setting up the generating state // 2. Finding and executing all granted tool calls // 3. Continuing with stream completion + const conversationId = message.conversationId + const lockAcquired = presenter.sessionManager.acquirePermissionResumeLock( + conversationId, + messageId + ) + if (!lockAcquired) { + console.log('[PermissionHandler] Resume already in progress for this message, skipping') + return + } await this.resumeToolExecutionAfterPermissions(messageId, true)
🤖 Fix all issues with AI agents
In `@src/renderer/src/stores/chat.ts`:
- Around line 2241-2250: The PERMISSION_UPDATED handler only calls
bumpMessageCacheVersion(), which won’t refresh message content; update the ipc
listener (STREAM_EVENTS.PERMISSION_UPDATED) to call
handleMessageEdited(payload.messageId) after logging so the latest message is
fetched from the main process, then bumpMessageCacheVersion() if needed; also
change the inline comment to English per coding guidelines. Ensure you reference
the existing ipc handler that uses window.electron.ipcRenderer.on and the
functions handleMessageEdited and bumpMessageCacheVersion.
🧹 Nitpick comments (8)
src/renderer/src/stores/chat.ts (1)
1409-1435: Pending user-action detection: consider adding a role guard before accessing.content.The code casts
enrichedMessagetoAssistantMessage(line 1409) without first confirmingenrichedMessage.role === 'assistant'. While in practice the generating messages cache should only contain assistant messages, a defensive check would prevent a subtle bug if a non-assistant message ever ends up in the cache (e.g., due to a future refactor).🛡️ Suggested defensive check
const enrichedMessage = await enrichMessageWithExtra(updatedMessage) - const assistantContent = (enrichedMessage as AssistantMessage).content - const hasPendingUserAction = - Array.isArray(assistantContent) && - assistantContent.some( + const assistantContent = + enrichedMessage.role === 'assistant' + ? (enrichedMessage as AssistantMessage).content + : undefined + const hasPendingUserAction = + Array.isArray(assistantContent) && + assistantContent.some(src/main/presenter/toolPresenter/index.ts (2)
16-40:PreCheckedPermissionResultduplicatesPermissionRequestPayloadfromtoolCallProcessor.ts.Both types share nearly identical fields (
toolName,serverName,permissionType,description,paths,command,commandInfo,providerId,requestId,sessionId,agentId,agentName,conversationId,rememberable, plus an index signature). Consider extracting a shared type to avoid drift between the two definitions.
191-246: Duplicated JSON parse/repair logic betweencallToolandpreCheckToolPermission.The argument parsing block (lines 206–225) is nearly identical to lines 150–167 in
callTool. Extract a shared private helper to reduce duplication.♻️ Proposed helper extraction
+ private parseToolArguments(argsString: string): Record<string, unknown> { + if (argsString.trim().length === 0) return {} + try { + return JSON.parse(argsString) as Record<string, unknown> + } catch (error) { + console.warn('[ToolPresenter] Failed to parse tool arguments, trying jsonrepair:', error) + try { + return JSON.parse(jsonrepair(argsString)) as Record<string, unknown> + } catch (error) { + console.warn('[ToolPresenter] Failed to repair tool arguments, using empty args.', error) + return {} + } + } + }Then use
const args = this.parseToolArguments(request.function.arguments || '')in both methods.src/main/presenter/agentPresenter/permission/permissionHandler.ts (4)
36-70: Redundant tool_call ID check incanBatchUpdate.Line 52 already returns
falsewhentargetPermission.tool_call?.id !== grantedPermission.tool_call?.id. The secondary check at line 66 for command / agent-filesystem / deepchat-settings is therefore alwaystrueand can never cause afalsereturn. The entireifblock (lines 59–67) is dead code.♻️ Remove redundant guard
// CRITICAL FIX: Only batch the exact same tool call (same tool_call.id) // This ensures user approval of one tool doesn't auto-approve other tools from the same server if (targetPermission.tool_call?.id !== grantedPermission.tool_call?.id) return false // Check permission type hierarchy const targetPermissionType = (targetPermission.extra?.permissionType as string) || 'read' if (!isPermissionSufficient(grantedPermissionType, targetPermissionType)) return false - // For special permission types, still require exact tool call matching (already checked above) - const targetType = targetPermission.extra?.permissionType as string - if ( - targetType === 'command' || - targetServerName === 'agent-filesystem' || - targetServerName === 'deepchat-settings' - ) { - // Additional safety: these types should never be batched across different tool calls - return targetPermission.tool_call?.id === grantedPermission.tool_call?.id - } - return true
875-878: Remove commented-out code.Line 877 has a commented-out variable declaration (
// const conversationId = message.conversationId). This is a leftover from refactoring.🧹 Clean up
- // const conversationId = message.conversationId
148-165: Verbose debug logging left in production code.Lines 148–164 dump all pending permission blocks to console when
updatedCount === 0. This is useful during development but noisy in production. Consider gating behind a debug flag or removing it.
697-709: Repeatedas anycasts onhandleLLMAgentResponsecalls.Every call to
handleLLMAgentResponsewithinexecuteSingleToolCallusesas anyto bypass type checking (lines 707, 760, 793, 827, 843). This hides potential type mismatches and defeats the benefit of strict TypeScript. Consider defining a proper interface/union type for the event payloads.Also applies to: 750-761, 783-794, 813-828, 832-844
src/main/presenter/agentPresenter/loop/toolCallProcessor.ts (1)
47-81: LocalPermissionTypeandPermissionRequestPayloadre-declare shared types.
PermissionTypeon line 53 duplicatessrc/shared/chat.d.ts:153, andPermissionRequestPayload(lines 63–81) overlaps withsrc/shared/types/core/llm-events.ts:173-191andPreCheckedPermissionResultintoolPresenter/index.ts. Consider importing the shared types to keep a single source of truth.
|
|
||
| // 监听权限更新事件，刷新消息以显示最新的权限状态 | ||
| window.electron.ipcRenderer.on( | ||
| STREAM_EVENTS.PERMISSION_UPDATED, | ||
| (_, payload: { messageId: string }) => { | ||
| console.log('[Chat Store] Permission updated, refreshing message:', payload.messageId) | ||
| // 触发消息缓存刷新，使UI显示下一个待处理的权限 | ||
| bumpMessageCacheVersion() | ||
| } | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for PERMISSION_UPDATED to understand when and how it's emitted
rg -n 'PERMISSION_UPDATED' --type=ts --type=js -C 5Repository: ThinkInAIXYZ/deepchat
Length of output: 4367
🏁 Script executed:
# Search for handleMessageEdited implementation to understand what it does
rg -n 'handleMessageEdited' --type=ts --type=js -C 3Repository: ThinkInAIXYZ/deepchat
Length of output: 1448
🏁 Script executed:
# Search for bumpMessageCacheVersion to understand its purpose
rg -n 'bumpMessageCacheVersion' --type=ts --type=js -C 3Repository: ThinkInAIXYZ/deepchat
Length of output: 2296
🏁 Script executed:
# Check the broader context around the PERMISSION_UPDATED handler in chat.ts
sed -n '2230,2260p' src/renderer/src/stores/chat.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 873
🏁 Script executed:
# Look for other IPC event listeners to understand the pattern used in this store
rg -n 'ipcRenderer\.on' src/renderer/src/stores/chat.ts -C 3Repository: ThinkInAIXYZ/deepchat
Length of output: 3120
Bump cache version alone won't display permission changes—fetch the message to ensure content is updated.
The PERMISSION_UPDATED event signals that permissions have changed, but bumpMessageCacheVersion() only re-evaluates dependent computed properties (messageItems, etc.) with the existing cached message objects. If the message content hasn't already been refreshed by a stream:response or MESSAGE_EDITED event, the UI won't reflect the permission update.
Call handleMessageEdited(payload.messageId) to fetch the latest message from the main process and guarantee the UI displays the updated permission state:
♻️ Fetch the message to ensure content is fresh
window.electron.ipcRenderer.on(
STREAM_EVENTS.PERMISSION_UPDATED,
(_, payload: { messageId: string }) => {
console.log('[Chat Store] Permission updated, refreshing message:', payload.messageId)
- // 触发消息缓存刷新，使UI显示下一个待处理的权限
- bumpMessageCacheVersion()
+ // Fetch the latest message to ensure UI reflects updated permission state
+ handleMessageEdited(payload.messageId)
}
)Also ensure comments use English per coding guidelines.
🤖 Prompt for AI Agents
In `@src/renderer/src/stores/chat.ts` around lines 2241 - 2250, The
PERMISSION_UPDATED handler only calls bumpMessageCacheVersion(), which won’t
refresh message content; update the ipc listener
(STREAM_EVENTS.PERMISSION_UPDATED) to call
handleMessageEdited(payload.messageId) after logging so the latest message is
fetched from the main process, then bumpMessageCacheVersion() if needed; also
change the inline comment to English per coding guidelines. Ensure you reference
the existing ipc handler that uses window.electron.ipcRenderer.on and the
functions handleMessageEdited and bumpMessageCacheVersion.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/agentPresenter/acp/agentToolManager.ts (1)
1089-1119:⚠️ Potential issue | 🔴 Critical
collectWriteTargetsdoes not handleedit_file, soassertWritePermissionis bypassed at runtime.
edit_fileis in the write tools list andassertWritePermissionis called for it (line 981), butcollectWriteTargetshas no case for'edit_file'. It falls through todefaultand returns[]. SinceassertWritePermissionreturns early whentargets.length === 0(line 1067), no write-permission check is enforced foredit_fileat execution time.The pre-check in
preCheckToolPermissionworks around this via the fallback on lines 1325–1331, but the actual enforcement path is skipped.🐛 Add edit_file case to collectWriteTargets
case 'text_replace': { const pathArg = args.path return typeof pathArg === 'string' ? [pathArg] : [] } + case 'edit_file': { + const pathArg = args.path + return typeof pathArg === 'string' ? [pathArg] : [] + } case 'move_files': {
🤖 Fix all issues with AI agents
In `@src/main/presenter/agentPresenter/acp/agentToolManager.ts`:
- Around line 989-999: The 'execute_command' switch case declares const
commandResult which can leak into other cases; fix it by adding a block scope
for that case (wrap the case body in { ... }) so local variables like
commandResult and the check for this.bashHandler are confined, and keep the
existing logic that calls this.bashHandler.executeCommand(parsedArgs, {
conversationId }) and returns the content (string or JSON.stringify) intact.
In `@src/main/presenter/agentPresenter/acp/backgroundExecSessionManager.ts`:
- Around line 453-467: The current flush logic only runs when
session.outputFilePath && session.totalOutputLength >
config.offloadThresholdChars, causing unread stdout/stderr to be dropped for
non-offloaded sessions on exit; update the flush block in
BackgroundExecSessionManager (the method handling the exit/cleanup for a session
object) to attempt reading and appending any remaining stdout/stderr for all
sessions (regardless of offload) by calling session.child.stdout?.read() and
session.child.stderr?.read() and then invoking this.appendOutput(session, ... ,
config) when data exists, preserving the existing try/catch and logger.warn
behavior.
In `@src/main/presenter/agentPresenter/permission/permissionHandler.ts`:
- Around line 647-653: Remove the unused `denied` property and the forced cast
when pushing pending tool calls: instead of spreading and casting the result of
buildPendingToolCallFromBlock into a new object with a `denied` flag, push the
PendingToolCall returned by buildPendingToolCallFromBlock directly (i.e.,
toolCalls.push(pendingCall)). This eliminates the incorrect `as PendingToolCall`
type assertion and the unused `denied` field; note that executeSingleToolCall
re-checks permissionBlock?.status === 'denied' so the extra flag is unnecessary.
Ensure references to PendingToolCall, buildPendingToolCallFromBlock, and
executeSingleToolCall remain unchanged otherwise.
In `@src/main/presenter/mcpPresenter/toolManager.ts`:
- Around line 342-345: The current preCheckToolPermission method returns null
when this.toolNameToTargetMap is missing, which can be misinterpreted as
"permission granted"; change preCheckToolPermission to instead throw a
descriptive Error (e.g., "Tool target map unavailable for permission check")
when this.toolNameToTargetMap is falsy so callers (and callTool) cannot proceed
silently, and update any call sites expecting null accordingly; reference the
preCheckToolPermission method and this.toolNameToTargetMap to locate and
implement the change.
- Around line 637-648: The code path where remember is false and conversationId
is absent currently only logs "Temporary permission granted" but doesn't store
or apply the permission, so callers get no effect; fix by adding and using an
in-memory ephemeral permission store (e.g., this.temporaryPermissions as a Set
or Map) inside ToolManager and, where the current snippet builds the key
(`${serverName}:${permissionType}`), add that key to the ephemeral store when
conversationId is missing; also update any permission-checking logic (the places
that currently consult this.sessionPermissions) to consult
this.temporaryPermissions as well so the non-persistent grant actually takes
effect.
- Around line 289-295: The new Chinese comments in ToolManager should be
replaced with English; locate the comment blocks near the permission check in
the method that references conversationId and checkSessionPermission(serverName,
permissionType) (inside the ToolManager class) and translate them to concise
English equivalents (e.g., "Prioritize session-level memory permissions (current
session auto-executes)") and similarly convert the other Chinese comments around
the related permission/cache logic at the other modified spots; update the
inline comments and any console/log messages to English while keeping the
original intent and leaving function/class names unchanged.
- Around line 651-688: Translate the remaining Chinese comments to English in
the ToolManager methods: change the comment above checkSessionPermission
("检查会话级别的权限" and the explanatory line about permission hierarchy) to an English
description (e.g., "Check session-level permissions" and "Permission hierarchy:
all > write > read"), and translate the comment above clearSessionPermissions
("清除会话的临时权限") to "Clear temporary session permissions"; update any accompanying
inline comments in checkSessionPermission or references to
this.sessionPermissions to English as well so all comments/logs in
checkSessionPermission and clearSessionPermissions are fully in English.
🧹 Nitpick comments (3)
docs/specs/edit-file-tool/plan.md (1)
7-31: Fenced code block missing language specifier.The ASCII diagram block on line 7 should have a language identifier (e.g.,
textorplaintext) to satisfy markdownlint MD040.-``` +```text ┌─────────────────────────────────────────────────────────────┐test/main/presenter/sessionPresenter/permissionHandler.test.ts (1)
269-316: These tests exercise mock behavior rather than PermissionHandler logic.The "Session manager helpers" and "Permission resume lock" tests directly invoke the
presenterMock.sessionManagermethods, not thePermissionHandler. While this validates the mock is wired correctly (important for the integration tests above), it doesn't test any productionSessionManagercode. Consider renaming thedescribeblock to something like"Session manager mock behavior"to clarify intent.src/main/presenter/agentPresenter/acp/agentToolManager.ts (1)
866-869: Redundant process-tool guard insidecallFileSystemTool.
callTool(line 403) already routes tocallProcessToolbefore reachingcallFileSystemTool. This guard is unreachable dead code in normal flow. While it's harmless as a safety net, consider removing it or adding a comment explaining its defensive intent.
| case 'execute_command': | ||
| if (!this.bashHandler) { | ||
| throw new Error('Bash handler not initialized for execute_command tool') | ||
| } | ||
| const commandResult = await this.bashHandler.executeCommand(parsedArgs, { | ||
| conversationId | ||
| }) | ||
| return { | ||
| content: await this.bashHandler.executeCommand(parsedArgs, { conversationId }) | ||
| content: | ||
| typeof commandResult === 'string' ? commandResult : JSON.stringify(commandResult) | ||
| } |
There was a problem hiding this comment.
Switch case declaration without block scope.
Biome correctly flags const commandResult on line 993 as a declaration that leaks into other switch cases. Wrap in braces.
♻️ Wrap in block scope
- case 'execute_command':
+ case 'execute_command': {
if (!this.bashHandler) {
throw new Error('Bash handler not initialized for execute_command tool')
}
const commandResult = await this.bashHandler.executeCommand(parsedArgs, {
conversationId
})
return {
content:
typeof commandResult === 'string' ? commandResult : JSON.stringify(commandResult)
}
+ }📝 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.
| case 'execute_command': | |
| if (!this.bashHandler) { | |
| throw new Error('Bash handler not initialized for execute_command tool') | |
| } | |
| const commandResult = await this.bashHandler.executeCommand(parsedArgs, { | |
| conversationId | |
| }) | |
| return { | |
| content: await this.bashHandler.executeCommand(parsedArgs, { conversationId }) | |
| content: | |
| typeof commandResult === 'string' ? commandResult : JSON.stringify(commandResult) | |
| } | |
| case 'execute_command': { | |
| if (!this.bashHandler) { | |
| throw new Error('Bash handler not initialized for execute_command tool') | |
| } | |
| const commandResult = await this.bashHandler.executeCommand(parsedArgs, { | |
| conversationId | |
| }) | |
| return { | |
| content: | |
| typeof commandResult === 'string' ? commandResult : JSON.stringify(commandResult) | |
| } | |
| } |
🧰 Tools
🪛 Biome (2.3.14)
[error] 993-997: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In `@src/main/presenter/agentPresenter/acp/agentToolManager.ts` around lines 989 -
999, The 'execute_command' switch case declares const commandResult which can
leak into other cases; fix it by adding a block scope for that case (wrap the
case body in { ... }) so local variables like commandResult and the check for
this.bashHandler are confined, and keep the existing logic that calls
this.bashHandler.executeCommand(parsedArgs, { conversationId }) and returns the
content (string or JSON.stringify) intact.
| // Flush any remaining output | ||
| if (session.outputFilePath && session.totalOutputLength > config.offloadThresholdChars) { | ||
| try { | ||
| const remainingStdout = session.child.stdout?.read?.() | ||
| const remainingStderr = session.child.stderr?.read?.() | ||
| if (remainingStdout) { | ||
| this.appendOutput(session, remainingStdout.toString('utf-8'), config) | ||
| } | ||
| if (remainingStderr) { | ||
| this.appendOutput(session, remainingStderr.toString('utf-8'), config) | ||
| } | ||
| } catch (error) { | ||
| logger.warn(`[BackgroundExec] Failed to flush remaining output:`, error) | ||
| } | ||
| } |
There was a problem hiding this comment.
Remaining output flush is skipped for non-offloaded sessions.
The guard on line 454 only flushes remaining stdout/stderr when the session has already been offloaded to a file. However, when using exit (not close), streams may still have unread data regardless of offload status. For non-offloaded sessions, any data arriving between the last data event and the exit event will be silently dropped.
🐛 Proposed fix: flush remaining output for all sessions
// Flush any remaining output
- if (session.outputFilePath && session.totalOutputLength > config.offloadThresholdChars) {
- try {
- const remainingStdout = session.child.stdout?.read?.()
- const remainingStderr = session.child.stderr?.read?.()
- if (remainingStdout) {
- this.appendOutput(session, remainingStdout.toString('utf-8'), config)
- }
- if (remainingStderr) {
- this.appendOutput(session, remainingStderr.toString('utf-8'), config)
- }
- } catch (error) {
- logger.warn(`[BackgroundExec] Failed to flush remaining output:`, error)
+ try {
+ const remainingStdout = session.child.stdout?.read?.()
+ const remainingStderr = session.child.stderr?.read?.()
+ if (remainingStdout) {
+ this.appendOutput(session, remainingStdout.toString('utf-8'), config)
}
+ if (remainingStderr) {
+ this.appendOutput(session, remainingStderr.toString('utf-8'), config)
+ }
+ } catch (error) {
+ logger.warn(`[BackgroundExec] Failed to flush remaining output:`, error)
}🤖 Prompt for AI Agents
In `@src/main/presenter/agentPresenter/acp/backgroundExecSessionManager.ts` around
lines 453 - 467, The current flush logic only runs when session.outputFilePath
&& session.totalOutputLength > config.offloadThresholdChars, causing unread
stdout/stderr to be dropped for non-offloaded sessions on exit; update the flush
block in BackgroundExecSessionManager (the method handling the exit/cleanup for
a session object) to attempt reading and appending any remaining stdout/stderr
for all sessions (regardless of offload) by calling session.child.stdout?.read()
and session.child.stderr?.read() and then invoking this.appendOutput(session,
... , config) when data exists, preserving the existing try/catch and
logger.warn behavior.
| } else if (permissionBlock.status === 'denied') { | ||
| // Denied - will generate error response later | ||
| const pendingCall = this.buildPendingToolCallFromBlock(block) | ||
| if (pendingCall) { | ||
| toolCalls.push({ ...pendingCall, denied: true } as PendingToolCall) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for PendingToolCall type definition
echo "=== Searching for PendingToolCall type definition ==="
rg -n "interface PendingToolCall|type PendingToolCall" --type=ts -A 15
echo ""
echo "=== Searching for denied field usage ==="
rg -n "denied\s*:" --type=ts src/main/presenter/agentPresenter/permission/
echo ""
echo "=== Checking executeSingleToolCall function ==="
rg -n "executeSingleToolCall" --type=ts -B 2 -A 30 | head -100Repository: ThinkInAIXYZ/deepchat
Length of output: 11179
🏁 Script executed:
#!/bin/bash
# Check the file to see the specific lines mentioned in the review
echo "=== Content of permissionHandler.ts around lines 647-653 ==="
sed -n '640,660p' src/main/presenter/agentPresenter/permission/permissionHandler.ts
echo ""
echo "=== Content around line 714 (executeSingleToolCall) ==="
sed -n '700,750p' src/main/presenter/agentPresenter/permission/permissionHandler.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 2804
🏁 Script executed:
#!/bin/bash
# Search for all usages of the 'denied' property/field in the codebase
echo "=== Searching for references to .denied property ==="
rg -n "\.denied" --type=ts src/
echo ""
echo "=== Searching for where PendingToolCall is used ==="
rg -n "PendingToolCall" --type=ts | head -50Repository: ThinkInAIXYZ/deepchat
Length of output: 2289
Remove the unused denied flag and type cast from line 651.
The denied property is not defined in the PendingToolCall type (defined in messageBuilder.ts:27-34) and is never used anywhere in the codebase. The spread operator and as PendingToolCall cast at line 651 hide a type error. The executeSingleToolCall method (line 692+) re-fetches the permission block from the database and checks permissionBlock?.status === 'denied' directly, never referencing the denied flag from the parameter.
Simply push the pendingCall as-is without adding the unused property:
toolCalls.push(pendingCall)
🤖 Prompt for AI Agents
In `@src/main/presenter/agentPresenter/permission/permissionHandler.ts` around
lines 647 - 653, Remove the unused `denied` property and the forced cast when
pushing pending tool calls: instead of spreading and casting the result of
buildPendingToolCallFromBlock into a new object with a `denied` flag, push the
PendingToolCall returned by buildPendingToolCallFromBlock directly (i.e.,
toolCalls.push(pendingCall)). This eliminates the incorrect `as PendingToolCall`
type assertion and the unused `denied` field; note that executeSingleToolCall
re-checks permissionBlock?.status === 'denied' so the extra flag is unnecessary.
Ensure references to PendingToolCall, buildPendingToolCallFromBlock, and
executeSingleToolCall remain unchanged otherwise.
| // 1. 优先检查 session 级别的内存权限（当前会话自动执行） | ||
| if (conversationId && this.checkSessionPermission(conversationId, serverName, permissionType)) { | ||
| console.log( | ||
| `[ToolManager] Permission granted via session cache: server '${serverName}' has '${permissionType}' permission` | ||
| ) | ||
| return true | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Chinese comments on changed lines violate coding guidelines.
Lines 289, 297, 303 contain Chinese comments in newly modified code. The same applies to lines 651–652 and 685. Per coding guidelines, all comments must be in English.
♻️ Translate to English
- // 1. 优先检查 session 级别的内存权限（当前会话自动执行）
+ // 1. Check session-level in-memory permission first (auto-execute for current session)- // 2. 检查持久化的 'all' 权限
+ // 2. Check persisted 'all' permission- // 3. 检查持久化的特定权限类型
+ // 3. Check persisted specific permission typeAs per coding guidelines: **/*.{js,ts,tsx,jsx,vue,mjs,cjs}: All logs and comments must be in English.
📝 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.
| // 1. 优先检查 session 级别的内存权限（当前会话自动执行） | |
| if (conversationId && this.checkSessionPermission(conversationId, serverName, permissionType)) { | |
| console.log( | |
| `[ToolManager] Permission granted via session cache: server '${serverName}' has '${permissionType}' permission` | |
| ) | |
| return true | |
| } | |
| // 1. Check session-level in-memory permission first (auto-execute for current session) | |
| if (conversationId && this.checkSessionPermission(conversationId, serverName, permissionType)) { | |
| console.log( | |
| `[ToolManager] Permission granted via session cache: server '${serverName}' has '${permissionType}' permission` | |
| ) | |
| return true | |
| } |
🤖 Prompt for AI Agents
In `@src/main/presenter/mcpPresenter/toolManager.ts` around lines 289 - 295, The
new Chinese comments in ToolManager should be replaced with English; locate the
comment blocks near the permission check in the method that references
conversationId and checkSessionPermission(serverName, permissionType) (inside
the ToolManager class) and translate them to concise English equivalents (e.g.,
"Prioritize session-level memory permissions (current session auto-executes)")
and similarly convert the other Chinese comments around the related
permission/cache logic at the other modified spots; update the inline comments
and any console/log messages to English while keeping the original intent and
leaving function/class names unchanged.
| if (!this.toolNameToTargetMap) { | ||
| console.error('[ToolManager] Tool target map is not available for permission check.') | ||
| return null | ||
| } |
There was a problem hiding this comment.
Returning null when tool map is unavailable masks a lookup failure.
When this.toolNameToTargetMap is null, the method returns null (meaning "no permission needed"), which could silently skip a permission check. While callTool would fail separately, preCheckToolPermission callers may interpret null as "safe to proceed."
Consider returning null with a more specific log, or throwing, so callers don't assume permission is granted.
🤖 Prompt for AI Agents
In `@src/main/presenter/mcpPresenter/toolManager.ts` around lines 342 - 345, The
current preCheckToolPermission method returns null when this.toolNameToTargetMap
is missing, which can be misinterpreted as "permission granted"; change
preCheckToolPermission to instead throw a descriptive Error (e.g., "Tool target
map unavailable for permission check") when this.toolNameToTargetMap is falsy so
callers (and callTool) cannot proceed silently, and update any call sites
expecting null accordingly; reference the preCheckToolPermission method and
this.toolNameToTargetMap to locate and implement the change.
| if (conversationId) { | ||
| const key = `${serverName}:${permissionType}` | ||
| const existing = this.sessionPermissions.get(conversationId) ?? new Set<string>() | ||
| existing.add(key) | ||
| this.sessionPermissions.set(conversationId, existing) | ||
| console.log( | ||
| `[ToolManager] Session permission stored: ${key} for conversation ${conversationId}` | ||
| ) | ||
| } else { | ||
| console.log(`[ToolManager] Temporary permission granted (no conversationId)`) | ||
| } | ||
| } |
There was a problem hiding this comment.
Non-persistent grant with no conversationId is a silent no-op.
When remember is false and conversationId is absent (line 645–647), the permission is neither persisted nor session-cached. The log says "Temporary permission granted" but nothing actually happens. This could confuse callers who expect the grant to take effect.
🤖 Prompt for AI Agents
In `@src/main/presenter/mcpPresenter/toolManager.ts` around lines 637 - 648, The
code path where remember is false and conversationId is absent currently only
logs "Temporary permission granted" but doesn't store or apply the permission,
so callers get no effect; fix by adding and using an in-memory ephemeral
permission store (e.g., this.temporaryPermissions as a Set or Map) inside
ToolManager and, where the current snippet builds the key
(`${serverName}:${permissionType}`), add that key to the ephemeral store when
conversationId is missing; also update any permission-checking logic (the places
that currently consult this.sessionPermissions) to consult
this.temporaryPermissions as well so the non-persistent grant actually takes
effect.
| // 检查会话级别的权限 | ||
| // 当前会话权限遵循层级：all > write > read | ||
| checkSessionPermission( | ||
| conversationId: string, | ||
| serverName: string, | ||
| permissionType: 'read' | 'write' | 'all' | ||
| ): boolean { | ||
| const sessionPerms = this.sessionPermissions.get(conversationId) | ||
| if (!sessionPerms) return false | ||
|
|
||
| const permissionLevelMap: Record<'read' | 'write' | 'all', number> = { | ||
| read: 1, | ||
| write: 2, | ||
| all: 3 | ||
| } | ||
| const requiredLevel = permissionLevelMap[permissionType] | ||
| const prefix = `${serverName}:` | ||
|
|
||
| for (const permKey of sessionPerms) { | ||
| if (!permKey.startsWith(prefix)) continue | ||
|
|
||
| const storedPermission = permKey.slice(prefix.length) as 'read' | 'write' | 'all' | ||
| const storedLevel = permissionLevelMap[storedPermission] | ||
| if (storedLevel >= requiredLevel) { | ||
| console.log( | ||
| `[ToolManager] Session auto-execute: server '${serverName}' has granted permission '${permKey}' in conversation '${conversationId}', required='${permissionType}'` | ||
| ) | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| // 清除会话的临时权限 | ||
| clearSessionPermissions(conversationId: string): void { | ||
| this.sessionPermissions.delete(conversationId) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Session permission hierarchy is now correctly implemented; translate remaining Chinese comments.
The checkSessionPermission method now properly enforces all > write > read using numeric levels, addressing the previous review's security concern about blanket server access.
However, lines 651–652 and 685 still have Chinese comments:
♻️ Translate to English
- // 检查会话级别的权限
- // 当前会话权限遵循层级：all > write > read
+ // Check session-level permission
+ // Session permission hierarchy: all > write > read
checkSessionPermission(- // 清除会话的临时权限
+ // Clear session's temporary permissions
clearSessionPermissions(conversationId: string): void {As per coding guidelines: **/*.{js,ts,tsx,jsx,vue,mjs,cjs}: All logs and comments must be in English.
🤖 Prompt for AI Agents
In `@src/main/presenter/mcpPresenter/toolManager.ts` around lines 651 - 688,
Translate the remaining Chinese comments to English in the ToolManager methods:
change the comment above checkSessionPermission ("检查会话级别的权限" and the explanatory
line about permission hierarchy) to an English description (e.g., "Check
session-level permissions" and "Permission hierarchy: all > write > read"), and
translate the comment above clearSessionPermissions ("清除会话的临时权限") to "Clear
temporary session permissions"; update any accompanying inline comments in
checkSessionPermission or references to this.sessionPermissions to English as
well so all comments/logs in checkSessionPermission and clearSessionPermissions
are fully in English.
1 participant
Summary by CodeRabbit
New Features
Permission & UX
Documentation
Tests