Initial attempt at ACP support in CLI #4256
Conversation
|
lambertjosh
force-pushed
the
jl-acp-prototype
branch
from
bcdb73f to
681d753
Compare
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ 3 Issues Found
| Severity | Issue | Location |
|---|---|---|
| WARNING | Hardcoded local path in debug script | cli/scripts/acp-debug.sh |
| WARNING | yoloMode bypasses security approvals | cli/src/acp/agent.ts:127 |
| SUGGESTION | Sessions never cleaned up (memory leak) | cli/src/acp/agent.ts:135 |
Recommendation: Address the yoloMode security concern before merging to production. The hardcoded path should be fixed for portability.
Review Details (10 files)
Files: cli/src/acp/agent.ts (2 issues), cli/src/acp/index.ts, cli/src/acp/types.ts, cli/src/acp/tests/agent.test.ts, cli/src/index.ts, cli/scripts/acp-debug.sh (1 issue), cli/package.json, cli/package.dist.json, apps/kilocode-docs/docs/cli.md, pnpm-lock.yaml
Checked: Security, bugs, performance, error handling
Notes:
- Good test coverage for the new ACP agent functionality
- Documentation is comprehensive and well-structured
- Error handling is present throughout the implementation
- The ACP SDK integration follows reasonable patterns
There was a problem hiding this comment.
This script contains a hardcoded path /Users/silv/projects/kilocode/cli/dist/index.js which is developer-specific. Consider using a relative path or $(dirname "$0")/../dist/index.js instead.
|
|
||
| // TEMPORARY: Enable yoloMode to test if this unblocks LLM calls | ||
| // TODO: Remove this and properly handle approvals through ACP | ||
| const extensionHost = extensionService.getExtensionHost() |
There was a problem hiding this comment.
Enabling yoloMode bypasses all tool approval prompts, which defeats the human-in-the-loop security model documented for ACP. The TODO comment acknowledges this, but this should be addressed before merging to production.
| acpDebug("yoloMode enabled for testing") | ||
| } catch (error) { | ||
| const err = error as Error | ||
| acpDebug("Failed to create extension service:", err.message, err.stack) |
There was a problem hiding this comment.
💡 SUGGESTION: Potential memory leak - sessions never cleaned up
Sessions are added to this.sessions Map but never removed. Consider implementing session cleanup on disconnect or adding a destroySession method to prevent memory growth over time.
There was a problem hiding this comment.
✅ No New Issues Found (Incremental Review)
13 files reviewed | Confidence: 90% | Recommendation: Address previous issues before merge
Previous Issues (Still Unresolved)
| Severity | Issue | Location |
|---|---|---|
| WARNING | Hardcoded local path in debug script | cli/scripts/acp-debug.sh:3 |
| WARNING | yoloMode bypasses security approvals | cli/src/acp/agent.ts:127 |
| SUGGESTION | Sessions never cleaned up (memory leak) | cli/src/acp/agent.ts:135 |
The previous review correctly identified these issues. They should be addressed before merging to production.
Review Details (13 files)
Source Files:
cli/src/acp/agent.ts- ACP Agent implementation (2 previous issues)cli/src/acp/index.ts- ACP entry pointcli/src/acp/types.ts- Type definitionscli/src/index.ts- CLI flag additionscli/src/host/ExtensionHost.ts- Message deduplication improvementssrc/services/glob/list-files.ts- Ripgrep fallback (good defensive change)src/core/tools/AttemptCompletionTool.ts- Parameter cleanup in handlePartial()
Config/Docs:
cli/package.json,cli/package.dist.json- Added @agentclientprotocol/sdkapps/kilocode-docs/docs/cli.md- Comprehensive ACP documentationpnpm-lock.yaml- Lock file update
Scripts:
cli/scripts/acp-debug.sh- Debug wrapper (1 previous issue)
Tests:
cli/src/acp/__tests__/agent.test.ts- Good test coverage for ACP agent
Checked: Security, bugs, performance, error handling
Notes:
- The ACP implementation follows reasonable patterns
- Test coverage is good for the new functionality
- The ripgrep fallback in list-files.ts is a good defensive addition
- ExtensionHost message deduplication improvements look correct
Context
Exploration of ACP support for Kilo Code.
Implementation
See docs in PR. General approach is to add ACP support to the CLI, and to enable the behavior by using a flag.
Screenshots
How to Test
See docs.
Get in Touch