fix(cursor): re-arm finalize after completion-only tool frames - #1341
Conversation
📝 WalkthroughWalkthroughThe Cursor live transport now handles client-tool completion frames before event mapping. It re-arms turn finalization when open tools drain. Tests cover sibling completions and call-ID-only completions. ChangesCursor tool finalization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cursor-tool-finalize-race.test.ts`:
- Around line 204-219: Update the test “completion-only sibling re-arms finalize
after draining the call set” to delay completedFrame("call_b", "echo_b") until
partway through the initial grace window, then wait past the original deadline
but before the re-armed deadline and assert that no done event or cancellation
occurred. Finally, wait through the new deadline and retain the existing
assertions for one done event, two tool_call_end events, and NGHTTP2_CANCEL.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1cfbe4a9-1694-4c8d-b4db-e6b3eae9dd21
📒 Files selected for processing (2)
src/adapters/cursor/live-transport.tstests/cursor-tool-finalize-race.test.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/cursor-tool-finalize-race.test.ts (1)
204-226: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIncrease the margin around the original deadline.
At Line [218], the test checks only about 20 ms after the original 200 ms deadline. If the event loop delays the stale timer callback by more than 20 ms, this assertion can pass even when the old timer was not revoked.
Use deterministic timer control, or widen the intervals so the assertion is well after the original deadline and well before the re-armed deadline.
Example with wider timing margins
- const h = makeHarness(200, ["echo_a", "echo_b"]); + const h = makeHarness(300, ["echo_a", "echo_b"]); ... - await sleep(60); + await sleep(100); ... - await sleep(160); + await sleep(250); ... - await sleep(80); + await sleep(100);🤖 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/cursor-tool-finalize-race.test.ts` around lines 204 - 226, Increase the timing margin in the test “completion-only sibling re-arms finalize after draining the call set” so the intermediate assertion occurs clearly after the original 200 ms deadline but before the re-armed finalize deadline. Adjust the sleep intervals or use deterministic timer control, while preserving the final single-done and cancellation assertions.
🤖 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.
Outside diff comments:
In `@tests/cursor-tool-finalize-race.test.ts`:
- Around line 204-226: Increase the timing margin in the test “completion-only
sibling re-arms finalize after draining the call set” so the intermediate
assertion occurs clearly after the original 200 ms deadline but before the
re-armed finalize deadline. Adjust the sleep intervals or use deterministic
timer control, while preserving the final single-done and cancellation
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 17f30eea-f928-444f-9b2c-3b3d0de9589b
📒 Files selected for processing (1)
tests/cursor-tool-finalize-race.test.ts
f5d4a51 to
9ce0af6
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Please rebase this PR onto the latest |
d3297bf to
fe41ee8
Compare
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
Rate Limit Exceeded
|
|
Thanks @luvs01 — nice focused fix for the Cursor completion/finalize race, especially the call-id-only completion handling and the deterministic re-arm regression coverage. Much appreciated 🙏 |
2 participants
Summary
interactionUpdatecompletion drains the open tool-call set, preventing a completed tool turn from waiting indefinitely fordone.toolCallCompletedframes that carry onlycallIdby capturing open-call ownership before the protobuf mapper removes the call.Verification
bun run typecheck— passed on Bun 1.3.14 and Bun 1.4.0-canary.1.bun run privacy:scan— passed.git diff --check HEAD^ HEAD— passed.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit