feat(acp): harden v1 protocol support#1724
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates ACP v1 reliability changes: capability snapshot/types, capability-driven formatting, session resumption and buffered session updates, terminal spawn/output fixes, remote session sync and metadata merging, SQLite schema/migration, debug actions/UI, i18n entries, tests, and design docs. ChangesACP v1 Reliability Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/settings/components/AcpDebugDialog.vue (1)
543-555:⚠️ Potential issue | 🔴 CriticalThe ACP debug provider dispatches
sessionResume,sessionClose, andsessionForkusing the top-levelAcpDebugRequest.sessionId(each case throws whensessionIdis missing).runAcpDebugActionis a pass-through toprovider.runDebugAction(request), so unlessAcpDebugDialogsynchronizes the editedpayload.sessionIdinto the top-levelsessionId, the user-edited value won’t be the one targeted by these actions.🤖 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/renderer/settings/components/AcpDebugDialog.vue` around lines 543 - 555, The code passes a top-level sessionId computed from requiresSession/debugSessionId while the actual user-edited session id may live inside parsedPayload, so update the sessionId computation before calling runAcpDebugAction to prefer any sessionId present in parsedPayload (e.g. use parsedPayload.sessionId if set, otherwise fall back to requiresSession.value ? debugSessionId.value : undefined), ensuring you coerce empty strings to undefined; keep payloadToSend = applyWorkdirToPayload(parsedPayload) as-is and then call llmProviderPresenter.runAcpDebugAction(...) so sessionResume/sessionClose/sessionFork use the user-edited session id.
🧹 Nitpick comments (1)
src/main/presenter/llmProviderPresenter/acp/acpMessageFormatter.ts (1)
97-108: 💤 Low value
input_imagetype may not extract data correctly from all input formats.The code handles
input_imageby looking forpart.image_url.url, but someinput_imageformats (e.g., Anthropic-style) store data differently (e.g.,part.source.dataorpart.data). Currently, if the image data is not underimage_url.url, the content will be silently dropped.If this is intentional (only supporting OpenAI-style
image_urlformat), consider adding a comment. Otherwise, consider handling additional formats.🤖 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/acpMessageFormatter.ts` around lines 97 - 108, The image handling branch for types 'image_url' and 'input_image' only reads part.image_url.url and drops other shapes; update the branch in acpMessageFormatter (the if (type === 'image_url' || type === 'input_image') block) to also probe alternative fields such as part.source?.data, part.data, and top-level URLs before returning, call this.parseDataUrl on any discovered string/blob, and push either normalized image ({ type: 'image', data, mimeType, uri }) or resource_link as currently done; if you intend to limit support to image_url only, instead add a clear comment above the block documenting the supported input formats.
🤖 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/acp-v1-reliability/plan.md`:
- Line 1: The SDD currently placed as
docs/architecture/acp-v1-reliability/plan.md belongs under the feature docs
path; move the entire document set from docs/architecture/acp-v1-reliability/
into docs/features/acp-v1-reliability/ (kebab-case) to match the repo taxonomy
for a feat(acp) change, and update any internal links, TOC entries, and
cross-references that pointed to the old docs/architecture location so they now
point to docs/features/acp-v1-reliability/plan.md.
In `@src/main/presenter/llmProviderPresenter/acp/acpCapabilities.ts`:
- Around line 74-77: The auth capability is being set when
options.enableTerminalAuth is true even if terminal support is disabled; update
the condition that assigns caps.auth (in acpCapabilities.ts where caps and
options are used) to require both options.enableTerminal and
options.enableTerminalAuth before setting caps.auth = { terminal: true },
otherwise omit the terminal auth entry so the client does not advertise an
unsupported auth flow.
In `@src/main/presenter/llmProviderPresenter/acp/acpProcessManager.ts`:
- Around line 1679-1694: flushBufferedSessionUpdates currently replays all
queued notifications regardless of age because pruneBufferedSessionUpdates only
runs when new notifications arrive; update flushBufferedSessionUpdates (and the
analogous block around lines 1696-1707) to drop entries older than
SESSION_UPDATE_BUFFER_TTL_MS before delivering: read buffered entries from
bufferedSessionUpdates, filter by (now - entry.timestamp) <=
SESSION_UPDATE_BUFFER_TTL_MS, delete the original buffer, log flushed vs expired
counts via debugLog.append (use same lifecycle action names), and only call
deliverSessionUpdate(entry, notification) for remaining non-expired
notifications; reference functions/variables: flushBufferedSessionUpdates,
pruneBufferedSessionUpdates, bufferedSessionUpdates,
SESSION_UPDATE_BUFFER_TTL_MS, deliverSessionUpdate, debugLog.append,
sessionListeners.
In `@src/main/presenter/llmProviderPresenter/acp/acpSessionPersistence.ts`:
- Around line 126-149: The update path is overwriting an existing session's
workdir with a guessed input.workdir when remoteSession.cwd is missing; change
the logic in the existing-session branch (use resolveRemoteSessionWorkdir and
saveSessionData) to preserve existing.workdir unless remoteSession.cwd is
explicitly provided—i.e., compute sessionWorkdir but when calling
saveSessionData for an existing record, pass existing.workdir if
remoteSession.cwd is undefined/null so the stored workdir is only changed when
the remote session actually reports a cwd; keep the acpSync merge behavior
(getAcpSessionByAgentAndSessionId, getRecord, and acpSync.lastSyncedAt) the
same.
- Around line 91-102: mergeMetadata() does a read-merge-write and races when
AcpProvider.handleSessionUpdate() calls it concurrently; replace this with a
per-session serialized update or an atomic DB merge. Implement either a
per-session mutex (e.g., a Map keyed by conversationId+agentId to serialize
async calls around getSessionData/saveSessionData) or, preferably, add an atomic
SQL merge in acpSessionPersistence (create a mergeMetadataAtomic function) that
runs a single UPDATE using SQLite JSON functions (e.g., UPDATE ... SET metadata
= json_patch(coalesce(metadata, '{}'), ?) WHERE conversationId = ? AND agentId =
?) so concurrent callers cannot overwrite newer metadata, and have
AcpProvider.handleSessionUpdate() call that atomic merge instead of the current
read-merge-write sequence.
In `@src/main/presenter/llmProviderPresenter/providers/acpProvider.ts`:
- Around line 421-428: The session.systemPromptSent flag is being set before the
prompt is actually dispatched, so move the assignment to after a successful
dispatch: do not set session.systemPromptSent in the block that computes
formattedPrompt; instead set it only after runPrompt (or inside runPrompt)
confirms the underlying connection.prompt/dispatch succeeded (e.g., after the
promise resolves or after successful turn persistence). Update the logic in
runPrompt (or its prompt-dispatch success callback) to mark
session.systemPromptSent = true when the prompt has been sent/persisted to
ensure retries still include the system prompt.
In `@src/main/presenter/sqlitePresenter/tables/acpSessions.ts`:
- Around line 86-99: The table enforces a global unique on session_id which
conflicts with the agent-scoped lookup in getByAgentAndSessionId and will cause
insert failures when different agents use the same remote session id; change the
schema for acp_sessions to remove the inline UNIQUE on session_id and instead
enforce uniqueness on the composite (agent_id, session_id) (or drop uniqueness
entirely if intended), update the table creation/DDL where acp_sessions is
defined to use UNIQUE(agent_id, session_id), and add a migration that
rebuilds/transfers data into a new table (or recreates the table) to remove the
old inline constraint so existing databases are updated safely.
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Around line 1646-1650: The Russian locale file contains raw protocol strings
for ACP debug method labels ("authenticate", "sessionClose", "sessionFork",
"sessionList", "sessionResume") instead of localized Russian text; update these
keys in src/renderer/src/i18n/ru-RU/settings.json to provide proper Russian
translations for each label (replace the raw values with localized strings that
match the style of surrounding entries) so the debug method picker shows
translated labels for Russian users.
In `@src/renderer/src/i18n/tr-TR/settings.json`:
- Around line 1674-1678: The new ACL debug keys ("authenticate", "sessionClose",
"sessionFork", "sessionList", "sessionResume") are left as raw protocol strings;
replace their English/raw values with Turkish localized labels so the tr-TR UI
shows translations (e.g., provide Turkish equivalents for authenticate,
session/close, session/fork, session/list, session/resume) by updating the
corresponding JSON entries to localized Turkish phrases while keeping the same
keys.
---
Outside diff comments:
In `@src/renderer/settings/components/AcpDebugDialog.vue`:
- Around line 543-555: The code passes a top-level sessionId computed from
requiresSession/debugSessionId while the actual user-edited session id may live
inside parsedPayload, so update the sessionId computation before calling
runAcpDebugAction to prefer any sessionId present in parsedPayload (e.g. use
parsedPayload.sessionId if set, otherwise fall back to requiresSession.value ?
debugSessionId.value : undefined), ensuring you coerce empty strings to
undefined; keep payloadToSend = applyWorkdirToPayload(parsedPayload) as-is and
then call llmProviderPresenter.runAcpDebugAction(...) so
sessionResume/sessionClose/sessionFork use the user-edited session id.
---
Nitpick comments:
In `@src/main/presenter/llmProviderPresenter/acp/acpMessageFormatter.ts`:
- Around line 97-108: The image handling branch for types 'image_url' and
'input_image' only reads part.image_url.url and drops other shapes; update the
branch in acpMessageFormatter (the if (type === 'image_url' || type ===
'input_image') block) to also probe alternative fields such as
part.source?.data, part.data, and top-level URLs before returning, call
this.parseDataUrl on any discovered string/blob, and push either normalized
image ({ type: 'image', data, mimeType, uri }) or resource_link as currently
done; if you intend to limit support to image_url only, instead add a clear
comment above the block documenting the supported input formats.
🪄 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: d7a5ad9c-21a7-4af6-bfe4-f8154338bac2
📒 Files selected for processing (44)
docs/architecture/acp-v1-reliability/plan.mddocs/architecture/acp-v1-reliability/spec.mddocs/architecture/acp-v1-reliability/tasks.mdsrc/main/presenter/llmProviderPresenter/acp/acpCapabilities.tssrc/main/presenter/llmProviderPresenter/acp/acpContentMapper.tssrc/main/presenter/llmProviderPresenter/acp/acpMessageFormatter.tssrc/main/presenter/llmProviderPresenter/acp/acpProcessManager.tssrc/main/presenter/llmProviderPresenter/acp/acpSessionManager.tssrc/main/presenter/llmProviderPresenter/acp/acpSessionPersistence.tssrc/main/presenter/llmProviderPresenter/acp/acpTerminalManager.tssrc/main/presenter/llmProviderPresenter/acp/index.tssrc/main/presenter/llmProviderPresenter/providers/acpProvider.tssrc/main/presenter/sqlitePresenter/index.tssrc/main/presenter/sqlitePresenter/tables/acpSessions.tssrc/renderer/settings/components/AcpDebugDialog.vuesrc/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/shared/types/presenters/legacy.presenters.d.tstest/main/presenter/acpProvider.test.tstest/main/presenter/llmProviderPresenter/acp/acpProcessManager.test.tstest/main/presenter/llmProviderPresenter/acp/acpProcessManagerCapabilities.test.tstest/main/presenter/llmProviderPresenter/acp/acpTerminalManager.test.tstest/main/presenter/llmProviderPresenter/acpCapabilities.test.tstest/main/presenter/llmProviderPresenter/acpContentMapper.test.tstest/main/presenter/llmProviderPresenter/acpMessageFormatter.test.tstest/main/presenter/llmProviderPresenter/acpSessionPersistence.test.ts
Contributor
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/llmProviderPresenter/acp/acpSessionPersistence.ts (1)
165-204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback the imported conversation on non-recoverable link-save failures.
After
createConversation()succeeds, anysaveSessionData()error that is not the concurrent-link case rethrows without deletingconversationId. That leaves a local conversation with no ACP session row and no later sync path to reconcile it.Suggested fix
} catch (error) { const concurrentExisting = await this.sqlitePresenter.getAcpSessionByAgentAndSessionId( input.agentId, remoteSession.sessionId ) if (!concurrentExisting) { + await this.deleteConversationSilently(conversationId) throw error } await this.deleteConversationSilently(conversationId)🤖 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/acpSessionPersistence.ts` around lines 165 - 204, After createConversation succeeds, saveSessionData errors currently rethrow without cleaning up; modify the catch in acpSessionPersistence so that when getAcpSessionByAgentAndSessionId returns null (i.e. not the concurrent-link case) you call deleteConversationSilently(conversationId) before rethrowing the error. Keep the existing concurrent case flow that calls updateRemoteSessionLink(...) when concurrentExisting exists; ensure you reference the existing methods createConversation, saveSessionData, getAcpSessionByAgentAndSessionId, deleteConversationSilently, and updateRemoteSessionLink to implement this rollback.
♻️ Duplicate comments (3)
src/main/presenter/llmProviderPresenter/acp/acpSessionPersistence.ts (1)
89-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize
mergeMetadata()per conversation/agent.This is still a read-merge-write sequence, and
AcpProvider.handleSessionUpdate()calls it fire-and-forget. Two closesessionInfo/usagenotifications can read the same row and the slower save will overwrite newer metadata.🤖 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/acpSessionPersistence.ts` around lines 89 - 105, mergeMetadata currently does a read-merge-write (getSessionData -> merge -> saveSessionData) and can race when AcpProvider.handleSessionUpdate calls it fire-and-forget; make mergeMetadata serialized per (conversationId, agentId) by acquiring a per-key lock or using a DB-level atomic update before reading/merging: implement a mutex keyed on `${conversationId}:${agentId}` (or replace the read-merge-write with an atomic JSON merge/update in your persistence layer) so mergeMetadata re-reads under the lock, merges existing.metadata with the new metadata, calls saveSessionData, and then releases the lock to prevent concurrent overwrites.src/main/presenter/llmProviderPresenter/providers/acpProvider.ts (1)
421-428:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMark
systemPromptSentonly after the prompt is actually dispatched.This flag still flips before
runPrompt()reachesconnection.prompt(). If turn persistence or request tracing fails first, the retry path will skip the system prompt even though the agent never received 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/llmProviderPresenter/providers/acpProvider.ts` around lines 421 - 428, The code sets session.systemPromptSent before the prompt is actually dispatched; instead remove the pre-call assignment and only mark session.systemPromptSent true after a successful send inside the async prompt path—either await runPrompt and set it after it resolves successfully, or set the flag in runPrompt (or the underlying connection.prompt success handler) immediately after a successful transmission. Update the call site around formattedPrompt and runPrompt (remove pre-call mutation of session.systemPromptSent), and implement the post-send assignment in runPrompt or the connection.prompt success branch so retries won't skip the system prompt.src/main/presenter/llmProviderPresenter/acp/acpProcessManager.ts (1)
1691-1705:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrune expired buffered updates before replaying them.
The TTL is only enforced when buffering. If no new notification arrives, a listener that binds later still gets stale session updates here, which can resurrect old mode/config state after the 30s buffer window.
Suggested fix
private flushBufferedSessionUpdates(sessionId: string): void { const entry = this.sessionListeners.get(sessionId) if (!entry) return - const buffered = this.bufferedSessionUpdates.get(sessionId) - if (!buffered?.length) return + this.pruneBufferedSessionUpdates() + const buffered = this.bufferedSessionUpdates.get(sessionId) + if (!buffered?.length) { + this.bufferedSessionUpdates.delete(sessionId) + return + } this.bufferedSessionUpdates.delete(sessionId) this.debugLog.append(entry.agentId, { kind: 'lifecycle', action: 'session/update.buffer.flush', sessionId, - payload: { count: buffered.length } + payload: { count: buffered.length } }) buffered.forEach(({ notification }) => this.deliverSessionUpdate(entry, notification)) }🤖 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 1691 - 1705, The flushBufferedSessionUpdates method currently replays all buffered notifications even if some have expired; before replaying, filter the array retrieved from this.bufferedSessionUpdates.get(sessionId) to remove entries whose timestamp is older than the session-update TTL (use the same TTL/check logic you use when initially buffering updates), then set this.bufferedSessionUpdates.delete(sessionId) and log/flush only the remaining (non-expired) entries by calling this.deliverSessionUpdate(entry, notification); also ensure the debugLog payload count uses the count of replayed (filtered) items and discard expired items without delivery.
🤖 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/providers/acpProvider.ts`:
- Around line 676-694: The current resolveWorkdir and resolvePayloadWorkdir read
request.workdir directly, which can override the canonicalized workdir in the
established handle; update both to prefer handle.workdir when handle exists (use
the handle as the source of truth after getConnection() canonicalizes it) and
only fall back to request.workdir when handle.workdir is absent, while still
passing the chosen value through normalizeWorkdir (which should continue to call
sessionPersistence.resolveWorkdir when available); adjust resolveWorkdir to base
cwd on handle.workdir ?? request.workdir and resolvePayloadWorkdir to prefer
handle.workdir over the raw incoming workdir before normalization.
---
Outside diff comments:
In `@src/main/presenter/llmProviderPresenter/acp/acpSessionPersistence.ts`:
- Around line 165-204: After createConversation succeeds, saveSessionData errors
currently rethrow without cleaning up; modify the catch in acpSessionPersistence
so that when getAcpSessionByAgentAndSessionId returns null (i.e. not the
concurrent-link case) you call deleteConversationSilently(conversationId) before
rethrowing the error. Keep the existing concurrent case flow that calls
updateRemoteSessionLink(...) when concurrentExisting exists; ensure you
reference the existing methods createConversation, saveSessionData,
getAcpSessionByAgentAndSessionId, deleteConversationSilently, and
updateRemoteSessionLink to implement this rollback.
---
Duplicate comments:
In `@src/main/presenter/llmProviderPresenter/acp/acpProcessManager.ts`:
- Around line 1691-1705: The flushBufferedSessionUpdates method currently
replays all buffered notifications even if some have expired; before replaying,
filter the array retrieved from this.bufferedSessionUpdates.get(sessionId) to
remove entries whose timestamp is older than the session-update TTL (use the
same TTL/check logic you use when initially buffering updates), then set
this.bufferedSessionUpdates.delete(sessionId) and log/flush only the remaining
(non-expired) entries by calling this.deliverSessionUpdate(entry, notification);
also ensure the debugLog payload count uses the count of replayed (filtered)
items and discard expired items without delivery.
In `@src/main/presenter/llmProviderPresenter/acp/acpSessionPersistence.ts`:
- Around line 89-105: mergeMetadata currently does a read-merge-write
(getSessionData -> merge -> saveSessionData) and can race when
AcpProvider.handleSessionUpdate calls it fire-and-forget; make mergeMetadata
serialized per (conversationId, agentId) by acquiring a per-key lock or using a
DB-level atomic update before reading/merging: implement a mutex keyed on
`${conversationId}:${agentId}` (or replace the read-merge-write with an atomic
JSON merge/update in your persistence layer) so mergeMetadata re-reads under the
lock, merges existing.metadata with the new metadata, calls saveSessionData, and
then releases the lock to prevent concurrent overwrites.
In `@src/main/presenter/llmProviderPresenter/providers/acpProvider.ts`:
- Around line 421-428: The code sets session.systemPromptSent before the prompt
is actually dispatched; instead remove the pre-call assignment and only mark
session.systemPromptSent true after a successful send inside the async prompt
path—either await runPrompt and set it after it resolves successfully, or set
the flag in runPrompt (or the underlying connection.prompt success handler)
immediately after a successful transmission. Update the call site around
formattedPrompt and runPrompt (remove pre-call mutation of
session.systemPromptSent), and implement the post-send assignment in runPrompt
or the connection.prompt success branch so retries won't skip the system prompt.
🪄 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: 53bc568a-da7c-4123-bcd5-bcd2e2838416
📒 Files selected for processing (9)
src/main/presenter/llmProviderPresenter/acp/acpProcessManager.tssrc/main/presenter/llmProviderPresenter/acp/acpSessionPersistence.tssrc/main/presenter/llmProviderPresenter/index.tssrc/main/presenter/llmProviderPresenter/providers/acpProvider.tssrc/main/presenter/sqlitePresenter/tables/acpSessions.tstest/main/presenter/acpProvider.test.tstest/main/presenter/llmProviderPresenter/acp/acpProcessManager.test.tstest/main/presenter/llmProviderPresenter/acpSessionPersistence.test.tstest/main/presenter/sqlitePresenter/acpSessions.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/main/presenter/llmProviderPresenter/acp/acpProcessManager.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/providers/acpProvider.ts`:
- Around line 684-716: The resolveHandleWorkdir function currently returns
handle.workdir directly which can be stale; instead trim and revalidate
handle.workdir before returning it by calling
this.sessionPersistence.isWorkdirUsable (if available) and only return the
cached value when usable; if not usable, fall back to request.workdir (via
this.sessionPersistence.resolveWorkdir when available) or process.cwd()—apply
the same validation logic referenced in normalizeWorkdir (use
resolveHandleWorkdir, normalizeWorkdir, sessionPersistence.isWorkdirUsable, and
sessionPersistence.resolveWorkdir names to locate and update the code).
🪄 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: 00e62c28-b5ff-49c8-9689-135cc8d2f746
📒 Files selected for processing (16)
docs/features/acp-v1-reliability/plan.mddocs/features/acp-v1-reliability/spec.mddocs/features/acp-v1-reliability/tasks.mdsrc/main/presenter/llmProviderPresenter/acp/acpCapabilities.tssrc/main/presenter/llmProviderPresenter/acp/acpMessageFormatter.tssrc/main/presenter/llmProviderPresenter/acp/acpProcessManager.tssrc/main/presenter/llmProviderPresenter/acp/acpSessionPersistence.tssrc/main/presenter/llmProviderPresenter/providers/acpProvider.tssrc/renderer/settings/components/AcpDebugDialog.vuesrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/settings.jsontest/main/presenter/acpProvider.test.tstest/main/presenter/llmProviderPresenter/acp/acpProcessManager.test.tstest/main/presenter/llmProviderPresenter/acpCapabilities.test.tstest/main/presenter/llmProviderPresenter/acpMessageFormatter.test.tstest/main/presenter/llmProviderPresenter/acpSessionPersistence.test.ts
💤 Files with no reviewable changes (3)
- docs/features/acp-v1-reliability/tasks.md
- docs/features/acp-v1-reliability/spec.md
- docs/features/acp-v1-reliability/plan.md
✅ Files skipped from review due to trivial changes (1)
- src/renderer/src/i18n/ru-RU/settings.json
🚧 Files skipped from review as they are similar to previous changes (8)
- src/renderer/src/i18n/tr-TR/settings.json
- test/main/presenter/llmProviderPresenter/acpMessageFormatter.test.ts
- test/main/presenter/llmProviderPresenter/acpCapabilities.test.ts
- src/main/presenter/llmProviderPresenter/acp/acpCapabilities.ts
- src/renderer/settings/components/AcpDebugDialog.vue
- src/main/presenter/llmProviderPresenter/acp/acpMessageFormatter.ts
- src/main/presenter/llmProviderPresenter/acp/acpSessionPersistence.ts
- src/main/presenter/llmProviderPresenter/acp/acpProcessManager.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/main/presenter/acpProvider.test.ts (1)
1-1:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRelocate test file to mirror source structure.
The test file should mirror the source file structure. The source file is at
src/main/presenter/llmProviderPresenter/providers/acpProvider.ts, so this test should be attest/main/presenter/llmProviderPresenter/providers/acpProvider.test.tsinstead oftest/main/presenter/acpProvider.test.ts.As per coding guidelines, tests should be located in
test/main/**ortest/renderer/**mirroring source structure with names*.test.tsor*.spec.ts.🤖 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/presenter/acpProvider.test.ts` at line 1, Move the test file test/main/presenter/acpProvider.test.ts so its path mirrors the source module: place it at test/main/presenter/llmProviderPresenter/providers/acpProvider.test.ts next to the source src/main/presenter/llmProviderPresenter/providers/acpProvider.ts; update any relative imports inside the test (e.g., imports that reference acpProvider or other presenter modules) to the new relative path and ensure the test filename remains acpProvider.test.ts to satisfy the test runner naming convention.
🤖 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.
Outside diff comments:
In `@test/main/presenter/acpProvider.test.ts`:
- Line 1: Move the test file test/main/presenter/acpProvider.test.ts so its path
mirrors the source module: place it at
test/main/presenter/llmProviderPresenter/providers/acpProvider.test.ts next to
the source src/main/presenter/llmProviderPresenter/providers/acpProvider.ts;
update any relative imports inside the test (e.g., imports that reference
acpProvider or other presenter modules) to the new relative path and ensure the
test filename remains acpProvider.test.ts to satisfy the test runner naming
convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2373f8e-f3c1-4dcb-b824-7e6f76a325f7
📒 Files selected for processing (2)
src/main/presenter/llmProviderPresenter/providers/acpProvider.tstest/main/presenter/acpProvider.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/presenter/llmProviderPresenter/providers/acpProvider.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
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
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Localization