Fix agentdiff sync and PR attribution reporting#7
Conversation
Made-with: Cursor
Greptile SummaryThis PR fixes three distinct issues: Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/robustness suggestions that do not affect the primary fix paths. The core fixes (new-side-only diff parsing, early-return push, idempotent PR comment) are correct and well-tested. Three P2 findings remain: silent git exit-code swallowing, potential over-attribution in extract_lines_from_changes when both top-level and nested range keys co-exist, and a slightly misleading success message when remote push is skipped. None of these block merge. src/commands/diff.rs (exit-code check) and scripts/capture-cursor.py (over-attribution guard) are worth a second look before the next release. Important Files Changed
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Push as agentdiff push
participant LocalRef as Local Git Ref
participant GitHub as GitHub API
Dev->>Push: agentdiff push
Push->>GitHub: fetch remote traces (API)
alt fetch succeeds
GitHub-->>Push: remote traces JSONL
else fetch fails
GitHub-->>Push: error (remote_read_failed=true)
end
Push->>Push: dedup local vs remote (new_count)
Push->>LocalRef: write merged traces.jsonl
alt new_count == 0
Push->>Push: prune_local_traces()
Push-->>Dev: already up to date
else new_count > 0
alt remote_read_failed
Push-->>Dev: wrote N trace(s) to local ref
else
Push->>GitHub: push_content_to_ref (best-effort)
alt push succeeds
GitHub-->>Push: ok
Push->>Push: prune_local_traces()
Push-->>Dev: pushed N trace(s)
else push fails
Push-->>Dev: warn + pushed N trace(s) [misleading]
end
end
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn run_git_diff(repo_root: &std::path::Path, commit: &str) -> Result<String> { | ||
| let spec = diff_spec(commit); | ||
| let output = std::process::Command::new("git") | ||
| .args(["diff", commit]) | ||
| .args(["diff", "--unified=0", "--no-color", "--no-ext-diff", &spec]) | ||
| .current_dir(repo_root) | ||
| .output()?; | ||
| Ok(String::from_utf8_lossy(&output.stdout).to_string()) | ||
| } |
There was a problem hiding this comment.
Command::output()? captures stdout but does not inspect output.status. If git diff HEAD^! exits non-zero (e.g. the initial commit has no parent, a typo in the ref, or a shallow clone), stdout is empty and parse_diff_hunks returns an empty map. The diff command then silently prints "0 lines changed, 0 AI-attributed" with no indication of the underlying error. The switch to {commit}^! makes this edge case more likely to surface (initial commits, shallow clones).
| fn run_git_diff(repo_root: &std::path::Path, commit: &str) -> Result<String> { | |
| let spec = diff_spec(commit); | |
| let output = std::process::Command::new("git") | |
| .args(["diff", commit]) | |
| .args(["diff", "--unified=0", "--no-color", "--no-ext-diff", &spec]) | |
| .current_dir(repo_root) | |
| .output()?; | |
| Ok(String::from_utf8_lossy(&output.stdout).to_string()) | |
| } | |
| fn run_git_diff(repo_root: &std::path::Path, commit: &str) -> Result<String> { | |
| let spec = diff_spec(commit); | |
| let output = std::process::Command::new("git") | |
| .args(["diff", "--unified=0", "--no-color", "--no-ext-diff", &spec]) | |
| .current_dir(repo_root) | |
| .output()?; | |
| if !output.status.success() { | |
| let stderr = String::from_utf8_lossy(&output.stderr); | |
| anyhow::bail!("git diff failed: {}", stderr.trim()); | |
| } | |
| Ok(String::from_utf8_lossy(&output.stdout).to_string()) | |
| } |
| def extract_lines_from_changes(changes): | ||
| out = set() | ||
| if not isinstance(changes, list): | ||
| return [] | ||
| for change in changes: | ||
| if not isinstance(change, dict): | ||
| continue | ||
| _add_lines_from_range_object(out, change) | ||
| for key in ("range", "newRange", "new_range", "selection", "targetSelection"): | ||
| _add_lines_from_range_object(out, change.get(key)) | ||
| return sorted(out) |
There was a problem hiding this comment.
range/selection
When a change dict carries both a top-level startLine/endLine and a nested range or selection key (a plausible Cursor payload, e.g. edit at line 10 inside a selected block spanning lines 5–20), _add_lines_from_range_object(out, change) adds only lines 10–10, but the subsequent for key loop then also calls _add_lines_from_range_object(out, change.get("range")), adding lines 5–20. The union over-attributes the surrounding selection as AI-authored. If a specific top-level range is already resolved, there is no reason to also expand from the nested range keys.
| def extract_lines_from_changes(changes): | |
| out = set() | |
| if not isinstance(changes, list): | |
| return [] | |
| for change in changes: | |
| if not isinstance(change, dict): | |
| continue | |
| _add_lines_from_range_object(out, change) | |
| for key in ("range", "newRange", "new_range", "selection", "targetSelection"): | |
| _add_lines_from_range_object(out, change.get(key)) | |
| return sorted(out) | |
| def extract_lines_from_changes(changes): | |
| out = set() | |
| if not isinstance(changes, list): | |
| return [] | |
| for change in changes: | |
| if not isinstance(change, dict): | |
| continue | |
| before = len(out) | |
| _add_lines_from_range_object(out, change) | |
| if len(out) == before: | |
| # No top-level line info; try nested range keys. | |
| for key in ("range", "newRange", "new_range", "selection", "targetSelection"): | |
| _add_lines_from_range_object(out, change.get(key)) | |
| return sorted(out) |
Made-with: Cursor
AgentDiff ReportSummary
Files Modified
|
1 participant
Summary
agentdiff diffso commit/range mode reports only changed new-side lines.Test plan
cargo testpython3 -m unittest scripts.tests.test_capture_cursorcargo buildtarget/debug/agentdiff -C /home/prakh/agentdiff diff HEAD --ai-onlyCloses #6