Skip to content

Commit 60dcf63

Browse files
committed
feat: track stale PR reviews by head sha
Persist each PR review's head SHA so stale-review detection only flips when the pull request actually moves, keeping same-head reruns from looking outdated while preserving immediate stale signals on new commits. Made-with: Cursor
1 parent def7adb commit 60dcf63

File tree

9 files changed

+172
-39
lines changed

9 files changed

+172
-39
lines changed

TODO.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ This roadmap is derived from deep research into Greptile's public docs, blog, MC
3838
11. [x] Track unresolved vs resolved findings for PR reviews as a first-class lifecycle state.
3939
12. [ ] Add review completeness metrics: total findings, acknowledged findings, fixed findings, stale findings.
4040
13. [x] Compute merge-readiness summaries for GitHub PR reviews using severity, unresolved count, and verification state.
41-
14. [ ] Add stale-review detection when new commits land after the latest completed review.
41+
14. [x] Add stale-review detection when new commits land after the latest completed review.
4242
15. [x] Show "needs re-review" state in review detail and history pages for incremental PR workflows.
4343
16. [ ] Distinguish informational findings from blocking findings in lifecycle and readiness calculations.
4444
17. [ ] Add "critical blockers" summary cards for unresolved `Error` and `Warning` comments.
@@ -157,4 +157,5 @@ This roadmap is derived from deep research into Greptile's public docs, blog, MC
157157
- [x] Surface repository rule sources and pattern repository sources in Settings.
158158
- [x] Ship first-pass finding lifecycle state and lightweight merge readiness through the backend, API, CLI summaries, and review UI.
159159
- [x] Make merge readiness verification-aware and surface stale PR reviews as needs re-review in history/detail views.
160+
- [x] Make stale-review detection compare PR head SHAs so same-head reruns do not look stale.
160161
- [ ] Commit and push each validated checkpoint before moving to the next epic.
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
ALTER TABLE reviews
2+
ADD COLUMN IF NOT EXISTS github_head_sha TEXT;

