Conversation
There was a problem hiding this comment.
Pull request overview
This PR integrates official Cursor tab completion hooks (beforeTabFileRead and afterTabFileEdit) to track AI-generated code from Cursor's Tab feature, replacing the previous experimental extension approach. This enables proper attribution of tab completions in Git AI's authorship tracking.
Key Changes:
- Added support for two new Cursor hook events for tab completion tracking
- Implemented edit application logic to reconstruct file state from Cursor's edit deltas
- Added comprehensive test coverage for various tab completion scenarios
- Updated VS Code extension to use consistent tool/model naming ("github-copilot"/"tab")
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/cursor.rs | Adds 8 new test cases covering tab completion workflows including single/multiple edits, inline completions, and multi-session scenarios |
| src/commands/install_hooks.rs | Registers beforeTabFileRead and afterTabFileEdit hooks in Cursor's configuration |
| src/commands/checkpoint_agent/agent_presets.rs | Implements handlers for new tab hooks including edit application logic and file content retrieval from working log |
| src/authorship/post_commit.rs | Filters AiTab checkpoints from prompt update logic since they lack conversation transcripts |
| agent-support/vscode/src/ai-tab-edit-manager.ts | Updates tool name from "github-copilot-tab" to "github-copilot" and model from "default" to "tab" for consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| Ok(lines.join("\n")) |
There was a problem hiding this comment.
The apply_edits_to_content function uses lines.join("\n") which will not preserve a trailing newline if the original content ended with one. For example, if old_content is "line1\nline2\n", after splitting with lines() and joining with "\n", it becomes "line1\nline2" (no trailing newline). This could cause issues when comparing file contents or applying diffs.
Consider checking if the original content ends with a newline and appending one after the join if needed:
let mut result = lines.join("\n");
if old_content.ends_with('\n') {
result.push('\n');
}
Ok(result)| Ok(lines.join("\n")) | |
| let mut result = lines.join("\n"); | |
| if old_content.ends_with('\n') { | |
| result.push('\n'); | |
| } | |
| Ok(result) |
| if start_line_idx == end_line_idx { | ||
| // Single-line edit | ||
| let line = &lines[start_line_idx]; | ||
| // Convert 1-indexed columns to 0-indexed | ||
| let start_col_idx = start_col.saturating_sub(1); | ||
| let end_col_idx = end_col.saturating_sub(1); | ||
|
|
||
| // Split the line and insert the new string | ||
| let before = if start_col_idx < line.len() { | ||
| &line[..start_col_idx] | ||
| } else { | ||
| line.as_str() | ||
| }; | ||
| let after = if end_col_idx < line.len() { | ||
| &line[end_col_idx..] | ||
| } else { | ||
| "" | ||
| }; | ||
|
|
||
| lines[start_line_idx] = format!("{}{}{}", before, new_string, after); |
There was a problem hiding this comment.
The single-line edit handling doesn't properly handle new_string values that contain newlines. When inserting a string like "for (let i = 0; i < 10; i++) {\n " (see test line 789), this code will embed the newline character into a single element of the lines vector (line 988), which will corrupt the line structure. When the lines are later joined with "\n", the embedded newlines will create additional unintended line breaks.
The code needs to properly split new_string on newlines and insert/update multiple line elements in the lines vector. For example, if new_string contains N newlines, it should result in N+1 lines being inserted/modified in the vector.
| // Apply each edit in order | ||
| for edit in edits_array { |
There was a problem hiding this comment.
The sequential application of edits is incorrect when multiple edits target the same line. Looking at the test on lines 786-811 in tests/cursor.rs, both edits target line 2 with columns 5 and 36 referring to positions in the ORIGINAL line. However, this code applies edits sequentially - after the first edit inserts newlines and modifies the line, the second edit's line/column positions no longer match the modified content, causing incorrect positioning.
Edits should either be applied in reverse order (from end to beginning) to avoid position corruption, or all edits targeting the same line should be collected and applied together using positions relative to the original line content.
| let old_content = Self::get_most_recent_file_content(&repo_working_dir, &file_path) | ||
| .map(|(content, _blob_sha)| content) | ||
| .unwrap_or_else(|| { | ||
| // If no checkpoint exists, try to read from filesystem as fallback | ||
| std::fs::read_to_string(&file_path).unwrap_or_default() | ||
| }); |
There was a problem hiding this comment.
The filesystem fallback reads the file after Cursor has already applied the edits, but then attempts to apply those same edits again via apply_edits_to_content. This will produce incorrect results since the edits are meant to be applied to the pre-edit content.
The fallback should not use std::fs::read_to_string(&file_path) since that reads the post-edit content. Instead, it should either:
- Return an error if no checkpoint exists (since we can't properly reconstruct the content)
- Or read the file from git's HEAD to get the pre-edit baseline
Note that the proper content should come from the beforeTabFileRead checkpoint that should have been created just before the edit.
| let old_content = Self::get_most_recent_file_content(&repo_working_dir, &file_path) | |
| .map(|(content, _blob_sha)| content) | |
| .unwrap_or_else(|| { | |
| // If no checkpoint exists, try to read from filesystem as fallback | |
| std::fs::read_to_string(&file_path).unwrap_or_default() | |
| }); | |
| let old_content = match Self::get_most_recent_file_content(&repo_working_dir, &file_path) { | |
| Some((content, _blob_sha)) => content, | |
| None => { | |
| // If no checkpoint exists, we cannot reconstruct the pre-edit content | |
| return Err(GitAiError::PresetError( | |
| format!("No checkpoint exists for file '{}', cannot reconstruct pre-edit content for afterTabFileEdit", file_path) | |
| )); | |
| } | |
| }; |
| let new_content = Self::apply_edits_to_content(&old_content, edits).unwrap_or_else(|e| { | ||
| eprintln!("[Warning] Failed to apply edits for afterTabFileEdit: {}", e); | ||
| old_content.clone() | ||
| }); |
There was a problem hiding this comment.
When edit application fails, silently falling back to old_content (the pre-edit state) will create an incorrect checkpoint that doesn't reflect the actual file state after the Tab completion. This could lead to attribution errors where AI edits aren't properly tracked.
Consider either:
- Propagating the error to fail the checkpoint creation
- Or reading the actual file content from disk as a fallback (though this has its own issues as noted in a separate comment)
The current approach of using pre-edit content when edits fail will result in the dirty_files snapshot being out of sync with the actual file state.
| let new_content = Self::apply_edits_to_content(&old_content, edits).unwrap_or_else(|e| { | |
| eprintln!("[Warning] Failed to apply edits for afterTabFileEdit: {}", e); | |
| old_content.clone() | |
| }); | |
| let new_content = Self::apply_edits_to_content(&old_content, edits) | |
| .map_err(|e| GitAiError::PresetError(format!("Failed to apply edits for afterTabFileEdit: {}", e)))?; |
b7ce6a7 to
164ff04
Compare
|
No AI authorship found for these commits. Please install git-ai to start tracking AI generated code in your commits. |
164ff04 to
784850f
Compare
784850f to
9a91dac
Compare
Uses official Cursor hooks for tracking tab completion. Replaces experimental Cursor extension that previously supported tracking tab completion for Git AI
DO NOT MERGE UNTIL FIXED.