fix(acp): align client runtime#1614
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds an internal AcpClientPresenter runtime boundary, implements an ACP-specific context-budget bypass in AgentRuntimePresenter, persists ACP prompt turns to a new SQLite table, hardens workspace path and terminal cwd handling, instruments ACP process/session debug events, rewires AcpProvider to use the presenter, updates renderer/i18n, and adds tests and docs. ChangesACP Client Runtime Refactor & Context Budget Bypass
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 12
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/llmProviderPresenter/acp/acpProcessManager.ts (1)
704-715:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the initialize timeout timer after
Promise.racesettlesThe timeout handle is never cleared, so each successful init leaves a live timer until expiry.
Suggested fix
- const timeoutPromise = new Promise<never>((_, reject) => { - setTimeout(() => { + let timeoutHandle: NodeJS.Timeout | null = null + const timeoutPromise = new Promise<never>((_, reject) => { + timeoutHandle = setTimeout(() => { reject( new Error( `[ACP] Connection initialization timeout after ${timeoutMs}ms for agent ${agent.id}` ) ) }, timeoutMs) }) - - const initResult = await Promise.race([initPromise, timeoutPromise]) + const initResult = await Promise.race([initPromise, timeoutPromise]) + if (timeoutHandle) { + clearTimeout(timeoutHandle) + }🤖 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/llmProviderPresenter/acp/acpProcessManager.ts` around lines 704 - 715, The timeout created in timeoutPromise uses setTimeout but never clears the timer, leaving a dangling handle; update the logic around Promise.race([initPromise, timeoutPromise]) (symbols: timeoutPromise, initPromise, setTimeout, timeoutMs, agent.id) to capture the timer id when creating the timeout and ensure clearTimeout(timerId) is called once the race settles (either in a finally block or immediately after awaiting initResult) so the timer is always cleared whether initPromise wins or the timeout rejects.
🧹 Nitpick comments (1)
src/main/presenter/acpClientPresenter/types.ts (1)
28-42: ⚡ Quick winAvoid maintaining two
DeepChatAgentEventunions in parallel.This local union now overlaps with
src/shared/contracts/acp.ts; keeping both as separate sources of truth creates easy drift risk across boundaries.♻️ Suggested refactor
+import type { DeepChatAgentEvent as SharedDeepChatAgentEvent } from '@shared/contracts/acp' import type { AssistantMessageBlock } from '@shared/chat' -export type DeepChatAgentEvent = - | { type: 'message.delta'; conversationId: string; text: string } - | { type: 'content.block'; conversationId: string; block: AssistantMessageBlock } +export type DeepChatAgentEvent = + | SharedDeepChatAgentEvent | { type: 'tool.created' | 'tool.updated' conversationId: string toolCallId?: string block: AssistantMessageBlock } - | { type: 'plan.updated'; conversationId: string; entries: unknown[] } - | { type: 'terminal.updated'; conversationId: string; terminal: unknown } - | { type: 'permission.requested'; conversationId: string; request: unknown } - | { type: 'config.updated'; conversationId: string; configState: AcpConfigState } - | { type: 'turn.completed'; conversationId: string; stopReason: string }🤖 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/acpClientPresenter/types.ts` around lines 28 - 42, You have duplicated the DeepChatAgentEvent union locally which overlaps the shared contract; remove the local type declaration in types.ts and instead import the canonical DeepChatAgentEvent from the shared contracts module (the exported union for agent events) and re-export or use that type from here, ensuring all references in this file point to the imported DeepChatAgentEvent; this keeps a single source of truth and prevents drift between the local types and the shared contract.
🤖 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/presenter/llmProviderPresenter/acp/acpProcessManager.ts`:
- Around line 216-219: The current resolveCwdInsideWorkdir resolves cwd against
the process cwd which rejects valid relative cwd values; change the logic in
resolveCwdInsideWorkdir so that resolvedCwd uses path.resolve(workdir, cwd)
(i.e. resolve cwd relative to the session workdir when cwd is relative) before
calling isPathInside; keep the existing isPathInside(resolvedWorkdir,
resolvedCwd) check and return null if the resolved path is outside.
In `@src/main/presenter/llmProviderPresenter/acp/acpSessionManager.ts`:
- Around line 357-359: The detachHandlers disposal loop can throw and abort the
fallback to creating a new session; update the code around
detachHandlers?.forEach((dispose) => dispose()) so each disposer call is wrapped
in its own try/catch (or otherwise guarded) to swallow/non-fatally log errors
and continue, then set detachHandlers = undefined and call
this.processManager.clearSession(persistedSessionId) regardless; ensure you
reference and modify the dispose invocation site (detachHandlers.forEach) and
preserve the subsequent clearSession(persistedSessionId) and newSession fallback
flow.
- Around line 202-205: The catch currently calls await
this.processManager.unbindProcess(agent.id, conversationId) and then throws
error, but if unbindProcess itself throws it will replace the original
initialization error; change the catch to capture the original initialization
error into a variable (e.g., const initError = error), then call unbindProcess
inside its own try/catch and if unbindProcess throws, log or attach that cleanup
error but rethrow the original initError so the original failure is preserved;
update the catch block around the promise where
this.processManager.unbindProcess(agent.id, conversationId) is invoked to
implement this pattern.
In `@src/main/presenter/llmProviderPresenter/providers/acpProvider.ts`:
- Around line 894-900: The code is calling sessionPersistence.startTurn and
finishTurn fire-and-forget which can drop errors and allow races; change those
calls to await the Promise returned by sessionPersistence.startTurn(...) and
sessionPersistence.finishTurn(...) and wrap them in try/catch so persistence
errors are logged/propagated instead of being silent; ensure any code that calls
finishTurn awaits the awaited startTurn when sequencing is required (e.g., in
the same flow that uses startTurn/finishTurn), and apply the same change to the
other occurrences referenced by startTurn/finishTurn in this file (the blocks
around the other spots noted).
In `@src/renderer/src/i18n/da-DK/settings.json`:
- Line 1310: The "lifecycle" label in the settings i18n JSON is still in
English; update the value for the "lifecycle" key in
src/renderer/src/i18n/da-DK/settings.json (the JSON entry with "lifecycle":
"Lifecycle") to the Danish translation (e.g., "lifecycle": "Livscyklus") so the
event-kind labels stay localized and consistent with the surrounding entries.
In `@src/renderer/src/i18n/fa-IR/settings.json`:
- Line 1403: The "lifecycle" label in settings.json is still English; update the
value for the "lifecycle" key in src/renderer/src/i18n/fa-IR/settings.json to a
Persian translation to match surrounding entries (e.g., replace "Lifecycle" with
the appropriate Persian term), ensuring the JSON string remains properly quoted
and the file stays valid.
In `@src/renderer/src/i18n/fr-FR/settings.json`:
- Line 1403: Update the fr-FR localization for the key "lifecycle" in
settings.json by replacing the English value "Lifecycle" with the French
translation (e.g., "Cycle de vie") so the "lifecycle" label is localized
consistently in the fr-FR locale.
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Line 1377: The "lifecycle" label in eventKinds (key "lifecycle") is still
English; replace the value "Lifecycle" with the appropriate Hebrew translation
to match sibling labels (update the "lifecycle" entry so eventKinds.lifecycle
shows the Hebrew string).
In `@src/renderer/src/i18n/ja-JP/settings.json`:
- Line 1403: The "lifecycle" string key is still in English; update the
localization for the "lifecycle" key in the ja-JP settings JSON (the "lifecycle"
property) to a Japanese translation such as "ライフサイクル" so it matches adjacent
event-kind labels and UI consistency.
In `@src/renderer/src/i18n/ko-KR/settings.json`:
- Line 1403: The "lifecycle" label in the ko-KR localization is still in
English; update the settings.json entry for the "lifecycle" key to a Korean
translation (for example, replace "Lifecycle" with "라이프사이클" or another
appropriate Korean term) so the event-kind labels are fully localized; locate
the "lifecycle" key in src/renderer/src/i18n/ko-KR/settings.json and change its
value to the Korean string.
In `@src/renderer/src/i18n/pt-BR/settings.json`:
- Line 1403: The "lifecycle" entry in the pt-BR locale map is still in English;
update the value for the "lifecycle" key in the settings.json locale object to
Portuguese (e.g., "Ciclo de vida") so the UI string for lifecycle is fully
localized in pt-BR and matches the other translated keys.
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Line 1403: The "lifecycle" entry in the Russian locale map is still in
English; update the value for the "lifecycle" key (the string "Lifecycle") to
the appropriate Russian translation (e.g., "Жизненный цикл") so the ru-RU
settings.json contains fully localized text for the lifecycle label.
---
Outside diff comments:
In `@src/main/presenter/llmProviderPresenter/acp/acpProcessManager.ts`:
- Around line 704-715: The timeout created in timeoutPromise uses setTimeout but
never clears the timer, leaving a dangling handle; update the logic around
Promise.race([initPromise, timeoutPromise]) (symbols: timeoutPromise,
initPromise, setTimeout, timeoutMs, agent.id) to capture the timer id when
creating the timeout and ensure clearTimeout(timerId) is called once the race
settles (either in a finally block or immediately after awaiting initResult) so
the timer is always cleared whether initPromise wins or the timeout rejects.
---
Nitpick comments:
In `@src/main/presenter/acpClientPresenter/types.ts`:
- Around line 28-42: You have duplicated the DeepChatAgentEvent union locally
which overlaps the shared contract; remove the local type declaration in
types.ts and instead import the canonical DeepChatAgentEvent from the shared
contracts module (the exported union for agent events) and re-export or use that
type from here, ensuring all references in this file point to the imported
DeepChatAgentEvent; this keeps a single source of truth and prevents drift
between the local types and the shared contract.
🪄 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: 8fffba05-13fb-4af4-9941-35787f30be3c
📒 Files selected for processing (47)
docs/architecture/acp-client-runtime/plan.mddocs/architecture/acp-client-runtime/spec.mddocs/architecture/acp-client-runtime/tasks.mddocs/issues/acp-context-budget-bypass/plan.mddocs/issues/acp-context-budget-bypass/spec.mddocs/issues/acp-context-budget-bypass/tasks.mdsrc/main/presenter/acpClientPresenter/connection/AcpConnectionManager.tssrc/main/presenter/acpClientPresenter/connection/AcpDebugLog.tssrc/main/presenter/acpClientPresenter/handlers/index.tssrc/main/presenter/acpClientPresenter/index.tssrc/main/presenter/acpClientPresenter/mapper/AcpEventMapper.tssrc/main/presenter/acpClientPresenter/registry/index.tssrc/main/presenter/acpClientPresenter/session/AcpPromptController.tssrc/main/presenter/acpClientPresenter/session/AcpSessionRuntime.tssrc/main/presenter/acpClientPresenter/types.tssrc/main/presenter/acpClientPresenter/workspace/AcpPathGuard.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/process.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/llmProviderPresenter/acp/acpFsHandler.tssrc/main/presenter/llmProviderPresenter/acp/acpProcessManager.tssrc/main/presenter/llmProviderPresenter/acp/acpSessionManager.tssrc/main/presenter/llmProviderPresenter/acp/acpSessionPersistence.tssrc/main/presenter/llmProviderPresenter/providers/acpProvider.tssrc/main/presenter/sqlitePresenter/index.tssrc/main/presenter/sqlitePresenter/schemaCatalog.tssrc/main/presenter/sqlitePresenter/tables/acpTurns.tssrc/renderer/settings/components/AcpDebugDialog.vuesrc/renderer/settings/components/AcpSettings.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/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/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/contracts/acp.tssrc/shared/types/presenters/legacy.presenters.d.tstest/main/presenter/acpProvider.test.tstest/main/presenter/agentRuntimePresenter/agentRuntimePresenter.test.tstest/main/presenter/llmProviderPresenter/acp/acpProcessManager.test.tstest/main/presenter/llmProviderPresenter/acpFsHandler.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/presenter/llmProviderPresenter/acp/acpProcessManager.ts`:
- Around line 240-243: In the catch block that handles canonicalization of the
terminal cwd in acpProcessManager.ts (the block catching errors for variable cwd
and returning resolvedCwd), change the fallback to return null instead of
resolvedCwd so callers can trigger their fallback-to-session-workdir logic;
ensure the catch still logs the error (e.g., the existing console.warn using cwd
and error) but returns null to avoid propagating an invalid/unsafe resolvedCwd
value.
🪄 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: 671c80b3-754b-403f-85a0-c9f4a89407c4
📒 Files selected for processing (18)
docs/issues/acp-pr-1614-review-fixes/plan.mddocs/issues/acp-pr-1614-review-fixes/spec.mddocs/issues/acp-pr-1614-review-fixes/tasks.mdsrc/main/presenter/acpClientPresenter/types.tssrc/main/presenter/llmProviderPresenter/acp/acpProcessManager.tssrc/main/presenter/llmProviderPresenter/acp/acpSessionManager.tssrc/main/presenter/llmProviderPresenter/providers/acpProvider.tssrc/renderer/src/i18n/da-DK/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/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsontest/main/presenter/acpProvider.test.tstest/main/presenter/acpSessionManager.test.tstest/main/presenter/llmProviderPresenter/acp/acpProcessManager.test.ts
✅ Files skipped from review due to trivial changes (8)
- docs/issues/acp-pr-1614-review-fixes/tasks.md
- docs/issues/acp-pr-1614-review-fixes/plan.md
- src/renderer/src/i18n/ko-KR/settings.json
- src/main/presenter/acpClientPresenter/types.ts
- src/renderer/src/i18n/pt-BR/settings.json
- src/renderer/src/i18n/fr-FR/settings.json
- src/renderer/src/i18n/ja-JP/settings.json
- src/renderer/src/i18n/he-IL/settings.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/renderer/src/i18n/fa-IR/settings.json
- src/renderer/src/i18n/ru-RU/settings.json
- test/main/presenter/llmProviderPresenter/acp/acpProcessManager.test.ts
- src/main/presenter/llmProviderPresenter/acp/acpSessionManager.ts
- src/main/presenter/llmProviderPresenter/providers/acpProvider.ts
Comment on lines
+240
to
+243
| } catch (error) { | ||
| console.warn(`[ACP] Failed to resolve terminal cwd "${cwd}":`, error) | ||
| return resolvedCwd | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve safe fallback when cwd canonicalization fails.
If canonicalization throws, this currently returns resolvedCwd, which bypasses the caller’s fallback-to-session-workdir path. Return null here so invalid/unsafe error-path cwd values cannot slip through.
Suggested fix
} catch (error) {
console.warn(`[ACP] Failed to resolve terminal cwd "${cwd}":`, error)
- return resolvedCwd
+ return null
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (error) { | |
| console.warn(`[ACP] Failed to resolve terminal cwd "${cwd}":`, error) | |
| return resolvedCwd | |
| } | |
| } catch (error) { | |
| console.warn(`[ACP] Failed to resolve terminal cwd "${cwd}":`, error) | |
| return null | |
| } |
🤖 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/llmProviderPresenter/acp/acpProcessManager.ts` around
lines 240 - 243, In the catch block that handles canonicalization of the
terminal cwd in acpProcessManager.ts (the block catching errors for variable cwd
and returning resolvedCwd), change the fallback to return null instead of
resolvedCwd so callers can trigger their fallback-to-session-workdir logic;
ensure the catch still logs the error (e.g., the existing console.warn using cwd
and error) but returns null to avoid propagating an invalid/unsafe resolvedCwd
value.
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 by CodeRabbit
New Features
Bug Fixes
Documentation
Tests