Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| let buffer = ""; | ||
| stream.on("data", (chunk: Buffer | string) => { | ||
| buffer += chunk.toString(); |
There was a problem hiding this comment.
🟢 Low Layers/CursorAdapter.ts:244
attachLineReader concatenates raw Buffer chunks as strings before buffering, so multi-byte UTF-8 characters split across chunk boundaries are corrupted. Each partial chunk is decoded independently, causing replacement characters or malformed sequences in the final buffer. Consider using stream.setEncoding('utf8') or string_decoder.StringDecoder to handle multi-byte characters correctly across chunks.
- let buffer = "";
- stream.on("data", (chunk: Buffer | string) => {
- buffer += chunk.toString();🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around lines 244-246:
`attachLineReader` concatenates raw `Buffer` chunks as strings before buffering, so multi-byte UTF-8 characters split across chunk boundaries are corrupted. Each partial chunk is decoded independently, causing replacement characters or malformed sequences in the final buffer. Consider using `stream.setEncoding('utf8')` or `string_decoder.StringDecoder` to handle multi-byte characters correctly across chunks.
Evidence trail:
apps/server/src/provider/Layers/CursorAdapter.ts lines 240-262 at REVIEWED_COMMIT. The `attachLineReader` function on line 240 handles stream data events at line 245-246 with `buffer += chunk.toString()` where chunk is `Buffer | string`. This pattern of calling toString() on each Buffer chunk independently before concatenation is the exact issue described - multi-byte UTF-8 characters split across chunk boundaries will be corrupted.
| ].join("\n"); | ||
| } | ||
|
|
||
| function buildCursorArgs(input: { |
There was a problem hiding this comment.
🟠 High Layers/CursorAdapter.ts:211
When buildCursorArgs returns the prompt as a raw command-line argument, and spawnProcess is invoked with shell: true on Windows (line 419), newlines in the prompt are interpreted by cmd.exe as command separators. This causes arbitrary command execution and guaranteed process failure on Windows. Additionally, prompts exceeding 8191 characters will crash on Windows due to command-line length limits. The prompt should be passed via standard input or a temporary file instead.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around line 211:
When `buildCursorArgs` returns the prompt as a raw command-line argument, and `spawnProcess` is invoked with `shell: true` on Windows (line 419), newlines in the prompt are interpreted by `cmd.exe` as command separators. This causes arbitrary command execution and guaranteed process failure on Windows. Additionally, prompts exceeding 8191 characters will crash on Windows due to command-line length limits. The prompt should be passed via standard input or a temporary file instead.
Evidence trail:
apps/server/src/provider/Layers/CursorAdapter.ts lines 211-223 (buildCursorArgs returns prompt as `-p` argument), lines 408-420 (spawnProcess call with shell: process.platform === "win32"), lines 199-209 (buildCursorPrompt adds newlines in plan mode), line 285 (spawnProcess defaults to Node.js spawn from line 3-4 import)
| function killChild(child: ChildProcessHandle, signal: NodeJS.Signals = "SIGTERM"): void { | ||
| if (process.platform === "win32" && child.pid !== undefined) { | ||
| try { | ||
| spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore" }); | ||
| return; | ||
| } catch { | ||
| // Fall back to direct kill. | ||
| } | ||
| } | ||
| child.kill(signal); | ||
| } |
There was a problem hiding this comment.
🟡 Medium Layers/CursorAdapter.ts:228
killChild uses spawnSync('taskkill', ...) on Windows and returns immediately if no exception is thrown, but spawnSync does not throw on failure — it returns an object with error or non-zero status. When taskkill fails (e.g., binary missing, access denied), the function silently exits without falling back to child.kill(), potentially leaving the child process running and the session stuck. Consider checking result.error or result.status !== 0 before returning, and fall back to child.kill() on any failure.
function killChild(child: ChildProcessHandle, signal: NodeJS.Signals = "SIGTERM"): void {
if (process.platform === "win32" && child.pid !== undefined) {
try {
- spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore" });
- return;
+ const result = spawnSync("taskkill", ["/pid", String(child.pid), "/T", "/F"], {
+ stdio: "ignore",
+ });
+ if (!result.error && result.status === 0) {
+ return;
+ }
} catch {
// Fall back to direct kill.
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around lines 228-238:
`killChild` uses `spawnSync('taskkill', ...)` on Windows and returns immediately if no exception is thrown, but `spawnSync` does not throw on failure — it returns an object with `error` or non-zero `status`. When `taskkill` fails (e.g., binary missing, access denied), the function silently exits without falling back to `child.kill()`, potentially leaving the child process running and the session stuck. Consider checking `result.error` or `result.status !== 0` before returning, and fall back to `child.kill()` on any failure.
Evidence trail:
1. Code at apps/server/src/provider/Layers/CursorAdapter.ts lines 228-238 shows `killChild` function using `try-catch` with `spawnSync` and returning immediately after the call.
2. Node.js documentation (https://nodejs.org/api/child_process.html) confirms that `spawnSync` returns an object with `status`, `signal`, and `error` properties instead of throwing exceptions on failure.
| const existing = sessions.get(input.threadId); | ||
| const createdAt = existing?.createdAt ?? nowIso(); | ||
| const updatedAt = nowIso(); |
There was a problem hiding this comment.
🟡 Medium Layers/CursorAdapter.ts:326
startSession overwrites an existing session in sessions without terminating its activeTurn. If a turn is running, the child process continues executing but becomes unreachable, causing zombie processes that exhaust server resources. Consider checking for and killing any existing active turn before replacing the session.
const existing = sessions.get(input.threadId);
+ if (existing?.activeTurn) {
+ killChild(existing.activeTurn.child);
+ }
const createdAt = existing?.createdAt ?? nowIso();🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around lines 326-328:
`startSession` overwrites an existing session in `sessions` without terminating its `activeTurn`. If a turn is running, the child process continues executing but becomes unreachable, causing zombie processes that exhaust server resources. Consider checking for and killing any existing active turn before replacing the session.
Evidence trail:
- apps/server/src/provider/Layers/CursorAdapter.ts lines 47-59 (CursorSessionState interface with activeTurn field)
- apps/server/src/provider/Layers/CursorAdapter.ts lines 315-372 (startSession function - creates `next` without activeTurn, sets it without killing existing activeTurn)
- apps/server/src/provider/Layers/CursorAdapter.ts lines 326, 331-355 (existing session is retrieved, but activeTurn is not preserved or terminated)
- apps/server/src/provider/Layers/CursorAdapter.ts lines 376-378 (example of proper activeTurn cleanup with killChild)
- apps/server/src/provider/Layers/CursorAdapter.ts lines 949-951, 968-970 (other places with proper cleanup)
|
The t3 team already have a cursor pr up #178, they are more likely to merge their own then yours |
|
@brrock Thanks! |
2 participants
Summary
Verification
Note
Add Cursor CLI provider support and register
CursorAdapterLiveinProviderAdapterRegistryLivevia ProviderAdapterRegistry.ts and wire providerOptions through orchestration and UIIntroduce a
cursorprovider with aCursorAdapterLivethat manages sessions and turns via the Cursor CLI, extend contracts to includecursorinProviderKindand providerOptions, wire providerOptions through orchestration, add health checks for the Cursor CLI, and expose cursor models and settings in the web UI.📍Where to Start
Start with
CursorAdapterLivein CursorAdapter.ts, then follow provider registration in ProviderAdapterRegistry.ts and providerOptions propagation in ProviderCommandReactor.ts.📊 Macroscope summarized 0255041. 19 files reviewed, 11 issues evaluated, 1 issue filtered, 4 comments posted
🗂️ Filtered Issues
apps/web/src/composerDraftStore.ts — 0 comments posted, 1 evaluated, 1 filtered
setProjectDraftThreadIdgarbage-collects an orphaned draft thread, it fails to revoke the Blob URLs associated with that draft's images before removing it from state. This causesblob:URLs to remain allocated in the browser's memory until the tab closes. Similar leaks exist inclearProjectDraftThreadIdandclearProjectDraftThreadById. You should iterate overstate.draftsByThreadId[previousThreadIdForProject].imagesand callrevokeObjectPreviewUrlfor each image before deleting the draft, matching the cleanup logic inclearThreadDraft. [ Out of scope ]