feat: JetBrains IDE diff approval and selection indicator - #926
feat: JetBrains IDE diff approval and selection indicator#926mbezhanov wants to merge 3 commits into
Conversation
Integrate the IDE diff viewer for write/edit approvals, surface the current IDE selection as a live chip in the prompt and auto-attach files from that selection. Co-authored-by: Marin Bezhanov <marin@cast.ai> Co-authored-by: Miroslav Uzunov <miroslav@cast.ai>
|
Documentation Changes Added
Actions
If neither actions are selected, on PR close/merge the docs branch in ReadMe will remain open. |
Kimchi Code Review
Summary📊 Review Score: 78/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Substantial test coverage was added: edit-apply.test.ts exercises normalisation, sequential edits, no-op filtering, and large-content replacement; ide-adapter/index.test.ts covers tool_call approval, user rejection, edit overrides, abort-signal forwarding, malformed-response fail-closed behaviour, and selection_changed clearing; lockfile.test.ts verifies workspace-only matching. Tests rely on mocked MCP envelopes and fake timers. 📝 Found 5 issue(s). See inline comments for details. What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![kimchi-review[bot]](https://avatars.githubusercontent.com/in/3223788?s=60&v=4){kind=small}
There was a problem hiding this comment.
📊 Review Score: 82/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 3/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Good coverage added for the new behavior: edit-apply.test.ts exercises normalisation and sequential edit application; index.test.ts adds tests for the new tool_call approval hook (approve/reject, edit ops, new files, user hand-edits, malformed responses, no-IDE fallback) and updates at-mention path assertions.
🔒 Security concerns found: computeProposedChange in src/extensions/ide-adapter/index.ts resolves the tool input path with resolve(cwd, rawPath) and reads the file to compute a diff, but it does not validate that the resolved path stays within the workspace. A malicious or confused agent could pass a path like ../../../etc/passwd and the IDE diff viewer would be asked to approve operating on arbitrary filesystem paths. The underlying write/edit tools likely have the same reach, but surfacing it through the IDE approval flow makes the exposure more visible.
📝 Found 3 issue(s). See inline comments for details.
|
@getkimchi review |
|
🔄 Starting review on |
There was a problem hiding this comment.
📊 Review Score: 70/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 4/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Tests were added for edit operation normalization/application (edit-apply.test.ts), IDE approval hook behavior (index.test.ts), and symlink-aware lockfile matching (lockfile.test.ts). They cover the new write/edit approval flow and selection indicator plumbing, but do not exercise the cross-extension race with permissions or the selection_changed zero-range clear contract.
🔒 Security concerns found: The permissions extension skips its terminal approval for write/edit when isIdeConnected() is true, but the ide-adapter hook can return undefined (allow) if the connection drops or the proposal cannot be computed, leaving those tools ungated. Additionally, the new WebSocket auth design uses a ?token= query parameter, which is more likely to be logged by proxies/servers than a header.
📝 Found 6 issue(s). See inline comments for details.
|
@getkimchi review |
|
🔄 Starting review on |
|
@getkimchi review |
|
🔄 Starting review on |
|
@getkimchi review |
|
🔄 Starting review on |
There was a problem hiding this comment.
📊 Review Score: 82/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 4/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — The PR adds a focused test suite for edit-apply.ts covering single/array forms, snake_case fallbacks, no-op filtering, first-occurrence replacement, and large-content handling. It also expands ide-adapter/index.test.ts significantly to exercise the new tool_call approval hook: approval/rejection paths, malformed responses, abort-signal forwarding, unreadable files, user-edited content overrides for both write and edit, and selection_changed indicator clearing. Existing at-mention and lockfile tests are updated to match the new absolute-path behavior and realpath-based workspace matching.
🔒 Security concerns found: The IDE auth token is now transmitted as a ?token= query parameter on the WebSocket URL rather than an x-secret-key header. Query-string credentials are more likely to be logged by reverse proxies, IDE server access logs, and terminal/browser histories, increasing the risk of token leakage. If the transport implementation was changed to match the updated contract, consider using a WebSocket subprotocol header or a short-lived handshake token exchanged after the socket is established to keep long-lived credentials out of URLs.
📝 Found 4 issue(s). See inline comments for details.
| return undefined | ||
| } | ||
| if (!approval.approved) { | ||
| return { |
There was a problem hiding this comment.
computeProposedChange is called with the safe fallback object input = event.input ?? {}, but applyEditedContent is passed event.input directly. If the original event.input is undefined, the cast event.input as Record<string, unknown> will be undefined and mutating event.input.content or event.input.edits will throw a TypeError, crashing the tool_call handler before it can return a graceful block.
💡 Suggestion: Pass the non-null input variable to applyEditedContent instead of event.input, and assign the mutated object back to event.input if needed: applyEditedContent(input, toolName, approval.newContent, proposed.originalContent); event.input = input
There was a problem hiding this comment.
📊 Review Score: 78/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 4/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Substantial test coverage was added: edit-apply.test.ts exercises normalisation, sequential edits, no-op filtering, and large-content replacement; ide-adapter/index.test.ts covers tool_call approval, user rejection, edit overrides, abort-signal forwarding, malformed-response fail-closed behaviour, and selection_changed clearing; lockfile.test.ts verifies workspace-only matching. Tests rely on mocked MCP envelopes and fake timers.
📝 Found 5 issue(s). See inline comments for details.
| @@ -101,7 +237,6 @@ export default function ideAdapterExtension(pi: ExtensionAPI): void { | |||
| if (isShuttingDown) break | |||
There was a problem hiding this comment.
The onDisconnect callback closes over the module-level connection variable rather than the specific connection instance. If an older WebSocket closes after a new connection has been assigned (e.g. during reconnect or a stale session), the callback will overwrite the live handle and set ideConnectionActive to false.
💡 Suggestion: Capture the connection reference in a local const conn = connection inside discoverAndConnect() and only clear the module state if connection === conn when the callback fires.
| return readFileSync(filePath, "utf-8") | ||
| } catch { | ||
| return "" | ||
| } |
There was a problem hiding this comment.
readCurrentContent calls readFileSync synchronously inside an async tool_call event handler. For large files this blocks the Node event loop while the file is read, causing UI/input latency and stalling other concurrent work.
💡 Suggestion: Use the async fs/promises readFile and make readCurrentContent/computeProposedChange async, or at least limit the read to a reasonable size and stream/binary-safe path before blocking the event loop.
| /** Tool names that mutate files and must be gated by IDE approval when enabled. */ | ||
| const APPROVAL_GATED_TOOLS = new Set(["write", "edit"]) | ||
|
|
||
| /** Short unique id for IDE tool-window queue tracking. */ |
There was a problem hiding this comment.
readCurrentContent uses synchronous readFileSync inside the async tool_call hook to read the entire file before computing a diff proposal. For large files this blocks the Node event loop and freezes the TUI until the read completes.
💡 Suggestion: Switch to asynchronous fs.readFile (or fs/promises.readFile) and propagate the resulting Promise<string | null> through computeProposedChange and requestIdeApproval.
|
|
||
| /** Override the agent's write/edit input with the user's hand-edited content | ||
| * from the IDE diff viewer. `write` sets `input.content`; `edit` rewrites | ||
| * `input.edits` as a single full-file replacement (robust vs. fragment-level |
There was a problem hiding this comment.
requestIdeApproval catches all errors with a bare catch { return null }, discarding the underlying failure reason. The caller logs only a generic warning, so network errors, malformed envelopes, or SDK issues become hard to diagnose in production.
💡 Suggestion: Capture the error in the catch clause and include it in the returned result or log, e.g. catch (err) { console.warn("[ide-adapter] proposeChange request failed:", err); return null } so operators can distinguish aborts from transport failures.
| } | ||
|
|
||
| /** Override the agent's write/edit input with the user's hand-edited content | ||
| * from the IDE diff viewer. `write` sets `input.content`; `edit` rewrites |
There was a problem hiding this comment.
ℹ️🔧 Maintainability
In applyEditedContent, when normalising a write call that used file_path to path, the original file_path key is left on the input object alongside path. This can create ambiguous input shapes for downstream tools.
💡 Suggestion: After copying file_path into path, delete input.file_path (and any snake_case variant) so the tool receives a single canonical path field.
2 participants
What does this PR do?
Add functionality that allows:
Checklist
pnpm run test)pnpm run check)