fix(agent): multi-agent host isolation and session lifecycle#1963
Merged
Conversation
Wire per-agent MCP allow-lists into catalog/call paths, stop cross-session workspace allow-list leaks, and reset permissions and skills when rebinding a session to another host agent.
Resolve permission mode, disabled tools, system prompt, and skill allow-lists from the target host agent while keeping parent workdir and model for delegated subagent sessions.
Stop ACP tool progress from looking like permissions, clarify Plugins Hub global vs agent policy scope, inherit parent approvals into subagents with clearer waiting guidance, and make chat/session timeouts and deletion best-effort instead of zombie-prone.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
🚧 Files skipped from review as they are similar to previous changes (21)
📝 WalkthroughWalkthroughThe changes define agent-isolation contracts and implement MCP policy propagation, workspace isolation, session transfer resets, subagent host-policy resolution, lifecycle cleanup, ACP progress mapping, and plugin scope messaging. ChangesAgent isolation enforcement
Subagent host policy
Session lifecycle and routing
ACP progress mapping
Plugin scope messaging
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/routes/chat/chatService.ts (1)
146-175: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
stopStream's outerscheduler.timeoutisn't guarded, contradicting the "honest stop result" contract.The wrapped task uses
Promise.allSettled(...), which never rejects on its own, so the only way thisscheduler.timeoutcall can throw is via the scheduler's own timeout/abort path — and that throw isn't caught here.stopStreamis documented (spec.md) to give an "honest stop result" via{ stopped: boolean }, but a scheduler-level timeout here would instead reject the wholestopStream()call.🛠️ Suggested fix
- let cancelFailed = false - await this.deps.scheduler.timeout({ + let cancelFailed = false + try { + await this.deps.scheduler.timeout({ task: Promise.allSettled([...]).then((results) => { ... }), ms: CHAT_STOP_TIMEOUT_MS, reason: `chat.stopStream:${targetSessionId}` - }) + }) + } catch { + cancelFailed = true + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/routes/chat/chatService.ts` around lines 146 - 175, Guard the outer scheduler.timeout call in stopStream so scheduler-level timeout or abort errors are caught instead of rejecting the method. Preserve the existing rejection handling for clearSessionPermissions and cancelGeneration, set cancelFailed when the scheduler operation fails, and always return the documented { stopped: !cancelFailed } result.
🧹 Nitpick comments (2)
test/main/agent/acp/runtime/acpContentMapper.test.ts (1)
43-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover
tool_call_updateas well as the initialtool_call.These assertions only protect the start path, while the mapper also emits reasoning for status-changing updates. Add a regression case that verifies update events likewise produce no
tool_call_permissionaction block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/agent/acp/runtime/acpContentMapper.test.ts` around lines 43 - 49, Add a regression test alongside the existing start-path assertions in the ACP content mapper tests covering a status-changing tool_call_update event. Verify the mapped update output contains no action block with action_type tool_call_permission, while preserving the existing initial tool_call coverage.src/main/presenter/agentRuntimePresenter/index.ts (1)
7530-7535: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming
normalizeSkillNamesfor generic usage.Using
normalizeSkillNamesto normalize MCP server IDs is functionally correct because the method simply deduplicates, trims, and sorts an array of strings. However, for clarity, consider renamingnormalizeSkillNamesto a more generic name likenormalizeStringListornormalizeIdentifiersif it is going to be reused across different identifier domains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentRuntimePresenter/index.ts` around lines 7530 - 7535, Rename normalizeSkillNames to a generic name such as normalizeStringList, updating its declaration and every call site including toToolDefinitionMcpServerIds. Preserve the existing deduplication, trimming, sorting, and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/routes/chat/chatService.ts`:
- Around line 86-88: Bound bestEffortCancel with the scheduler timeout so
cleanup cannot block sendMessage indefinitely when turn.cancelGeneration or
sessionPermissionPort.clearSessionPermissions never settles. Apply the same
bounded-cleanup behavior to the additional bestEffortCancel usage around lines
190–209, while preserving propagation of the original send TimeoutError.
- Line 40: Update ChatService’s acceptControllers lifecycle to retain all
concurrent AbortControllers per session, allowing stopStream() to abort every
active accept path instead of overwriting earlier ones. Bound bestEffortCancel()
with a timeout covering clearSessionPermissions() and cancelGeneration(), and
ensure stopStream() converts scheduler.timeout cleanup failures into { stopped:
false } rather than propagating the error.
In `@src/renderer/src/i18n/zh-CN/settings.json`:
- Line 2778: Update the scopeCurrentAgent translation to clarify that this
setting only affects the selected agent’s available Skills/MCP and does not
change global plugin installation or status, matching the wording used by other
locales.
---
Outside diff comments:
In `@src/main/routes/chat/chatService.ts`:
- Around line 146-175: Guard the outer scheduler.timeout call in stopStream so
scheduler-level timeout or abort errors are caught instead of rejecting the
method. Preserve the existing rejection handling for clearSessionPermissions and
cancelGeneration, set cancelFailed when the scheduler operation fails, and
always return the documented { stopped: !cancelFailed } result.
---
Nitpick comments:
In `@src/main/presenter/agentRuntimePresenter/index.ts`:
- Around line 7530-7535: Rename normalizeSkillNames to a generic name such as
normalizeStringList, updating its declaration and every call site including
toToolDefinitionMcpServerIds. Preserve the existing deduplication, trimming,
sorting, and return behavior.
In `@test/main/agent/acp/runtime/acpContentMapper.test.ts`:
- Around line 43-49: Add a regression test alongside the existing start-path
assertions in the ACP content mapper tests covering a status-changing
tool_call_update event. Verify the mapped update output contains no action block
with action_type tool_call_permission, while preserving the existing initial
tool_call coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b62d8fb3-3f1f-460c-92a9-2243ffa873e3
📒 Files selected for processing (62)
docs/architecture/agent-scoped-extensions/spec.mddocs/architecture/multi-agent-isolation/plan.mddocs/architecture/multi-agent-isolation/spec.mddocs/architecture/multi-agent-isolation/tasks.mddocs/architecture/subagent-host-policy-isolation/plan.mddocs/architecture/subagent-host-policy-isolation/spec.mddocs/architecture/subagent-host-policy-isolation/tasks.mddocs/issues/acp-tool-progress-permission-ui/spec.mddocs/issues/agent-mcp-allowlist-runtime-enforce/spec.mddocs/issues/agent-tool-workspace-cross-session-leak/spec.mddocs/issues/agent-transfer-permission-skill-reset/spec.mddocs/issues/chat-generation-lifecycle-timeouts/spec.mdsrc/main/agent/acp/runtime/acpContentMapper.tssrc/main/presenter/agentRuntimePresenter/dispatch.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/types.tssrc/main/presenter/index.tssrc/main/presenter/permission/commandPermissionCache.tssrc/main/presenter/permission/commandPermissionService.tssrc/main/presenter/permission/filePermissionService.tssrc/main/presenter/permission/settingsPermissionService.tssrc/main/presenter/runtimePorts.tssrc/main/presenter/sessionApplication/agentAssignmentPolicy.tssrc/main/presenter/sessionApplication/lifecycleCoordinator.tssrc/main/presenter/sessionApplication/lifecycleDeletionTransaction.tssrc/main/presenter/sessionApplication/ports.tssrc/main/presenter/toolPresenter/agentTools/agentToolManager.tssrc/main/presenter/toolPresenter/agentTools/subagentOrchestratorTool.tssrc/main/presenter/toolPresenter/runtimePorts.tssrc/main/routes/chat/chatService.tssrc/main/routes/index.tssrc/main/routes/sessions/sessionService.tssrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/pages/plugins/PluginsHubPage.vuetest/main/agent/acp/runtime/acpContentMapper.test.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentRuntimePresenter/dispatch.test.tstest/main/presenter/agentSessionPresenter/agentSessionPresenter.test.tstest/main/presenter/sessionApplication/agentAssignmentPolicy.test.tstest/main/presenter/sessionApplication/lifecycleCoordinator.test.tstest/main/presenter/sessionApplication/lifecycleDeletionTransaction.test.tstest/main/presenter/toolPresenter/agentTools/subagentOrchestratorTool.test.tstest/main/routes/chatService.test.ts
💤 Files with no reviewable changes (1)
- src/main/routes/index.ts
zerob13
requested changes
Jul 14, 2026
zerob13
left a comment
zerob13
left a comment
Collaborator
There was a problem hiding this comment.
Requesting changes for runtime isolation gaps and a confirmed test failure. I verified the branch at ef44dc2; typecheck, lint, format check, i18n, and the PluginsHub renderer test pass, while the main test suite fails in sessionService.test.ts.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/routes/chat/chatService.ts (1)
150-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolves prior AbortController/timeout issues; consider deduplicating cleanup logic.
Aborting the full
Set<AbortController>and bounding cleanup withscheduler.timeoutcorrectly addresses the earlier "single overwritten controller" and "unbounded cleanup" issues. However, this cleanup block (allSettled + per-result logging + timeout catch) is nearly identical tobestEffortCancel(Lines 218-249), just inlined with an extracancelFailedflag. Consider extracting a shared private helper that bothstopStreamandbestEffortCancelcall, returning per-operation failure flags, to avoid maintaining two copies of the same cleanup semantics.♻️ Sketch of a shared helper
+ private async runCleanupTasks( + sessionId: string, + timeoutReason: string + ): Promise<{ permissionsFailed: boolean; generationFailed: boolean }> { + const results = await this.deps.scheduler.timeout({ + task: Promise.allSettled([ + Promise.resolve().then(() => this.deps.sessionPermissionPort.clearSessionPermissions(sessionId)), + Promise.resolve().then(() => this.deps.turn.cancelGeneration(sessionId)) + ]), + ms: CHAT_STOP_TIMEOUT_MS, + reason: timeoutReason + }) + return { + permissionsFailed: results[0]?.status === 'rejected', + generationFailed: results[1]?.status === 'rejected' + } + }Both
stopStreamandbestEffortCancelcan then call this helper and only differ in logging wording / how they use the returned flags.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/routes/chat/chatService.ts` around lines 150 - 193, Deduplicate the session cleanup logic shared by stopStream and bestEffortCancel by extracting a private helper that performs the allSettled operations, per-operation logging, and scheduler.timeout handling. Have the helper return the cancellation failure status (and any other required operation flags), then update both callers to use it while preserving their existing logging wording and return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/routes/chat/chatService.ts`:
- Around line 150-193: Deduplicate the session cleanup logic shared by
stopStream and bestEffortCancel by extracting a private helper that performs the
allSettled operations, per-operation logging, and scheduler.timeout handling.
Have the helper return the cancellation failure status (and any other required
operation flags), then update both callers to use it while preserving their
existing logging wording and return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b84336f5-bf86-4ed0-99c9-ddc5d37af722
📒 Files selected for processing (27)
docs/architecture/multi-agent-isolation/plan.mddocs/architecture/multi-agent-isolation/spec.mddocs/architecture/multi-agent-isolation/tasks.mddocs/architecture/subagent-host-policy-isolation/plan.mddocs/architecture/subagent-host-policy-isolation/spec.mddocs/architecture/subagent-host-policy-isolation/tasks.mddocs/issues/agent-tool-workspace-cross-session-leak/spec.mddocs/issues/agent-transfer-permission-skill-reset/spec.mddocs/issues/chat-generation-lifecycle-timeouts/spec.mdsrc/main/presenter/agentRuntimePresenter/dispatch.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/index.tssrc/main/presenter/mcpPresenter/index.tssrc/main/presenter/runtimePorts.tssrc/main/presenter/sessionApplication/lifecycleCoordinator.tssrc/main/presenter/toolPresenter/agentTools/agentToolManager.tssrc/main/routes/chat/chatService.tssrc/main/routes/sessions/sessionService.tssrc/renderer/src/i18n/zh-CN/settings.jsonsrc/shared/types/presenters/core.presenter.d.tstest/main/agent/acp/runtime/acpContentMapper.test.tstest/main/presenter/agentRuntimePresenter/dispatch.test.tstest/main/presenter/mcpPresenter.test.tstest/main/presenter/sessionApplication/lifecycleCoordinator.test.tstest/main/presenter/toolPresenter/agentTools/agentToolManagerRead.test.tstest/main/routes/chatService.test.tstest/main/routes/sessionService.test.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- docs/architecture/subagent-host-policy-isolation/tasks.md
- test/main/agent/acp/runtime/acpContentMapper.test.ts
- src/main/presenter/runtimePorts.ts
- docs/architecture/subagent-host-policy-isolation/spec.md
- src/main/presenter/index.ts
- docs/architecture/multi-agent-isolation/tasks.md
- docs/issues/agent-tool-workspace-cross-session-leak/spec.md
- src/main/presenter/agentRuntimePresenter/dispatch.ts
- docs/architecture/multi-agent-isolation/plan.md
- docs/architecture/multi-agent-isolation/spec.md
- src/main/presenter/sessionApplication/lifecycleCoordinator.ts
- src/renderer/src/i18n/zh-CN/settings.json
- test/main/routes/chatService.test.ts
- src/main/presenter/agentRuntimePresenter/index.ts
…isolation # Conflicts: # test/main/presenter/sessionApplication/sessionApplication.integration.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pnpm run i18nclean.Key changes
enabledMcpServerIdsinto catalog/fingerprint/call/deferred pathstool_call_permissionblocks)Test plan
pnpm run i18n— no missing/invalid keyspnpm run format/pnpm run lint(incl. architecture guards)Summary by CodeRabbit
New Features
Bug Fixes
Documentation