refactor(agent): split runtime lifecycle owners#1971
Merged
Conversation
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change decomposes ChangesRuntime decomposition
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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: 6
🧹 Nitpick comments (6)
src/main/presenter/agentRuntimePresenter/deepChatLoopRunner.ts (1)
741-743: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent use of raw
console.*vs the sharedloggeracross the newly-extracted execution files. Both files already import@shared/loggerand use it for info-level logs, but warn/error paths fall back toconsole.warn/console.error, bypassing whatever transport/formatting/log-level filtering the shared logger provides.
src/main/presenter/agentRuntimePresenter/deepChatLoopRunner.ts#L741-L743: replaceconsole.warn(...)inonInterleavedReasoningGapwithlogger.warn(...).src/main/presenter/agentRuntimePresenter/turnCoordinator.ts#L670-L670: replaceconsole.error('[DeepChatAgent] processMessage error:', err)withlogger.error(...).src/main/presenter/agentRuntimePresenter/turnCoordinator.ts#L701-L701: replaceconsole.warn('[DeepChatAgent] failed to release claimed queue input:', releaseError)withlogger.warn(...).src/main/presenter/agentRuntimePresenter/turnCoordinator.ts#L1101-L1101: replaceconsole.error('[DeepChatAgent] resumeAssistantMessage error:', error)withlogger.error(...).🤖 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/deepChatLoopRunner.ts` around lines 741 - 743, Replace the raw console logging with the shared logger in all four affected sites: use logger.warn in deepChatLoopRunner.ts lines 741-743 within onInterleavedReasoningGap, and logger.error, logger.warn, and logger.error respectively in turnCoordinator.ts lines 670, 701, and 1101, preserving each existing message and error argument.src/main/presenter/agentRuntimePresenter/generationSettings.ts (1)
187-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated reasoning-capability resolution between
buildDefaultGenerationSettingsandsanitizeGenerationSettings.Both functions independently recompute
fixedTemperatureKimi,portrait,capabilityProviderId,anthropicReasoningToggle, andanthropicReasoningEnabled(Lines 193-206 vs 344-357) with identical logic. Extracting a sharedresolveReasoningCapabilityContext(configPresenter, providerId, modelId, modelConfig)helper would remove the duplication and the risk of the two copies drifting apart.Also applies to: 336-576
🤖 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/generationSettings.ts` around lines 187 - 334, Extract the duplicated reasoning-capability resolution from buildDefaultGenerationSettings and sanitizeGenerationSettings into a shared resolveReasoningCapabilityContext helper accepting configPresenter, providerId, modelId, and modelConfig. Update both functions to consume the helper’s fixedTemperatureKimi, portrait, capabilityProviderId, anthropicReasoningToggle, and anthropicReasoningEnabled values, preserving their existing behavior.src/main/presenter/agentRuntimePresenter/toolAdapters.ts (1)
97-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame
throwIfAbortRequestedhelper reimplemented identically in three files.
toolAdapters.ts,toolPermissionReviewer.ts, anddeferredToolExecutor.tseach define a byte-for-byte identical localthrowIfAbortRequested.interactionCoordinator.tsalready avoids this by receivingthrowIfAbortRequested/isAbortErroras injected ports rather than local functions — extracting a shared helper (e.g. alongsideisAbortErrorintoolAdapters.ts, or a new smallabortUtils.ts) and importing it in all three would remove the drift risk of these copies diverging over time.
src/main/presenter/agentRuntimePresenter/toolAdapters.ts#L97-L105: keep (or re-export) the canonical implementation here, alongsideisAbortError.src/main/presenter/agentRuntimePresenter/toolPermissionReviewer.ts#L17-L25: replace local definition with an import from the shared helper.src/main/presenter/agentRuntimePresenter/deferredToolExecutor.ts#L56-L64: replace local definition with an import from the shared helper.🤖 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/toolAdapters.ts` around lines 97 - 105, Deduplicate the identical throwIfAbortRequested helper by keeping or exporting the canonical implementation alongside isAbortError in src/main/presenter/agentRuntimePresenter/toolAdapters.ts#L97-L105, then import and use it in src/main/presenter/agentRuntimePresenter/toolPermissionReviewer.ts#L17-L25 and src/main/presenter/agentRuntimePresenter/deferredToolExecutor.ts#L56-L64, removing both local definitions.src/main/agent/deepchat/resources/systemPromptBuilder.ts (1)
332-364: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
buildVerificationPolicyPromptreads and parsespackage.jsontwice for the same directory.
getVerificationScriptNames(normalizedWorkdir)already callsreadPackageJsonManifestinternally (line 71), then line 345 callsreadPackageJsonManifest(normalizedWorkdir)again — duplicate disk I/O + JSON.parse on every system-prompt rebuild (cache miss).♻️ Proposed fix: read the manifest once
-function getVerificationScriptNames(workdir: string): string[] { - const manifest = readPackageJsonManifest(workdir); - const scripts = manifest?.scripts; +function getVerificationScriptNames(manifest: PackageJsonManifest | null): string[] { + const scripts = manifest?.scripts; if (!scripts || typeof scripts !== "object") { return []; } ... - const verificationScripts = getVerificationScriptNames(normalizedWorkdir); - const manifest = readPackageJsonManifest(normalizedWorkdir); + const manifest = readPackageJsonManifest(normalizedWorkdir); + const verificationScripts = getVerificationScriptNames(manifest);🤖 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/agent/deepchat/resources/systemPromptBuilder.ts` around lines 332 - 364, Update buildVerificationPolicyPrompt and getVerificationScriptNames so the package.json manifest is read and parsed once per directory, reusing the existing manifest for both verification script detection and workspace identification; preserve the current script-selection and DeepChat detection behavior.src/main/presenter/agentRuntimePresenter/toolResolver.ts (1)
65-100: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAgent extension policy is resolved independently up to 3× per turn.
resolveAgentExtensionPolicy(→configPresenter.resolveDeepChatAgentConfig) has no shared result threaded through turn preparation, so tool-profile resolution, active-skill resolution, and system-prompt building each fetch it separately for the same session/turn.
src/main/presenter/agentRuntimePresenter/toolResolver.ts#L65-L100: reuse thepolicyalready computed at L71 instead of letting the L74 call re-resolve it.src/main/presenter/agentRuntimePresenter/toolResolver.ts#L175-L199: add an optional pre-resolvedpolicyparameter toresolveActiveSkillNamesForToolProfileand skip the internalresolveAgentExtensionPolicycall when it's supplied (backward compatible for existing callers likedeferredToolExecutor.ts).src/main/agent/deepchat/resources/systemPromptBuilder.ts#L117-L120: if feasible, accept the policy already resolved earlier in the turn (e.g. viaSystemPromptBuilderDependencies) instead of callingresolveAgentExtensionPolicyagain.♻️ Proposed fix (toolResolver.ts)
async resolveActiveSkillNamesForToolProfile( sessionId: string, - resourceInstance?: DeepChatAgentInstance + resourceInstance?: DeepChatAgentInstance, + resolvedPolicy?: AgentExtensionPolicy ): Promise<string[]> { if ( !this.dependencies.configPresenter.getSkillsEnabled() || !this.dependencies.skillPresenter?.getActiveSkills ) { return [] } try { - const policy = await this.resolveAgentExtensionPolicy(sessionId, resourceInstance) + const policy = + resolvedPolicy ?? (await this.resolveAgentExtensionPolicy(sessionId, resourceInstance)) return filterSkillNamesByPolicy( normalizeStringList(await this.dependencies.skillPresenter.getActiveSkills(sessionId)), policy )const policy = await this.resolveAgentExtensionPolicy(sessionId, resourceInstance) const effectiveActiveSkillNames = activeSkillNamesOverride === undefined - ? await this.resolveActiveSkillNamesForToolProfile(sessionId, resourceInstance) + ? await this.resolveActiveSkillNamesForToolProfile(sessionId, resourceInstance, policy) : filterSkillNamesByPolicy(activeSkillNamesOverride, policy)🤖 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/toolResolver.ts` around lines 65 - 100, Thread the policy resolved in resolveContext through the turn-preparation flow. In src/main/presenter/agentRuntimePresenter/toolResolver.ts#L65-L100, pass the existing policy to resolveActiveSkillNamesForToolProfile instead of resolving it again; update src/main/presenter/agentRuntimePresenter/toolResolver.ts#L175-L199 to accept an optional policy while preserving resolution for callers without one, including deferredToolExecutor.ts. In src/main/agent/deepchat/resources/systemPromptBuilder.ts#L117-L120, reuse the earlier policy through SystemPromptBuilderDependencies when feasible rather than calling resolveAgentExtensionPolicy again.src/main/presenter/agentRuntimePresenter/providerPermissionCoordinator.ts (1)
189-203: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winStale-permission detection still hinges on an error message literal.
resolveSafelytreats a request as stale only when the thrown message starts withUnknown ACP permission request:. That couples correctness to exact wording in the ACP permission path; a future message change would misclassify the result. Use a typed error/code or a shared helper instead of parsing the message.🤖 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/providerPermissionCoordinator.ts` around lines 189 - 203, The resolveSafely method should identify stale permission requests through a typed error, error code, or shared helper rather than checking the “Unknown ACP permission request:” message prefix. Update the ACP permission rejection path to produce that stable identifier, and have resolveSafely use it while preserving normal propagation for unrelated errors.
🤖 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
`@docs/architecture/agent-system-layered-runtime/modules/loop-engine-and-lifecycle.md`:
- Around line 7-12: Remove the blank line within the ownership blockquote so the
lines beginning with “Current ownership” through “DeepChatAgentInstance/LoopRun”
remain one contiguous quoted paragraph, satisfying MD028.
In `@docs/architecture/agent-system-layered-runtime/README.md`:
- Around line 47-51: Update the presenter boundary metrics in the architecture
paragraph to the current values: 2,604 lines and a 3,200-line architecture
guard, keeping the surrounding ownership and referenced specification details
unchanged.
In `@src/main/presenter/agentRuntimePresenter/compactionRuntimeCoordinator.ts`:
- Around line 71-83: Update the error handling around
CompactionService.applyCompaction in the compaction coordinator so every
failure, not only aborts, deletes the compaction placeholder, refreshes the
message, and restores the previous session compaction state before rethrowing.
Preserve the existing abort-specific throw behavior and current-session
validation.
In `@src/main/presenter/agentRuntimePresenter/generationSettings.ts`:
- Around line 38-53: Extend PersistedSessionGenerationRow with the persisted
image_generation_options_json and video_generation_options_json fields, then
update mapPersistedGenerationPatch to parse and assign them to
patch.imageGeneration and patch.videoGeneration. Preserve null/absent values
appropriately so cold rehydration restores stored user-specific settings instead
of model defaults.
In `@src/main/presenter/agentRuntimePresenter/toolAdapters.ts`:
- Around line 169-190: Update the generateCompletionStandalone call in the
screenshot analysis flow to explicitly disable error swallowing by passing
swallowErrors: false in its options. Preserve the existing abort signal handling
so provider and network failures propagate to the surrounding catch block and
use its dedicated failure message.
In `@src/main/presenter/agentRuntimePresenter/turnCoordinator.ts`:
- Around line 234-236: Update prepareTurnResources to always merge
sessionActiveSkillNames with instance.getRuntimeActivatedSkills(), including the
resume path; remove the conditional dependence on
input.runtimeActivatedSkillNames. Ensure both rebuilt tools and the base system
prompt receive the merged active skill names, while preserving the existing
skill-presenter resolution behavior.
---
Nitpick comments:
In `@src/main/agent/deepchat/resources/systemPromptBuilder.ts`:
- Around line 332-364: Update buildVerificationPolicyPrompt and
getVerificationScriptNames so the package.json manifest is read and parsed once
per directory, reusing the existing manifest for both verification script
detection and workspace identification; preserve the current script-selection
and DeepChat detection behavior.
In `@src/main/presenter/agentRuntimePresenter/deepChatLoopRunner.ts`:
- Around line 741-743: Replace the raw console logging with the shared logger in
all four affected sites: use logger.warn in deepChatLoopRunner.ts lines 741-743
within onInterleavedReasoningGap, and logger.error, logger.warn, and
logger.error respectively in turnCoordinator.ts lines 670, 701, and 1101,
preserving each existing message and error argument.
In `@src/main/presenter/agentRuntimePresenter/generationSettings.ts`:
- Around line 187-334: Extract the duplicated reasoning-capability resolution
from buildDefaultGenerationSettings and sanitizeGenerationSettings into a shared
resolveReasoningCapabilityContext helper accepting configPresenter, providerId,
modelId, and modelConfig. Update both functions to consume the helper’s
fixedTemperatureKimi, portrait, capabilityProviderId, anthropicReasoningToggle,
and anthropicReasoningEnabled values, preserving their existing behavior.
In `@src/main/presenter/agentRuntimePresenter/providerPermissionCoordinator.ts`:
- Around line 189-203: The resolveSafely method should identify stale permission
requests through a typed error, error code, or shared helper rather than
checking the “Unknown ACP permission request:” message prefix. Update the ACP
permission rejection path to produce that stable identifier, and have
resolveSafely use it while preserving normal propagation for unrelated errors.
In `@src/main/presenter/agentRuntimePresenter/toolAdapters.ts`:
- Around line 97-105: Deduplicate the identical throwIfAbortRequested helper by
keeping or exporting the canonical implementation alongside isAbortError in
src/main/presenter/agentRuntimePresenter/toolAdapters.ts#L97-L105, then import
and use it in
src/main/presenter/agentRuntimePresenter/toolPermissionReviewer.ts#L17-L25 and
src/main/presenter/agentRuntimePresenter/deferredToolExecutor.ts#L56-L64,
removing both local definitions.
In `@src/main/presenter/agentRuntimePresenter/toolResolver.ts`:
- Around line 65-100: Thread the policy resolved in resolveContext through the
turn-preparation flow. In
src/main/presenter/agentRuntimePresenter/toolResolver.ts#L65-L100, pass the
existing policy to resolveActiveSkillNamesForToolProfile instead of resolving it
again; update src/main/presenter/agentRuntimePresenter/toolResolver.ts#L175-L199
to accept an optional policy while preserving resolution for callers without
one, including deferredToolExecutor.ts. In
src/main/agent/deepchat/resources/systemPromptBuilder.ts#L117-L120, reuse the
earlier policy through SystemPromptBuilderDependencies when feasible rather than
calling resolveAgentExtensionPolicy again.
🪄 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: 82f9f73f-37f7-4328-97e1-6e54edb002a9
📒 Files selected for processing (35)
docs/architecture/agent-runtime-presenter-thinning/plan.mddocs/architecture/agent-runtime-presenter-thinning/spec.mddocs/architecture/agent-runtime-presenter-thinning/tasks.mddocs/architecture/agent-system-layered-runtime/README.mddocs/architecture/agent-system-layered-runtime/modules/loop-engine-and-lifecycle.mddocs/architecture/agent-system-layered-runtime/modules/permission-and-interactions.mddocs/architecture/agent-turn-preparation-deduplication/plan.mddocs/architecture/agent-turn-preparation-deduplication/spec.mddocs/architecture/agent-turn-preparation-deduplication/tasks.mddocs/architecture/deepchat-runtime-lifecycle-owners/plan.mddocs/architecture/deepchat-runtime-lifecycle-owners/spec.mddocs/architecture/deepchat-runtime-lifecycle-owners/tasks.mddocs/guides/code-navigation.mdscripts/agent-cleanup-guard.mjssrc/main/agent/deepchat/resources/systemPromptBuilder.tssrc/main/presenter/agentRuntimePresenter/acpCompatibilityDependencies.tssrc/main/presenter/agentRuntimePresenter/compactionRuntimeCoordinator.tssrc/main/presenter/agentRuntimePresenter/deepChatLoopRunner.tssrc/main/presenter/agentRuntimePresenter/deferredToolExecutor.tssrc/main/presenter/agentRuntimePresenter/generationSettings.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/interactionCoordinator.tssrc/main/presenter/agentRuntimePresenter/interactionProjection.tssrc/main/presenter/agentRuntimePresenter/providerPermissionCoordinator.tssrc/main/presenter/agentRuntimePresenter/runtimeMetadata.tssrc/main/presenter/agentRuntimePresenter/sessionSettingsCoordinator.tssrc/main/presenter/agentRuntimePresenter/toolAdapters.tssrc/main/presenter/agentRuntimePresenter/toolPermissionReviewer.tssrc/main/presenter/agentRuntimePresenter/toolResolver.tssrc/main/presenter/agentRuntimePresenter/turnCoordinator.tstest/main/agent/deepchat/resources/systemPromptBuilder.test.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentRuntimePresenter/generationSettings.test.tstest/main/presenter/agentRuntimePresenter/toolAdapters.test.tstest/main/presenter/agentRuntimePresenter/toolPermissionReviewer.test.ts
Comment on lines
+47
to
+51
| 后续的 [Agent Runtime Presenter Thinning](../agent-runtime-presenter-thinning/spec.md) 又把 generation、 | ||
| prompt/resource、permission review、tool adaptation、interaction projection、session settings、ACP | ||
| compatibility 与 compaction/provider-permission coordination 移到 focused owner；presenter boundary | ||
| 经 [turn preparation deduplication](../agent-turn-preparation-deduplication/spec.md) 后现为 4,874 行 / | ||
| 136 methods，并由 5,000 行 architecture guard 约束。 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the presenter size and guard values.
This paragraph contradicts docs/architecture/deepchat-runtime-lifecycle-owners/spec.md, which records the final presenter at 2,604 lines with a 3,200-line guard. Leaving 4,874/5,000 here makes the architecture documentation describe an obsolete checkpoint rather than the current boundary.
🤖 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 `@docs/architecture/agent-system-layered-runtime/README.md` around lines 47 -
51, Update the presenter boundary metrics in the architecture paragraph to the
current values: 2,604 lines and a 3,200-line architecture guard, keeping the
surrounding ownership and referenced specification details unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
src/main/presenter/agentRuntimePresenter/providerPermissionCoordinator.ts (1)
189-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale-permission detection relies on fragile string matching.
resolveSafelydistinguishes "stale" from other failures by checkingmessage?.startsWith('Unknown ACP permission request:')against the port's thrown error text. Any future wording change inAcpAsLlmProviderPermissionPort.resolveAgentPermissionwould silently break this classification (falling through tothrow errorinstead of being treated as stale), and the coupling isn't visible from this file alone.Consider a typed/discriminated error (e.g., a named error class or an error code) exported from the permission port so this check doesn't depend on exact message text.
🤖 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/providerPermissionCoordinator.ts` around lines 189 - 203, Replace the message-prefix check in resolveSafely with a typed or discriminated stale-permission error contract exported by AcpAsLlmProviderPermissionPort and thrown by resolveAgentPermission. Detect that error type or code directly, return the existing { status: 'stale', error } result for it, and rethrow all other failures.src/main/presenter/agentRuntimePresenter/toolPermissionReviewer.ts (1)
122-160: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAlign the system prompt with the enforced high-risk policy
riskLevel: 'high'is always downgraded toask_userhere, so the prompt line about allowing high risk is misleading. Update that text to say high-risk actions always require user confirmation, or remove it.🤖 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/toolPermissionReviewer.ts` around lines 122 - 160, Update the system prompt used by the tool permission reviewer to state that high-risk actions always require user confirmation, matching the enforced riskLevel === 'high' behavior in the decision logic. Do not change the existing decision handling.src/main/presenter/agentRuntimePresenter/turnCoordinator.ts (1)
282-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the exported
TurnStartContexttype instead of duplicating its shape.
TurnStartContext(Lines 58-64) is exported but unused;start()'scontextparameter redeclares the identical structure inline, so the two can silently drift apart on future edits.♻️ Proposed fix
async start( sessionId: string, content: string | SendMessageInput, - context?: { - projectDir?: string | null - emitRefreshBeforeStream?: boolean - pendingQueueItemId?: string - pendingQueueItemSource?: ProcessPendingInputSource - maxProviderRounds?: number - } + context?: TurnStartContext ): Promise<MessageStartResult> {🤖 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/turnCoordinator.ts` around lines 282 - 292, Update the start method’s context parameter in TurnCoordinator to use the exported TurnStartContext type instead of redeclaring the inline object shape, while preserving the existing optional context behavior and fields.src/main/presenter/agentRuntimePresenter/deepChatLoopRunner.ts (1)
338-351: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
as unknown ascast here.ILlmProviderPresenter.getProviderInstance()already returnsBaseLLMProvider, which exposescoreStream, so the extra assertion just weakens type safety and can hide signature drift.🤖 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/deepChatLoopRunner.ts` around lines 338 - 351, Update the provider lookup in the deep chat loop to call this.ports.llmProviderPresenter.getProviderInstance(state.providerId) directly, removing the unknown and structural type assertions. Use the existing ILlmProviderPresenter/BaseLLMProvider typing so coreStream retains its declared signature.src/main/agent/deepchat/resources/systemPromptBuilder.ts (2)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFile-wide quote/semicolon style deviates from the repo's Oxfmt rule.
This entire new file uses double quotes and semicolons, while sibling files in the same PR (
toolResolver.ts,sessionSettingsCoordinator.ts) use single quotes and no semicolons. As per coding guidelines,**/*.{ts,tsx,vue}should "Follow Oxfmt formatting rules: single quotes, no semicolons, and 100-character line width."🤖 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/agent/deepchat/resources/systemPromptBuilder.ts` around lines 1 - 8, Apply the repository’s Oxfmt style throughout the system prompt builder file: replace double-quoted strings with single-quoted strings, remove semicolons, and preserve the 100-character line width across imports and implementation code.Source: Coding guidelines
52-68: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSynchronous fs calls block the main process on prompt-cache misses.
readPackageJsonManifestusesfs.existsSync/fs.readFileSyncsynchronously, called (viagetVerificationScriptNames→buildVerificationPolicyPrompt) withoutawaitinsidebuildSystemPromptWithSkillson the Electron main process. It's cache-gated so not on every turn, but on each cache miss it still blocks the event loop for the duration of the disk read. The static-analysis path-traversal flag on the read call looks like a false positive here since the path is alwaysworkdir/package.jsonwithin the user's own configured project directory, not an attacker-controlled traversal string.♻️ Suggested direction: switch to async fs
-function readPackageJsonManifest(workdir: string): PackageJsonManifest | null { - try { - const packageJsonPath = path.join(workdir, "package.json"); - if (!fs.existsSync(packageJsonPath)) { - return null; - } - - const parsed = JSON.parse(fs.readFileSync(packageJsonPath, "utf-8")) as unknown; +async function readPackageJsonManifest(workdir: string): Promise<PackageJsonManifest | null> { + try { + const packageJsonPath = path.join(workdir, "package.json"); + const parsed = JSON.parse(await fs.promises.readFile(packageJsonPath, "utf-8")) as unknown;Note this requires threading
async/awaitthroughgetVerificationScriptNamesandbuildVerificationPolicyPromptas well.🤖 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/agent/deepchat/resources/systemPromptBuilder.ts` around lines 52 - 68, Replace the synchronous filesystem operations in readPackageJsonManifest with asynchronous equivalents, returning the same null-on-missing, invalid, or unreadable manifest behavior. Thread async/await through getVerificationScriptNames and buildVerificationPolicyPrompt, then update buildSystemPromptWithSkills to await the resulting prompt construction so cache misses do not block the Electron main process.src/main/presenter/agentRuntimePresenter/toolResolver.ts (1)
65-82: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid resolving the agent extension policy twice per tool-profile lookup.
In
resolveContext,policyis computed viaresolveAgentExtensionPolicy(Line 71), thenresolveActiveSkillNamesForToolProfileis called when no override is given (Line 74) — which internally re-resolves the same policy via anotherresolveAgentExtensionPolicycall (Line 187). That's twoconfigPresenter.resolveDeepChatAgentConfiground-trips for one resolution.♻️ Proposed fix: thread the already-resolved policy through
async resolveActiveSkillNamesForToolProfile( sessionId: string, - resourceInstance?: DeepChatAgentInstance + resourceInstance?: DeepChatAgentInstance, + extensionPolicy?: AgentExtensionPolicy ): Promise<string[]> { if ( !this.dependencies.configPresenter.getSkillsEnabled() || !this.dependencies.skillPresenter?.getActiveSkills ) { return [] } try { - const policy = await this.resolveAgentExtensionPolicy(sessionId, resourceInstance) + const policy = + extensionPolicy ?? (await this.resolveAgentExtensionPolicy(sessionId, resourceInstance)) return filterSkillNamesByPolicy( normalizeStringList(await this.dependencies.skillPresenter.getActiveSkills(sessionId)), policy )const effectiveActiveSkillNames = activeSkillNamesOverride === undefined - ? await this.resolveActiveSkillNamesForToolProfile(sessionId, resourceInstance) + ? await this.resolveActiveSkillNamesForToolProfile(sessionId, resourceInstance, policy) : filterSkillNamesByPolicy(activeSkillNamesOverride, policy)Also applies to: 175-199
🤖 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/toolResolver.ts` around lines 65 - 82, Update resolveContext and resolveActiveSkillNamesForToolProfile to reuse the policy already returned by resolveAgentExtensionPolicy instead of resolving it again. Thread the existing policy into resolveActiveSkillNamesForToolProfile when activeSkillNamesOverride is undefined, while preserving policy filtering for overrides and the existing resolveToolProfile flow.test/main/agent/deepchat/resources/systemPromptBuilder.test.ts (1)
9-12: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the unused
fs.promises.readFilestub
systemPromptBuilder.tsonly readspackage.jsonviafs.existsSyncandfs.readFileSync, so thisfs.promises.readFilemock never affects the test. Dropping it would make the setup clearer.🤖 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/deepchat/resources/systemPromptBuilder.test.ts` around lines 9 - 12, Remove the unused fs.promises.readFile mock rejection from the test setup, keeping the fs.existsSync stub and any fs.readFileSync mocking required by systemPromptBuilder.ts unchanged.src/main/presenter/agentRuntimePresenter/acpCompatibilityDependencies.ts (1)
60-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
throwIfAbortRequestedhelper across two files. Both files define an identical abort-check helper; the shared root cause is a missing common utility, and both already depend on@/lib/awaitWithAbort.
src/main/presenter/agentRuntimePresenter/acpCompatibilityDependencies.ts#L60-L68: remove the local definition and import the shared helper from@/lib/awaitWithAbort.src/main/presenter/agentRuntimePresenter/deferredToolExecutor.ts#L56-L64: remove the local definition and import the same shared helper.🤖 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/acpCompatibilityDependencies.ts` around lines 60 - 68, Remove the local throwIfAbortRequested helper and import the shared helper from `@/lib/awaitWithAbort` in both src/main/presenter/agentRuntimePresenter/acpCompatibilityDependencies.ts (lines 60-68) and src/main/presenter/agentRuntimePresenter/deferredToolExecutor.ts (lines 56-64), preserving existing call sites and behavior.src/main/presenter/agentRuntimePresenter/interactionProjection.ts (1)
257-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnreachable dead code.
next.inlineItemswas already unconditionally deleted at line 220, soArray.isArray(next.inlineItems)here is alwaysfalse.🧹 Proposed cleanup
- if (Array.isArray(next.inlineItems)) { - delete next.inlineItems - } - return JSON.stringify(next)🤖 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/interactionProjection.ts` around lines 257 - 259, Remove the unreachable Array.isArray(next.inlineItems) conditional and its delete operation from the interaction projection logic, leaving the earlier unconditional inlineItems deletion unchanged.
🤖 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 `@docs/architecture/agent-runtime-presenter-thinning/spec.md`:
- Around line 30-31: Reconcile the acceptance criteria and Outcome in the
architecture spec: remove the claim that the 3,200-line guard is already
enforced and update the reported results so they do not contradict the
≤3,200-line/≤130-method criteria. Keep adoption of that ceiling attributed to
the later deepchat-runtime-lifecycle-owners work, unless this spec explicitly
documents the criteria as unmet.
In
`@docs/architecture/agent-system-layered-runtime/modules/loop-engine-and-lifecycle.md`:
- Around line 7-12: Fix the MD028 warning in the ownership blockquote by
removing the trailing blank line or prefixing it with `>` so the blockquote
remains continuous. Preserve the existing ownership text and formatting.
In `@src/main/presenter/agentRuntimePresenter/interactionCoordinator.ts`:
- Line 521: Update the fire-and-forget call in the interaction coordinator to
attach a rejection handler to drainPendingQueueIfPossible, logging or otherwise
handling the error through the existing presenter error-handling path so
rejected promises cannot become unhandled rejections. Preserve the current
sessionId and 'completed' arguments.
- Around line 148-158: Update the abort-signal selection logic in the
interaction coordination try block: change the branch after
interactionOwnedByActiveRun to handle every non-owned interaction, including
when interactionOwnerRun exists for another messageId. Reuse
interactionOwnerRun.abortController.signal in that else path so
throwIfAbortRequested and awaitWithAbort always receive a signal.
---
Nitpick comments:
In `@src/main/agent/deepchat/resources/systemPromptBuilder.ts`:
- Around line 1-8: Apply the repository’s Oxfmt style throughout the system
prompt builder file: replace double-quoted strings with single-quoted strings,
remove semicolons, and preserve the 100-character line width across imports and
implementation code.
- Around line 52-68: Replace the synchronous filesystem operations in
readPackageJsonManifest with asynchronous equivalents, returning the same
null-on-missing, invalid, or unreadable manifest behavior. Thread async/await
through getVerificationScriptNames and buildVerificationPolicyPrompt, then
update buildSystemPromptWithSkills to await the resulting prompt construction so
cache misses do not block the Electron main process.
In `@src/main/presenter/agentRuntimePresenter/acpCompatibilityDependencies.ts`:
- Around line 60-68: Remove the local throwIfAbortRequested helper and import
the shared helper from `@/lib/awaitWithAbort` in both
src/main/presenter/agentRuntimePresenter/acpCompatibilityDependencies.ts (lines
60-68) and src/main/presenter/agentRuntimePresenter/deferredToolExecutor.ts
(lines 56-64), preserving existing call sites and behavior.
In `@src/main/presenter/agentRuntimePresenter/deepChatLoopRunner.ts`:
- Around line 338-351: Update the provider lookup in the deep chat loop to call
this.ports.llmProviderPresenter.getProviderInstance(state.providerId) directly,
removing the unknown and structural type assertions. Use the existing
ILlmProviderPresenter/BaseLLMProvider typing so coreStream retains its declared
signature.
In `@src/main/presenter/agentRuntimePresenter/interactionProjection.ts`:
- Around line 257-259: Remove the unreachable Array.isArray(next.inlineItems)
conditional and its delete operation from the interaction projection logic,
leaving the earlier unconditional inlineItems deletion unchanged.
In `@src/main/presenter/agentRuntimePresenter/providerPermissionCoordinator.ts`:
- Around line 189-203: Replace the message-prefix check in resolveSafely with a
typed or discriminated stale-permission error contract exported by
AcpAsLlmProviderPermissionPort and thrown by resolveAgentPermission. Detect that
error type or code directly, return the existing { status: 'stale', error }
result for it, and rethrow all other failures.
In `@src/main/presenter/agentRuntimePresenter/toolPermissionReviewer.ts`:
- Around line 122-160: Update the system prompt used by the tool permission
reviewer to state that high-risk actions always require user confirmation,
matching the enforced riskLevel === 'high' behavior in the decision logic. Do
not change the existing decision handling.
In `@src/main/presenter/agentRuntimePresenter/toolResolver.ts`:
- Around line 65-82: Update resolveContext and
resolveActiveSkillNamesForToolProfile to reuse the policy already returned by
resolveAgentExtensionPolicy instead of resolving it again. Thread the existing
policy into resolveActiveSkillNamesForToolProfile when activeSkillNamesOverride
is undefined, while preserving policy filtering for overrides and the existing
resolveToolProfile flow.
In `@src/main/presenter/agentRuntimePresenter/turnCoordinator.ts`:
- Around line 282-292: Update the start method’s context parameter in
TurnCoordinator to use the exported TurnStartContext type instead of redeclaring
the inline object shape, while preserving the existing optional context behavior
and fields.
In `@test/main/agent/deepchat/resources/systemPromptBuilder.test.ts`:
- Around line 9-12: Remove the unused fs.promises.readFile mock rejection from
the test setup, keeping the fs.existsSync stub and any fs.readFileSync mocking
required by systemPromptBuilder.ts unchanged.
🪄 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: 93120f97-145e-45db-8975-ed054234ec70
📒 Files selected for processing (70)
docs/ARCHITECTURE.mddocs/FLOWS.mddocs/README.mddocs/architecture/agent-runtime-presenter-thinning/spec.mddocs/architecture/agent-system-layered-runtime/README.mddocs/architecture/agent-system-layered-runtime/modules/loop-engine-and-lifecycle.mddocs/architecture/agent-system-layered-runtime/modules/permission-and-interactions.mddocs/architecture/deepchat-runtime-lifecycle-owners/spec.mddocs/architecture/multi-agent-isolation/plan.mddocs/architecture/multi-agent-isolation/tasks.mddocs/architecture/retire-agent-session-presenter/plan.mddocs/architecture/retire-agent-session-presenter/tasks.mddocs/architecture/session-application-coordinators/plan.mddocs/architecture/session-application-coordinators/spec.mddocs/architecture/session-application-coordinators/tasks.mddocs/architecture/session-boundary-cleanup/plan.mddocs/architecture/session-boundary-cleanup/tasks.mddocs/architecture/subagent-host-policy-isolation/plan.mddocs/architecture/subagent-host-policy-isolation/tasks.mddocs/features/cua-cross-platform-computer-use/spec.mddocs/features/cua-plugin-icon/spec.mddocs/features/daoxe-provider/plan.mddocs/features/daoxe-provider/tasks.mddocs/features/deepchat-skills-management/plan.mddocs/features/deepchat-skills-management/spec.mddocs/features/deepchat-skills-management/tasks.mddocs/features/grok-oauth/plan.mddocs/features/grok-oauth/tasks.mddocs/features/opencode-go-provider/spec.mddocs/guides/code-navigation.mddocs/guides/getting-started.mddocs/issues/acp-runtime-install-state-identity/spec.mddocs/issues/acp-tool-progress-permission-ui/spec.mddocs/issues/acp-tool-result-projection/spec.mddocs/issues/agent-mcp-allowlist-runtime-enforce/spec.mddocs/issues/agent-tool-catalog-concurrency/spec.mddocs/issues/agent-tool-workspace-cross-session-leak/spec.mddocs/issues/agent-transfer-permission-skill-reset/spec.mddocs/issues/artifact-streaming-parse-hotspot/spec.mddocs/issues/build-action-platform-failures/spec.mddocs/issues/chat-generation-lifecycle-timeouts/spec.mddocs/issues/chat-search-highlight-flicker/spec.mddocs/issues/chat-stream-scroll-feedback-loop/spec.mddocs/issues/cua-driver-0-6-7-update/spec.mddocs/issues/display-messages-streaming-hot-path/spec.mddocs/issues/markdown-codeblock-scrollbar-jitter/spec.mddocs/issues/sidebar-new-chat-workspace-intent/spec.mdscripts/agent-cleanup-guard.mjssrc/main/agent/deepchat/resources/systemPromptBuilder.tssrc/main/presenter/agentRuntimePresenter/acpCompatibilityDependencies.tssrc/main/presenter/agentRuntimePresenter/compactionRuntimeCoordinator.tssrc/main/presenter/agentRuntimePresenter/deepChatLoopRunner.tssrc/main/presenter/agentRuntimePresenter/deferredToolExecutor.tssrc/main/presenter/agentRuntimePresenter/generationSettings.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/interactionCoordinator.tssrc/main/presenter/agentRuntimePresenter/interactionProjection.tssrc/main/presenter/agentRuntimePresenter/providerPermissionCoordinator.tssrc/main/presenter/agentRuntimePresenter/runtimeMetadata.tssrc/main/presenter/agentRuntimePresenter/sessionSettingsCoordinator.tssrc/main/presenter/agentRuntimePresenter/toolAdapters.tssrc/main/presenter/agentRuntimePresenter/toolPermissionReviewer.tssrc/main/presenter/agentRuntimePresenter/toolResolver.tssrc/main/presenter/agentRuntimePresenter/turnCoordinator.tstest/main/agent/deepchat/resources/systemPromptBuilder.test.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/agentRuntimePresenter/generationSettings.test.tstest/main/presenter/agentRuntimePresenter/process.test.tstest/main/presenter/agentRuntimePresenter/toolAdapters.test.tstest/main/presenter/agentRuntimePresenter/toolPermissionReviewer.test.ts
💤 Files with no reviewable changes (33)
- docs/features/deepchat-skills-management/plan.md
- docs/features/deepchat-skills-management/tasks.md
- docs/architecture/multi-agent-isolation/tasks.md
- docs/features/cua-plugin-icon/spec.md
- docs/architecture/session-application-coordinators/plan.md
- docs/architecture/subagent-host-policy-isolation/plan.md
- docs/features/daoxe-provider/tasks.md
- docs/architecture/retire-agent-session-presenter/tasks.md
- docs/issues/markdown-codeblock-scrollbar-jitter/spec.md
- docs/features/daoxe-provider/plan.md
- docs/architecture/session-application-coordinators/tasks.md
- docs/features/grok-oauth/tasks.md
- docs/architecture/subagent-host-policy-isolation/tasks.md
- docs/architecture/multi-agent-isolation/plan.md
- docs/features/grok-oauth/plan.md
- docs/issues/chat-stream-scroll-feedback-loop/spec.md
- docs/architecture/session-boundary-cleanup/plan.md
- docs/issues/cua-driver-0-6-7-update/spec.md
- docs/issues/agent-transfer-permission-skill-reset/spec.md
- docs/architecture/retire-agent-session-presenter/plan.md
- docs/issues/acp-runtime-install-state-identity/spec.md
- docs/issues/agent-tool-workspace-cross-session-leak/spec.md
- docs/issues/chat-search-highlight-flicker/spec.md
- docs/issues/acp-tool-progress-permission-ui/spec.md
- docs/architecture/session-boundary-cleanup/tasks.md
- docs/issues/sidebar-new-chat-workspace-intent/spec.md
- docs/issues/agent-tool-catalog-concurrency/spec.md
- docs/issues/artifact-streaming-parse-hotspot/spec.md
- docs/issues/build-action-platform-failures/spec.md
- docs/issues/agent-mcp-allowlist-runtime-enforce/spec.md
- docs/issues/chat-generation-lifecycle-timeouts/spec.md
- docs/issues/display-messages-streaming-hot-path/spec.md
- docs/issues/acp-tool-result-projection/spec.md
Comment on lines
+30
to
+31
| - `agentRuntimePresenter/index.ts` is at most 3,200 lines. | ||
| - The class has at most 130 methods. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Outcome contradicts this spec's own acceptance criteria.
Acceptance criteria require ≤3,200 lines / ≤130 methods (Lines 30-31), but the Outcome reports 4,905 lines / 135 methods (Lines 59-60) while also claiming the guard already "rejects growth beyond 3,200 lines" (Line 64). A 4,905-line file cannot coexist with an already-enforced 3,200-line guard. Per the follow-up spec, the 3,200 ceiling appears to actually be adopted by the later deepchat-runtime-lifecycle-owners work, not this one — Line 64 seems to prematurely claim that outcome here.
Also applies to: 59-64
🤖 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 `@docs/architecture/agent-runtime-presenter-thinning/spec.md` around lines 30 -
31, Reconcile the acceptance criteria and Outcome in the architecture spec:
remove the claim that the 3,200-line guard is already enforced and update the
reported results so they do not contradict the ≤3,200-line/≤130-method criteria.
Keep adoption of that ceiling attributed to the later
deepchat-runtime-lifecycle-owners work, unless this spec explicitly documents
the criteria as unmet.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR completes the
AgentRuntimePresenterownership split while preserving its public façade.TurnCoordinatorowns initial and resumed turn preparation and terminal settlement.DeepChatLoopRunnerowns provider/tool execution and context-pressure recovery.InteractionCoordinatorowns paused interaction reconciliation and resume decisions.agentRuntimePresenter/index.tsfrom 8,167 lines / 211 methods to 2,604 lines / 122 methods.Architecture
AgentRuntimePresenterremains the composition root and compatibility boundary. Extracted owners receive explicit, owner-specific ports; none receives the presenter instance or a generic service locator.Per-session and per-run mutable state remains in
DeepChatAgentInstanceandLoopRun. Existing cancellation, stale-instance, Tape, Memory, permission, queue, persistence, and terminal-ordering contracts are preserved.Design records:
Test and documentation cleanup
The resulting inventory contains 576 test files and 5,541 cases.
Review follow-up
Compatibility
Validation
pnpm run typecheck,pnpm run format,pnpm run format:check,pnpm run i18n, andpnpm run lintpassed.build-check (x64)andmemory-native-validationpassed.Credentialed provider and destructive desktop-profile E2E paths were statically audited but not executed locally.
Summary by CodeRabbit