Add multi-IDE installer support for OpenCode, Cursor, Codex, Roo, and Kilo#291
Open
RaviTharuma wants to merge 4 commits into
Open
Add multi-IDE installer support for OpenCode, Cursor, Codex, Roo, and Kilo#291RaviTharuma wants to merge 4 commits into
RaviTharuma wants to merge 4 commits into
Conversation
|
@RaviTharuma is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughAdds an ChangesAgentMemory Installation Framework
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 4
🤖 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/hooks/codex.ts`:
- Around line 38-95: After determining the event (variable event), capture a
single timestamp once (e.g., const timestamp = new Date().toISOString()) and
reuse that variable in all fetch bodies instead of calling new
Date().toISOString() multiple times; update the payloads in the UserPromptSubmit
and PostToolUse/PreToolUse fetch calls to use this shared timestamp variable
(keep sid, root and hookType fields unchanged) so the same timestamp is sent for
all observations within the same execution.
In `@src/hooks/cursor.ts`:
- Around line 98-160: The code calls new Date().toISOString() multiple times
inside the switch handling inferred events; capture a single timestamp string
once before calling inferEvent/switch (e.g., const ts = new
Date().toISOString()) and replace each inline new Date().toISOString() in the
postObserve/postSessionStart payloads with that ts; update places using
postObserve (and postSessionStart if it needs timestamp) referenced in this
file—look for inferEvent(payload), postSessionStart({ sessionId: sid, project:
root, cwd: root }), and each postObserve(...) case to reuse the captured
timestamp variable.
In `@src/installers.ts`:
- Around line 94-121: The plugin currently generates a new session_id for every
event which prevents correlating events; inside generateOpenCodePlugin
(AgentmemoryPlugin) capture and store a single session_id when handling the
'session.created' event (e.g., assign to a closure-scoped variable) and then
reuse that stored session_id in all other event handlers and hooks
('session.idle', 'file.edited', 'command.executed', 'tool.execute.after',
'chat.message') instead of calling Date.now().toString(36) each time; ensure you
fall back to a sensible default if the stored session_id is not yet set.
- Around line 43-45: The packageRootFromCliDist function currently uses new
URL(import.meta.url).pathname which yields Windows paths with a leading slash;
change it to convert import.meta.url via fileURLToPath (from 'url') and use that
value with dirname and resolve so filesystem paths are correct
cross-platform—update packageRootFromCliDist to call
fileURLToPath(import.meta.url) instead of accessing .pathname and ensure the
function still uses dirname and resolve to return the package root.
🪄 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: 39354f66-c30b-44f8-90e6-cd2820ff76c4
📒 Files selected for processing (8)
plugin/scripts/codex.mjsplugin/scripts/cursor.mjssrc/cli.tssrc/hooks/codex.tssrc/hooks/cursor.tssrc/installers.tstest/installers.test.tstsdown.config.ts
Author
|
Addressed the actionable review feedback in commit 2e7e169:\n\n- reused a single timestamp in the Codex and Cursor hook handlers\n- switched installer path resolution to fileURLToPath for cross-platform correctness\n- fixed the generated OpenCode plugin to keep one stable session id across events\n\nRe-ran verification after the fixes:\n- npm run build\n- npx vitest run test/installers.test.ts\n- manual installer QA for OpenCode, Cursor, Codex, Roo, and Kilo\n\nI also checked the PR review threads — the actionable CodeRabbit threads are already marked resolved on GitHub after the update. The remaining Vercel failure is an authorization gate on the upstream team side rather than a code issue. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/installers.test.ts (2)
86-111: ⚡ Quick winUse imported
writeFileSyncandmkdirSyncfor consistency.Lines 88-98 use
require("node:fs")to access filesystem functions that are already imported at line 2. For consistency with the rest of the test file, use the imported functions.♻️ Proposed consistency fix
it("reuses native .jsonc paths when they already exist", () => { const root = tempRoot(); - require("node:fs").writeFileSync( + writeFileSync( join(root, "opencode.jsonc"), '{\n // comment\n "mcp": {"existing": {"type": "local", "command": ["foo"]}}\n}\n', "utf8", ); - require("node:fs").mkdirSync(join(root, ".kilo"), { recursive: true }); - require("node:fs").writeFileSync( + mkdirSync(join(root, ".kilo"), { recursive: true }); + writeFileSync( join(root, "kilo.jsonc"), '{\n // comment\n "mcp": {"existing": {"type": "local", "command": ["foo"]}}\n}\n', "utf8", );🤖 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/installers.test.ts` around lines 86 - 111, Replace the inline requires for node:fs with the already-imported filesystem helpers to keep the test consistent: change the three occurrences of require("node:fs").writeFileSync and require("node:fs").mkdirSync in the "reuses native .jsonc paths when they already exist" test to use the imported writeFileSync and mkdirSync functions (the rest of the test uses readFileSync/join), ensuring you reference the same symbols used elsewhere in the file.
58-84: ⚡ Quick winUse imported
mkdirSyncandwriteFileSyncfor consistency.Lines 62-64 and 69-72 use
require("node:fs")to accessmkdirSyncandwriteFileSync, but these functions are already imported at line 2. Using the imported functions improves consistency and avoids mixing import styles.♻️ Proposed consistency fix
+import { existsSync, mkdirSync, writeFileSync } from "node:fs"; + it("preserves JSONC-based existing server entries when merging", () => { const root = tempRoot(); const cursorDir = join(root, ".cursor"); const kiloDir = join(root, ".kilo"); - require("node:fs").mkdirSync(cursorDir, { recursive: true }); - require("node:fs").mkdirSync(kiloDir, { recursive: true }); - require("node:fs").writeFileSync( + mkdirSync(cursorDir, { recursive: true }); + mkdirSync(kiloDir, { recursive: true }); + writeFileSync( join(cursorDir, "mcp.json"), '{\n // comment\n "mcpServers": {\n "existing": {"command": "foo"}\n }\n}\n', "utf8", ); - require("node:fs").writeFileSync( + writeFileSync( join(kiloDir, "kilo.jsonc"), '{\n // comment\n "mcp": {\n "existing": {"type": "local", "command": ["foo"]}\n }\n}\n', "utf8", );🤖 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/installers.test.ts` around lines 58 - 84, Replace the ad-hoc require("node:fs") calls with the already imported fs helpers: use the imported mkdirSync and writeFileSync instead of require("node:fs").mkdirSync and require("node:fs").writeFileSync in the "preserves JSONC-based existing server entries when merging" test (the block that creates cursorDir/kiloDir and writes mcp.json and kilo.jsonc); this keeps import style consistent and reuses the top-level imports for mkdirSync and writeFileSync.
🤖 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/installers.ts`:
- Around line 31-68: The stripJsonComments function reads input[i + 1] into next
without bounds checks; update stripJsonComments to check i + 1 < input.length
before using next (and treat end-of-input as non-comment), and when scanning
single-line "//" or block "/*" comments ensure you don't read past the end:
verify i + 1 exists before entering those comment branches and handle an
unclosed block comment by advancing to input.length and breaking/returning
appropriate output. Specifically, guard uses of input[i + 1] (the variable next)
and adjust the while loops that skip to newline or "*/" to safely stop at
input.length to avoid accessing undefined or looping past the buffer.
- Around line 124-157: In generateOpenCodePlugin move the variable declarations
out of the returned object: declare let sessionId and const getSessionId before
the return string is constructed (so the generated plugin code places them as
top-level variables/functions inside the plugin factory), then keep the event,
'tool.execute.after', and 'chat.message' properties unchanged but referencing
sessionId/getSessionId; update any string interpolation that embeds these
symbols (sessionId, getSessionId) so they refer to the moved declarations rather
than being placed as inline variable declarations inside the object literal.
---
Nitpick comments:
In `@test/installers.test.ts`:
- Around line 86-111: Replace the inline requires for node:fs with the
already-imported filesystem helpers to keep the test consistent: change the
three occurrences of require("node:fs").writeFileSync and
require("node:fs").mkdirSync in the "reuses native .jsonc paths when they
already exist" test to use the imported writeFileSync and mkdirSync functions
(the rest of the test uses readFileSync/join), ensuring you reference the same
symbols used elsewhere in the file.
- Around line 58-84: Replace the ad-hoc require("node:fs") calls with the
already imported fs helpers: use the imported mkdirSync and writeFileSync
instead of require("node:fs").mkdirSync and require("node:fs").writeFileSync in
the "preserves JSONC-based existing server entries when merging" test (the block
that creates cursorDir/kiloDir and writes mcp.json and kilo.jsonc); this keeps
import style consistent and reuses the top-level imports for mkdirSync and
writeFileSync.
🪄 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: f38d3487-2223-49dd-9d64-29b239cc9398
📒 Files selected for processing (2)
src/installers.tstest/installers.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/installers.test.ts (3)
78-79: 💤 Low valueOptional: Remove unnecessary type assertions.
Same issue as line 53—the
as Record<string, any>assertions are unnecessary and inconsistent with otherJSON.parse()calls in the test file.♻️ Simplify to match other parse calls
- const cursor = JSON.parse(readFileSync(join(cursorDir, "mcp.json"), "utf8")) as Record<string, any>; - const kilo = JSON.parse(readFileSync(join(kiloDir, "kilo.jsonc"), "utf8")) as Record<string, any>; + const cursor = JSON.parse(readFileSync(join(cursorDir, "mcp.json"), "utf8")); + const kilo = JSON.parse(readFileSync(join(kiloDir, "kilo.jsonc"), "utf8"));🤖 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/installers.test.ts` around lines 78 - 79, The two JSON.parse calls creating variables cursor and kilo include unnecessary type assertions ("as Record<string, any>"); remove those assertions so the lines simply parse the file contents (using JSON.parse(readFileSync(join(...), "utf8"))) to match the other parse calls in the test and maintain consistency for cursor and kilo variable initialization.
22-111: ⚡ Quick winConsider adding error case and idempotency tests.
The current test suite provides solid happy-path coverage. Consider adding tests for:
- Invalid target names
- Permission/filesystem errors
- Malformed existing JSON/JSONC files
- Idempotency (running
installTargettwice on the same root)These additions would increase confidence in error handling and ensure the installer gracefully handles edge cases.
🤖 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/installers.test.ts` around lines 22 - 111, Add tests to installers.test.ts to cover error and idempotency cases for installTarget: add specs that call installTarget with an invalid target name and assert it throws or returns an error, simulate filesystem/permission errors by creating read-only dirs or mocking writeFileSync and assert installTarget fails gracefully, write malformed JSON/JSONC files into tempRoot before calling installTarget and assert it either preserves the file or reports a clear parse error, and add an idempotency test that runs installTarget twice on the same projectRoot and asserts no duplicate entries and that subsequent runs are no-ops (check filesWritten and contents); reference the installTarget function and existing tests in installers.test.ts to place these new cases.
53-53: 💤 Low valueOptional: Remove unnecessary type assertion.
The
as Record<string, any>type assertion is unnecessary sinceJSON.parse()already returnsanyby default. This assertion appears inconsistently—otherJSON.parse()calls in the file (lines 27, 34, 41-42, 52) work without it.♻️ Simplify to match other parse calls
- const kilo = JSON.parse(readFileSync(join(root, ".kilo", "kilo.jsonc"), "utf8")) as Record<string, any>; + const kilo = JSON.parse(readFileSync(join(root, ".kilo", "kilo.jsonc"), "utf8"));🤖 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/installers.test.ts` at line 53, The local variable declaration for kilo uses an unnecessary type assertion; remove "as Record<string, any>" from the JSON.parse call so it matches the other parses in the file. Update the line that assigns to the variable kilo (the JSON.parse(readFileSync(...)) expression) to omit the explicit cast, leaving the plain JSON.parse(...) result assigned to kilo.
🤖 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.
Nitpick comments:
In `@test/installers.test.ts`:
- Around line 78-79: The two JSON.parse calls creating variables cursor and kilo
include unnecessary type assertions ("as Record<string, any>"); remove those
assertions so the lines simply parse the file contents (using
JSON.parse(readFileSync(join(...), "utf8"))) to match the other parse calls in
the test and maintain consistency for cursor and kilo variable initialization.
- Around line 22-111: Add tests to installers.test.ts to cover error and
idempotency cases for installTarget: add specs that call installTarget with an
invalid target name and assert it throws or returns an error, simulate
filesystem/permission errors by creating read-only dirs or mocking writeFileSync
and assert installTarget fails gracefully, write malformed JSON/JSONC files into
tempRoot before calling installTarget and assert it either preserves the file or
reports a clear parse error, and add an idempotency test that runs installTarget
twice on the same projectRoot and asserts no duplicate entries and that
subsequent runs are no-ops (check filesWritten and contents); reference the
installTarget function and existing tests in installers.test.ts to place these
new cases.
- Line 53: The local variable declaration for kilo uses an unnecessary type
assertion; remove "as Record<string, any>" from the JSON.parse call so it
matches the other parses in the file. Update the line that assigns to the
variable kilo (the JSON.parse(readFileSync(...)) expression) to omit the
explicit cast, leaving the plain JSON.parse(...) result assigned to kilo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d910350-79a4-4e59-9222-89ed46385cad
📒 Files selected for processing (2)
src/installers.tstest/installers.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/installers.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
agentmemory installCLI command for OpenCode, Cursor, Codex, Roo, Kilo, PI, Hermes, and OpenClaw targetsWhat changed
src/installers.tswith target-specific installerssrc/hooks/cursor.tsandsrc/hooks/codex.tsagentmemory installintosrc/cli.tstsdown.config.tsso the new hook scripts are built into bothdist/hooksand trackedplugin/scriptstest/installers.test.tsQA
npm run buildnpx vitest run test/installers.test.tsnode dist/cli.mjs install opencode --project-root <tmp>node dist/cli.mjs install cursor --project-root <tmp>node dist/cli.mjs install codex --project-root <tmp>node dist/cli.mjs install roo --project-root <tmp>node dist/cli.mjs install kilo --project-root <tmp>and inspection of generated files/configs
Notes
Summary by CodeRabbit
New Features
installCLI command to install Agent Memory integrations for OpenCode, Cursor, Codex, Roo, Kilo, PI, OpenClaw, and Hermes.Chores
Tests