src/commands/feedback_eval/input.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ mod tests {
4747
id: "review-1".to_string(),
4848
status: ReviewStatus::Complete,
4949
diff_source: "raw".to_string(),
50+
github_head_sha: None,
5051
started_at: 1,
5152
completed_at: Some(2),
5253
comments,

src/core/comment/summary.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ pub(super) fn apply_review_runtime_state(
8888
reasons.push("verification was inconclusive or fail-open; rerun this review".to_string());
8989
}
9090
if stale_review {
91-
reasons.push("a newer review exists for this pull request".to_string());
91+
reasons.push("new commits landed after this review".to_string());
9292
}
9393
summary.readiness_reasons = reasons;
9494
summary.merge_readiness = if !summary.readiness_reasons.is_empty() {
@@ -296,7 +296,7 @@ mod tests {
296296
assert_eq!(summary.merge_readiness, MergeReadiness::NeedsReReview);
297297
assert_eq!(
298298
summary.readiness_reasons,
299-
vec!["a newer review exists for this pull request".to_string()]
299+
vec!["new commits landed after this review".to_string()]
300300
);
301301
}
302302
}

src/server/api.rs

Lines changed: 119 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,7 @@ pub async fn start_review(
380380
id: id.clone(),
381381
status: ReviewStatus::Pending,
382382
diff_source: display_source,
383+
github_head_sha: None,
383384
started_at: current_timestamp(),
384385
completed_at: None,
385386
comments: Vec::new(),
@@ -726,31 +727,47 @@ async fn load_review_inventory(state: &Arc<AppState>) -> Vec<ReviewSession> {
726727
sessions
727728
}
728729

729-
fn latest_review_started_by_source(
730+
#[derive(Debug, Clone)]
731+
struct LatestGitHubHead {
732+
started_at: i64,
733+
head_sha: String,
734+
}
735+
736+
fn latest_review_head_by_source(
730737
reviews: &[ReviewSession],
731-
) -> std::collections::HashMap<String, i64> {
732-
let mut latest = std::collections::HashMap::new();
738+
) -> std::collections::HashMap<String, LatestGitHubHead> {
739+
let mut latest: std::collections::HashMap<String, LatestGitHubHead> =
740+
std::collections::HashMap::new();
733741
for review in reviews {
742+
let Some(head_sha) = review.github_head_sha.as_ref() else {
743+
continue;
744+
};
734745
if !review.diff_source.starts_with("pr:") {
735746
continue;
736747
}
737-
latest
738-
.entry(review.diff_source.clone())
739-
.and_modify(|current: &mut i64| *current = (*current).max(review.started_at))
740-
.or_insert(review.started_at);
748+
let candidate = LatestGitHubHead {
749+
started_at: review.started_at,
750+
head_sha: head_sha.clone(),
751+
};
752+
match latest.get(&review.diff_source) {
753+
Some(current) if current.started_at >= review.started_at => {}
754+
_ => {
755+
latest.insert(review.diff_source.clone(), candidate);
756+
}
757+
}
741758
}
742759
latest
743760
}
744761

745762
fn apply_dynamic_review_state(
746763
mut session: ReviewSession,
747-
latest_by_source: &std::collections::HashMap<String, i64>,
764+
latest_by_source: &std::collections::HashMap<String, LatestGitHubHead>,
748765
) -> ReviewSession {
749-
let stale_review = session.diff_source.starts_with("pr:")
750-
&& latest_by_source
751-
.get(&session.diff_source)
752-
.copied()
753-
.is_some_and(|latest| latest > session.started_at);
766+
let stale_review = session
767+
.github_head_sha
768+
.as_ref()
769+
.zip(latest_by_source.get(&session.diff_source))
770+
.is_some_and(|(current_head, latest)| latest.head_sha != *current_head);
754771

755772
if let Some(summary) = session.summary.take() {
756773
session.summary = Some(CommentSynthesizer::apply_runtime_review_state(
@@ -779,7 +796,7 @@ pub async fn get_review(
779796
};
780797

781798
let inventory = load_review_inventory(&state).await;
782-
let latest_by_source = latest_review_started_by_source(&inventory);
799+
let latest_by_source = latest_review_head_by_source(&inventory);
783800
Ok(Json(apply_dynamic_review_state(session, &latest_by_source)))
784801
}
785802

@@ -791,7 +808,7 @@ pub async fn list_reviews(
791808
let per_page = params.per_page.unwrap_or(20).clamp(1, 100);
792809

793810
let mut reviews = load_review_inventory(&state).await;
794-
let latest_by_source = latest_review_started_by_source(&reviews);
811+
let latest_by_source = latest_review_head_by_source(&reviews);
795812
reviews = reviews
796813
.into_iter()
797814
.map(|session| apply_dynamic_review_state(session, &latest_by_source))
@@ -1499,6 +1516,31 @@ async fn github_api_get_diff(
14991516
Ok(text)
15001517
}
15011518

1519+
async fn fetch_github_pr_head_sha(
1520+
client: &reqwest::Client,
1521+
token: &str,
1522+
repo: &str,
1523+
pr_number: u32,
1524+
) -> Result<String, String> {
1525+
let pr_url = format!("https://api.github.com/repos/{}/pulls/{}", repo, pr_number);
1526+
let pr_resp = github_api_get(client, token, &pr_url).await?;
1527+
if !pr_resp.status().is_success() {
1528+
let status = pr_resp.status();
1529+
let body = pr_resp.text().await.unwrap_or_default();
1530+
return Err(format!("Failed to get PR info {}: {}", status, body));
1531+
}
1532+
let pr_data: serde_json::Value = pr_resp
1533+
.json()
1534+
.await
1535+
.map_err(|e| format!("Failed to parse PR response: {}", e))?;
1536+
pr_data
1537+
.get("head")
1538+
.and_then(|head| head.get("sha"))
1539+
.and_then(|value| value.as_str())
1540+
.map(str::to_string)
1541+
.ok_or_else(|| "No head SHA in PR response".to_string())
1542+
}
1543+
15021544
// === GitHub integration types and handlers ===
15031545

15041546
#[derive(Serialize)]
@@ -2044,6 +2086,10 @@ pub async fn start_pr_review(
20442086
let diff_content = github_api_get_diff(&state.http_client, &token, &diff_url)
20452087
.await
20462088
.map_err(|e| (StatusCode::BAD_GATEWAY, e))?;
2089+
let head_sha =
2090+
fetch_github_pr_head_sha(&state.http_client, &token, &request.repo, request.pr_number)
2091+
.await
2092+
.map_err(|e| (StatusCode::BAD_GATEWAY, e))?;
20472093

20482094
let id = Uuid::new_v4().to_string();
20492095
let diff_source = format!("pr:{}#{}", request.repo, request.pr_number);
@@ -2052,6 +2098,7 @@ pub async fn start_pr_review(
20522098
id: id.clone(),
20532099
status: ReviewStatus::Pending,
20542100
diff_source: diff_source.clone(),
2101+
github_head_sha: Some(head_sha.clone()),
20552102
started_at: current_timestamp(),
20562103
completed_at: None,
20572104
comments: Vec::new(),
@@ -2070,6 +2117,7 @@ pub async fn start_pr_review(
20702117
let review_id = id.clone();
20712118
let repo = request.repo.clone();
20722119
let pr_number = request.pr_number;
2120+
let pr_head_sha = head_sha.clone();
20732121
let post_results = request.post_results;
20742122

20752123
tokio::spawn(async move {
@@ -2079,6 +2127,7 @@ pub async fn start_pr_review(
20792127
diff_content,
20802128
repo,
20812129
pr_number,
2130+
pr_head_sha,
20822131
post_results,
20832132
)
20842133
.await;
@@ -2096,6 +2145,7 @@ async fn run_pr_review_task(
20962145
diff_content: String,
20972146
repo: String,
20982147
pr_number: u32,
2148+
_head_sha: String,
20992149
post_results: bool,
21002150
) {
21012151
let _permit = match state.review_semaphore.clone().acquire_owned().await {
@@ -3094,4 +3144,58 @@ mod tests {
30943144
assert_eq!(req.pr_number, 42);
30953145
assert!(req.post_results);
30963146
}
3147+
3148+
fn make_pr_review_session(
3149+
id: &str,
3150+
started_at: i64,
3151+
head_sha: &str,
3152+
) -> crate::server::state::ReviewSession {
3153+
crate::server::state::ReviewSession {
3154+
id: id.to_string(),
3155+
status: crate::server::state::ReviewStatus::Complete,
3156+
diff_source: "pr:owner/repo#42".to_string(),
3157+
github_head_sha: Some(head_sha.to_string()),
3158+
started_at,
3159+
completed_at: Some(started_at + 1),
3160+
comments: Vec::new(),
3161+
summary: Some(crate::core::CommentSynthesizer::generate_summary(&[])),
3162+
files_reviewed: 0,
3163+
error: None,
3164+
pr_summary_text: None,
3165+
diff_content: None,
3166+
event: None,
3167+
progress: None,
3168+
}
3169+
}
3170+
3171+
#[test]
3172+
fn stale_detection_ignores_same_head_reruns() {
3173+
let older = make_pr_review_session("r1", 10, "sha-a");
3174+
let newer_same_head = make_pr_review_session("r2", 20, "sha-a");
3175+
let latest_by_source = latest_review_head_by_source(&[older.clone(), newer_same_head]);
3176+
let updated = apply_dynamic_review_state(older, &latest_by_source);
3177+
let summary = updated.summary.expect("summary");
3178+
assert_eq!(
3179+
summary.merge_readiness,
3180+
crate::core::comment::MergeReadiness::Ready
3181+
);
3182+
assert!(summary.readiness_reasons.is_empty());
3183+
}
3184+
3185+
#[test]
3186+
fn stale_detection_requires_newer_head_sha() {
3187+
let older = make_pr_review_session("r1", 10, "sha-a");
3188+
let newer_head = make_pr_review_session("r2", 20, "sha-b");
3189+
let latest_by_source = latest_review_head_by_source(&[older.clone(), newer_head]);
3190+
let updated = apply_dynamic_review_state(older, &latest_by_source);
3191+
let summary = updated.summary.expect("summary");
3192+
assert_eq!(
3193+
summary.merge_readiness,
3194+
crate::core::comment::MergeReadiness::NeedsReReview
3195+
);
3196+
assert_eq!(
3197+
summary.readiness_reasons,
3198+
vec!["new commits landed after this review".to_string()]
3199+
);
3200+
}
30973201
}

src/server/github.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,7 @@ pub async fn handle_webhook(
419419
id: review_id.clone(),
420420
status: ReviewStatus::Pending,
421421
diff_source: diff_source.clone(),
422+
github_head_sha: Some(head_sha.clone()),
422423
started_at: current_timestamp(),
423424
completed_at: None,
424425
comments: Vec::new(),

src/server/state.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,8 @@ pub struct ReviewSession {
127127
pub id: String,
128128
pub status: ReviewStatus,
129129
pub diff_source: String,
130+
#[serde(default)]
131+
pub github_head_sha: Option<String>,
130132
pub started_at: i64,
131133
pub completed_at: Option<i64>,
132134
pub comments: Vec<Comment>,

src/server/storage_json.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,7 @@ mod tests {
424424
id: id.to_string(),
425425
status,
426426
diff_source: "head".to_string(),
427+
github_head_sha: None,
427428
started_at,
428429
completed_at: None,
429430
comments: vec![],

0 commit comments

Comments
 (0)