Migrate to official @cursor/sdk and composite action#3
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
PunGrumpy
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR migrates cursor-action from downloading and invoking the Cursor CLI to using the official ChangesSDK Migration
Sequence Diagram(s)sequenceDiagram
participant Index as index.ts
participant RunAgent as runAgent()
participant AgentSDK as `@cursor/sdk` Agent
Index->>RunAgent: inputs (apiKey, prompt, model, cwd)
RunAgent->>AgentSDK: Agent.create(apiKey, { cwd, model })
RunAgent->>AgentSDK: agent.send(prompt)
loop stream text events
AgentSDK-->>RunAgent: run.stream() yields { text }
RunAgent->>RunAgent: accumulate stdout
end
alt success
AgentSDK-->>RunAgent: run.wait() -> run.result (optional)
RunAgent->>Index: return exitCode: 0, stdout
else error
AgentSDK--XRunAgent: throws/rejects
RunAgent->>Index: return exitCode: 1, stderr (+ diagnostics)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
action.yml (2)
56-71: ⚡ Quick winQuote
$GITHUB_ACTION_PATHto survive paths with spaces.Unquoted
$GITHUB_ACTION_PATHworks on hosted runners (whose paths have no spaces) but breaks on self-hosted runners where the runner workspace path may contain spaces or other word-splitting characters. Quoting both occurrences makes the action robust to runner layout.🛡️ Quote the expansions
- run: npm ci --omit=dev --prefix $GITHUB_ACTION_PATH + run: npm ci --omit=dev --prefix "$GITHUB_ACTION_PATH" shell: bash ... - run: node $GITHUB_ACTION_PATH/dist/index.js + run: node "$GITHUB_ACTION_PATH/dist/index.mjs" shell: bash🤖 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 `@action.yml` around lines 56 - 71, The run commands use unquoted expansions of $GITHUB_ACTION_PATH which will break if the path contains spaces; update both occurrences (the npm run and the "Run Cursor Agent" step that calls node $GITHUB_ACTION_PATH/dist/index.js) to quote the variable expansions (e.g., "$GITHUB_ACTION_PATH" and "node \"$GITHUB_ACTION_PATH\"/dist/index.js" or equivalently wrap the entire run string in double quotes) so shell word-splitting is prevented and the action works on self-hosted runners with spaces in paths.
10-13: 💤 Low valueConsider also marking
cursor-versionas deprecated in metadata, not just description.The description now says "(Deprecated)…" but the input is still wired to
INPUT_CURSOR_VERSION(line 64) and defaults to"latest". GitHub Actions has no first-class deprecation field for inputs, but a stronger signal is to (a) emit acore.warningfromsrc/input.tswhenever it's set to anything other than empty/"latest"(the PR summary suggests this already happens — good) and (b) plan to remove the input in a next major. Optional now; flagging for the deprecation timeline.🤖 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 `@action.yml` around lines 10 - 13, The cursor-version input should be treated as deprecated at runtime and documented for removal: in src/input.ts detect the environment/input named INPUT_CURSOR_VERSION and if its value is non-empty and not "latest" call core.warning with a clear deprecation message (mentioning it will be removed in the next major) so users get a runtime warning; also add a short deprecation note alongside the existing description in action.yml (and/or README) so the metadata surfaces the deprecation and your planned removal timeline.src/runner.ts (1)
41-43: 💤 Low value
error.causeinterpolation may stringify to[object Object].Template-literal interpolation calls
toString(); for plain object causes this yields[object Object], hiding root-cause info that's the whole point of attachingcause. Either format withutil.inspectorJSON.stringify(with a safe replacer), or recurse ifcauseis itself anError.♻️ Suggested formatting
- if (error.cause) { - stderr += `\nCause: ${error.cause}`; - } + if (error.cause !== undefined) { + const cause = + error.cause instanceof Error + ? error.cause.stack ?? error.cause.message + : typeof error.cause === "string" + ? error.cause + : JSON.stringify(error.cause); + stderr += `\nCause: ${cause}`; + }🤖 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/runner.ts` around lines 41 - 43, The current stderr append uses template interpolation of error.cause which can stringify objects as "[object Object]"; update the logic around error.cause (the check that appends to stderr) to detect if error.cause is an Error and recursively extract its message/stack, otherwise format non-error objects with util.inspect (or JSON.stringify with a safe replacer) before appending to stderr so the real cause details are preserved; ensure you import util.inspect if used and keep the existing stderr variable and error.cause check but replace the simple `${error.cause}` interpolation with the improved formatting routine.__tests__/runner.test.ts (1)
86-92: 💤 Low valueDead
get result()rejection in the stream-error test.When
run.stream()throws (after yielding"Starting..."), thefor awaitloop inrunAgentpropagates synchronously into thecatchblock, soawait run.resultis never reached and the getter never runs. Either drop theget result()mock here (the stream throw alone proves the assertion) or, conversely, test only theresult-rejection path by usingmockStreamSuccessforstream. As written it pretends to exercise two failure modes but only the stream throw is observable.♻️ Suggested simplification
it("returns exitCode 1 when stream throws an error", async () => { mockAgentSend.mockResolvedValue({ - get result() { - return Promise.reject(new Error("Stream aborted")); - }, stream: mockStreamError, });🤖 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 `@__tests__/runner.test.ts` around lines 86 - 92, The test currently mocks both a throwing stream and a rejected getter (mockAgentSend returning get result that rejects) but only the stream throw is observed; remove the dead get result() mock and only assert the stream-error path by returning { stream: mockStreamError } from mockAgentSend, or if you intended to test the result-rejection path instead, swap to { stream: mockStreamSuccess, get result() { return Promise.reject(new Error("...")) } } so the test exercises exactly one failure mode (references: mockAgentSend, mockStreamError, mockStreamSuccess, and the runAgent/run.result behavior).
🤖 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 `@__tests__/runner.test.ts`:
- Around line 62-66: The test currently only verifies apiKey/local/model passed
to Agent.create via mockAgentCreate, which lets permissions and timeout from
baseInputs be dropped; update the expectation so mockAgentCreate is asserted to
have been called with permissions: "read-only" and timeout: 300 in the same call
object (i.e., include permissions and timeout alongside apiKey, local, model) so
the test will fail if src/runner.ts (the code that calls Agent.create) does not
forward baseInputs.permissions and baseInputs.timeout.
In `@action.yml`:
- Around line 55-57: The "Install Action Dependencies" step currently runs `npm
ci --omit=dev --prefix $GITHUB_ACTION_PATH` on every invocation, causing
cold-install latency and registry dependency; replace this by bundling
dependencies into the action (option B): update your build to produce a single
bundled output (e.g., dist/index.mjs) that includes `@cursor/sdk` via tsdown
rather than externalizing it, remove the `npm ci` step from action.yml, and
adjust tsdown.config.ts to include/bundle `@cursor/sdk` so consumers do not need
runtime installs.
In `@src/runner.ts`:
- Around line 33-36: The code awaits a plain string property run.result; remove
the unnecessary await and read the property directly (const finalResult =
run.result) or, if you need to guarantee completion, await run.wait() first and
then read run.result (e.g., await run.wait(); const finalResult = run.result)
before assigning to stdout. Update the use of finalResult/stdout in the function
that currently does await run.result so it reflects this direct-access or
wait-then-access approach.
- Around line 19-25: The PR currently drops "permissions" and "timeout" inputs;
update runner.ts so permissions are not silently advertised as effective (remove
logged usage or replace with a clear warning that permissions are auth-scoped
and not passed to Agent.create) and implement timeout behavior by using the Run
object returned from agent.send: after creating the agent via Agent.create(...)
and calling const run = await agent.send(inputs.prompt), if inputs.timeout is
set start a timer that checks run.supports('cancel') and calls run.cancel() when
the timer elapses (also clear the timer when the run finishes), ensuring no
"timeout" option is passed into Agent.create and no unsupported "permissions"
option is forwarded.
In `@tsdown.config.ts`:
- Line 8: The build output extension in tsdown.config.ts currently sets
outExtensions via outExtensions: () => ({ js: ".js" }) which yields
dist/index.js but the runtime/composite action expects dist/index.mjs; change
the outExtensions function to emit .mjs (e.g., return { js: ".mjs" }) so the
generated artifact name matches the runtime entrypoint (update any references to
outExtensions if present and ensure tooling that uses outExtensions respects
.mjs).
---
Nitpick comments:
In `@__tests__/runner.test.ts`:
- Around line 86-92: The test currently mocks both a throwing stream and a
rejected getter (mockAgentSend returning get result that rejects) but only the
stream throw is observed; remove the dead get result() mock and only assert the
stream-error path by returning { stream: mockStreamError } from mockAgentSend,
or if you intended to test the result-rejection path instead, swap to { stream:
mockStreamSuccess, get result() { return Promise.reject(new Error("...")) } } so
the test exercises exactly one failure mode (references: mockAgentSend,
mockStreamError, mockStreamSuccess, and the runAgent/run.result behavior).
In `@action.yml`:
- Around line 56-71: The run commands use unquoted expansions of
$GITHUB_ACTION_PATH which will break if the path contains spaces; update both
occurrences (the npm run and the "Run Cursor Agent" step that calls node
$GITHUB_ACTION_PATH/dist/index.js) to quote the variable expansions (e.g.,
"$GITHUB_ACTION_PATH" and "node \"$GITHUB_ACTION_PATH\"/dist/index.js" or
equivalently wrap the entire run string in double quotes) so shell
word-splitting is prevented and the action works on self-hosted runners with
spaces in paths.
- Around line 10-13: The cursor-version input should be treated as deprecated at
runtime and documented for removal: in src/input.ts detect the environment/input
named INPUT_CURSOR_VERSION and if its value is non-empty and not "latest" call
core.warning with a clear deprecation message (mentioning it will be removed in
the next major) so users get a runtime warning; also add a short deprecation
note alongside the existing description in action.yml (and/or README) so the
metadata surfaces the deprecation and your planned removal timeline.
In `@src/runner.ts`:
- Around line 41-43: The current stderr append uses template interpolation of
error.cause which can stringify objects as "[object Object]"; update the logic
around error.cause (the check that appends to stderr) to detect if error.cause
is an Error and recursively extract its message/stack, otherwise format
non-error objects with util.inspect (or JSON.stringify with a safe replacer)
before appending to stderr so the real cause details are preserved; ensure you
import util.inspect if used and keep the existing stderr variable and
error.cause check but replace the simple `${error.cause}` interpolation with the
improved formatting routine.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca8d6881-9618-49f3-814a-e4d84c12ee41
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.changeset/migrate-sdk.md__tests__/cursor-version.test.ts__tests__/input.test.ts__tests__/installer.test.ts__tests__/output.test.ts__tests__/runner.test.tsaction.ymlpackage.jsonsrc/cursor-version.tssrc/index.tssrc/input.tssrc/installer.tssrc/output.tssrc/runner.tssrc/types.tstsconfig.jsontsdown.config.ts
💤 Files with no reviewable changes (4)
- tests/cursor-version.test.ts
- tests/installer.test.ts
- src/cursor-version.ts
- src/installer.ts
📜 Review details
🔇 Additional comments (9)
tsconfig.json (1)
13-13: Looks good for Bun-based test typing.No blockers in this change segment.
.changeset/migrate-sdk.md (1)
1-5: Changeset entry is clear and appropriately marked major.src/index.ts (1)
16-16: Signature update is consistent with the new output flow.src/input.ts (1)
8-10: Deprecation handling is clean and consistent.Trim + optional omission + targeted warning behavior looks correct.
Also applies to: 20-25, 54-54
__tests__/output.test.ts (1)
39-39: Test updates correctly track the newsetOutputs(result)signature.Also applies to: 57-57, 67-67, 77-77, 87-87, 97-97, 103-103, 113-113, 123-123
__tests__/input.test.ts (1)
46-53: New cursor-version tests are well aligned with the updated input contract.Also applies to: 55-62, 64-71
src/types.ts (1)
3-23: LGTM — type shape aligns with new SDK-based runner and output contract.
cursorVersionbecoming optional, droppingcacheHitfromActionOutputs, and narrowingAgentResultto{ stdout, stderr, exitCode, diagnostics? }are coherent with the runner/output/action.yml updates in this PR.src/output.ts (1)
5-100: LGTM — cleanly removes CLI-specific fields and dedups diagnostics against stderr.The
parseSummaryJSON-then-raw-text flow, the simplified job summary table, and thediag !== errTextguard all align with the narrowedAgentResultand the updatedsetOutputs(result)signature.action.yml (1)
61-61: ⚡ Quick winThe entry filename in action.yml is correct and consistent with the build configuration.
The tsdown configuration explicitly sets
outExtensions: () => ({ js: ".js" })intsdown.config.ts, which means the bundler will emitdist/index.js. The reference todist/index.json line 61 of action.yml is correct and aligns with both the bundler output and the package.jsonmainfield. There is no filename mismatch issue.> Likely an incorrect or invalid review comment.
…runAgent function
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
action.yml (1)
57-57: ⚡ Quick winQuote the path variable to prevent word splitting.
The path
$GITHUB_ACTION_PATH/dist/index.mjsis unquoted, which could break if the path contains spaces. While rare in CI environments, it's a shell best practice to quote variable expansions.♻️ Proposed fix
- run: node $GITHUB_ACTION_PATH/dist/index.mjs + run: node "$GITHUB_ACTION_PATH/dist/index.mjs"🤖 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 `@action.yml` at line 57, The unquoted path in the action.yml run step can break if $GITHUB_ACTION_PATH contains spaces; update the run value that invokes node with the target script (the line currently using $GITHUB_ACTION_PATH/dist/index.mjs) so the expanded path is quoted (wrap the $GITHUB_ACTION_PATH/dist/index.mjs expansion in double quotes) to prevent word-splitting and ensure the node command always receives the correct single argument.
🤖 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 `@tsdown.config.ts`:
- Line 6: The bundling of `@cursor/sdk` in the alwaysBundle array causes an ES
module runtime error due to __filename references; update tsdown.config.ts to
stop bundling it by removing "@cursor/sdk" from the alwaysBundle list (or
alternatively mark "@cursor/sdk" as external in the bundler config so it isn't
inlined), ensuring the bundled action expects node_modules to provide that
dependency; locate the alwaysBundle setting in tsdown.config.ts and either
delete the "@cursor/sdk" entry or adjust the bundler/external settings to
prevent bundling that package.
---
Nitpick comments:
In `@action.yml`:
- Line 57: The unquoted path in the action.yml run step can break if
$GITHUB_ACTION_PATH contains spaces; update the run value that invokes node with
the target script (the line currently using $GITHUB_ACTION_PATH/dist/index.mjs)
so the expanded path is quoted (wrap the $GITHUB_ACTION_PATH/dist/index.mjs
expansion in double quotes) to prevent word-splitting and ensure the node
command always receives the correct single argument.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41ddefd7-ab9b-43a0-9e53-cbe2039a2178
📒 Files selected for processing (7)
README.md__tests__/runner.test.tsaction.ymlpackage.jsonsrc/input.tssrc/runner.tstsdown.config.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- src/input.ts
- src/runner.ts
- tests/runner.test.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
1 participant
Summary
Migrates the GitHub Action from a custom Node.js binary wrapper to using the official
@cursor/sdk, and updates the execution model fromnode24to acompositeaction. This ensures reliable, cross-platform resolution of native dependencies and significantly cleans up the codebase.Key Changes
@cursor/sdkto replace fragile binary download and execution logic.compositeto automatically runnpm cifor native dependency resolution.tsdownto output modern ESM (.mjs) with proper module externalization.cursor-versioninput as the SDK manages agent versioning internally.Architecture Overview
1. Concept / Strategy
The previous implementation manually downloaded the
cursor-agentbinary and spawned it as a child process. This PR transitions to the official@cursor/sdkNPM package, utilizing itsAgent.createAPI for a more robust, typed, and integrated execution flow.2. Implementation
src/installer.ts(custom caching and downloading logic).src/runner.tsto instantiate the SDK Agent and handle event streams.action.ymlto run a setup step (npm ci) prior to executing the bundleddist/index.mjsscript.3. Components / Systems
runner.ts)action.yml)tsdown.config.ts,package.json)4. Migration / Breaking Changes
composite(requires Node.js/npm on the runner).cursor-versionis deprecated.Summary by CodeRabbit
Breaking Changes
cache-hitoutput removed;cursor-versioninput deprecated/optional and may emit a warning. Newexit-codeoutput provided.New Features
Chores
Tests
Docs