Repository navigation
fix: validate marks immediately after creation in visual selection - #137
funkybooboo wants to merge 4 commits into
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 8. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Moves mark validation from async on_complete callback to immediately after mark creation. This prevents crashes when marks are used for logging or passed to RequestStatus before validation occurs. Fails fast with clear error message if buffer has been modified and marks cannot be created, rather than crashing later with cryptic errors.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds defensive mark creation for visual selections to avoid runtime errors when the buffer/selection changes mid-operation.
Changes:
- Wrap mark creation in
pcallto handle exceptions from mark APIs - Validate created marks before storing them in
context.marks - Log and exit early when marks can’t be created/validated
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local ok_top, top_mark = pcall(Mark.mark_above_range, range) | ||
| local ok_bottom, bottom_mark = pcall(Mark.mark_point, range.buffer, range.end_) | ||
|
|
||
| if not ok_top or not ok_bottom or not top_mark:is_valid() or not bottom_mark:is_valid() then |
|
|
||
| if not ok_top or not ok_bottom or not top_mark:is_valid() or not bottom_mark:is_valid() then | ||
| logger:fatal( | ||
| "Cannot create marks for visual selection. Buffer may have been modified." |
| local ok_top, top_mark = pcall(Mark.mark_above_range, range) | ||
| local ok_bottom, bottom_mark = pcall(Mark.mark_point, range.buffer, range.end_) | ||
|
|
||
| if not ok_top or not ok_bottom or not top_mark:is_valid() or not bottom_mark:is_valid() then | ||
| logger:fatal( | ||
| "Cannot create marks for visual selection. Buffer may have been modified." | ||
| ) | ||
| return |
There was a problem hiding this comment.
Bug: If one of two extmark creation calls fails, the successfully created mark is not cleaned up, leading to a resource leak.
Severity: MEDIUM
Suggested Fix
Before returning early on a partial failure, check if top_mark or bottom_mark were successfully created and, if so, call their respective delete() methods to ensure no extmarks are leaked.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: lua/99/ops/over-range.lua#L22-L29
Potential issue: In `over-range.lua`, two extmarks, `top_mark` and `bottom_mark`, are
created in separate `pcall` blocks. If one of these calls succeeds but the other fails,
a subsequent validation check will trigger an early return. However, the successfully
created extmark is not cleaned up during this early return because it is only added to
the tracked `context.marks` list after the validation has already passed. This results
in an orphaned extmark that persists in the buffer, causing a resource leak that can
accumulate over time and may lead to performance degradation.
Did we get this right? 👍 / 👎 to inform future reviews.
| return { | ||
| "pi", | ||
| "--print", | ||
| "--provider", | ||
| "opencode-go", | ||
| "--model", | ||
| context.model or "qwen3.7-plus", | ||
| "--no-context-files", | ||
| query, | ||
| } | ||
| end |
There was a problem hiding this comment.
Bug: The new PiProvider._build_command function constructs a command for pi but doesn't specify where to write the output, leading to empty responses and silent failures.
Severity: MEDIUM
Suggested Fix
Modify PiProvider._build_command to correctly instruct the pi command where to write its output. This likely involves adding an argument to the command string that points to context.tmp_file, similar to how other providers are implemented. This will ensure that _retrieve_response can successfully read the AI's output.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: lua/99/providers.lua#L316-L327
Potential issue: The newly introduced `PiProvider` has an incomplete implementation in
its `_build_command` method. This function assembles the command to be executed but
fails to include an argument specifying the output file path. Consequently, when
`BaseProvider:make_request` calls `_retrieve_response`, it attempts to read from
`context.tmp_file`, which remains empty because the `pi` command never wrote its output
to it. This results in silent failures, such as a "response was empty" message, for any
operation that relies on the AI's response being written to a temporary file, like
visual selection or search.
Did we get this right? 👍 / 👎 to inform future reviews.
Fixes crashes when marks are used before validation during visual selection operations.
Problem
The current code creates marks and immediately uses them for logging and RequestStatus before validating they were created successfully. When the buffer is modified between range calculation and mark creation, the marks fail to create but are still used, causing crashes.
Current flow:
Solution
Validate marks immediately after creation, before using them. This fails fast with a clear error message instead of crashing later with cryptic errors.
New flow:
Benefits
Testing