feat(review): infer addressed findings from follow-up diffs

Author: haasonsaas

TODO.md

@@ -23,7 +23,7 @@ This roadmap is derived from deep research into Greptile's public docs, blog, MC
 ## 1. Feedback, Memory, and Outcomes
 
 1. [x] Add first-class comment outcome states beyond thumbs: `new`, `accepted`, `rejected`, `addressed`, `stale`, `auto_fixed`.
-2. [ ] Infer "addressed by later commit" by diffing follow-up pushes against the original commented lines.
+2. [x] Infer "addressed by later commit" by diffing follow-up pushes against the original commented lines.
 3. [ ] Feed addressed/not-addressed outcomes into the reinforcement store alongside thumbs.
 4. [x] Separate false-positive rejections from "valid but won't fix" dismissals in stored feedback.
 5. [ ] Weight reinforcement by reviewer role or trust level when GitHub identity is available.

src/core/comment.rs

@@ -37,11 +37,11 @@ use summary::{
 use tags::extract_tags;
 
 pub use identity::compute_comment_id;
-pub use outcomes::derive_comment_outcomes;
+pub use outcomes::{derive_comment_outcomes, infer_addressed_by_follow_up_comments};
 pub use types::{
-    Category, CodeSuggestion, Comment, CommentOutcome, CommentStatus, FixEffort, FixLoopTelemetry,
-    MergeReadiness, RawComment, ReviewCompletenessSummary, ReviewSummary, ReviewVerificationState,
-    ReviewVerificationSummary, Severity,
+    Category, CodeSuggestion, Comment, CommentOutcome, CommentOutcomeContext, CommentStatus,
+    FixEffort, FixLoopTelemetry, MergeReadiness, RawComment, ReviewCompletenessSummary,
+    ReviewSummary, ReviewVerificationState, ReviewVerificationSummary, Severity,
 };
 
 pub struct CommentSynthesizer;

src/core/comment/outcomes.rs

@@ -1,6 +1,62 @@
-use super::{Comment, CommentOutcome, CommentStatus};
+use std::collections::{HashMap, HashSet};
+use std::path::Path;
 
-pub fn derive_comment_outcomes(comment: &Comment, stale_review: bool) -> Vec<CommentOutcome> {
+use crate::core::{diff_parser::ChangeType, UnifiedDiff};
+
+use super::{Comment, CommentOutcome, CommentOutcomeContext, CommentStatus};
+
+fn push_unique(outcomes: &mut Vec<CommentOutcome>, outcome: CommentOutcome) {
+    if !outcomes.contains(&outcome) {
+        outcomes.push(outcome);
+    }
+}
+
+fn normalize_comment_path(path: &Path) -> String {
+    path.to_string_lossy()
+        .replace('\\', "/")
+        .trim_start_matches("./")
+        .to_string()
+}
+
+pub fn infer_addressed_by_follow_up_comments(
+    comments: &[Comment],
+    follow_up_diffs: &[UnifiedDiff],
+) -> HashSet<String> {
+    let mut changed_old_lines_by_path: HashMap<String, HashSet<usize>> = HashMap::new();
+
+    for diff in follow_up_diffs {
+        let changed_old_lines = changed_old_lines_by_path
+            .entry(normalize_comment_path(diff.file_path.as_path()))
+            .or_default();
+
+        for hunk in &diff.hunks {
+            for line in &hunk.changes {
+                if line.change_type != ChangeType::Context {
+                    if let Some(old_line_no) = line.old_line_no {
+                        changed_old_lines.insert(old_line_no);
+                    }
+                }
+            }
+        }
+    }
+
+    comments
+        .iter()
+        .filter(|comment| comment.status == CommentStatus::Open)
+        .filter_map(|comment| {
+            let path = normalize_comment_path(comment.file_path.as_path());
+            changed_old_lines_by_path
+                .get(&path)
+                .filter(|changed_lines| changed_lines.contains(&comment.line_number))
+                .map(|_| comment.id.clone())
+        })
+        .collect()
+}
+
+pub fn derive_comment_outcomes(
+    comment: &Comment,
+    context: CommentOutcomeContext,
+) -> Vec<CommentOutcome> {
     let mut outcomes = Vec::new();
 
     match comment
@@ -10,31 +66,40 @@ pub fn derive_comment_outcomes(comment: &Comment, stale_review: bool) -> Vec<Com
         .map(str::to_ascii_lowercase)
         .as_deref()
     {
-        Some("accept") => outcomes.push(CommentOutcome::Accepted),
-        Some("reject") => outcomes.push(CommentOutcome::Rejected),
+        Some("accept") => push_unique(&mut outcomes, CommentOutcome::Accepted),
+        Some("reject") => push_unique(&mut outcomes, CommentOutcome::Rejected),
         _ => {}
     }
 
-    if comment.status == CommentStatus::Resolved {
-        outcomes.push(CommentOutcome::Addressed);
+    if comment.status == CommentStatus::Resolved || context.addressed_by_follow_up {
+        push_unique(&mut outcomes, CommentOutcome::Addressed);
+    }
+
+    if context.auto_fixed {
+        push_unique(&mut outcomes, CommentOutcome::Addressed);
+        push_unique(&mut outcomes, CommentOutcome::AutoFixed);
     }
 
-    if comment.status == CommentStatus::Open && stale_review {
-        outcomes.push(CommentOutcome::Stale);
+    if comment.status == CommentStatus::Open && context.stale_review {
+        push_unique(&mut outcomes, CommentOutcome::Stale);
     }
 
     if outcomes.is_empty() && comment.status == CommentStatus::Open {
-        outcomes.push(CommentOutcome::New);
+        push_unique(&mut outcomes, CommentOutcome::New);
     }
 
     outcomes
 }
 
 #[cfg(test)]
 mod tests {
+    use std::collections::HashSet;
     use std::path::PathBuf;
 
-    use crate::core::comment::{Category, Comment, FixEffort, Severity};
+    use crate::core::{
+        comment::{Category, Comment, CommentOutcomeContext, FixEffort, Severity},
+        DiffParser,
+    };
 
     use super::*;
 
@@ -61,7 +126,7 @@ mod tests {
     #[test]
     fn derives_new_for_open_comments_without_other_signals() {
         assert_eq!(
-            derive_comment_outcomes(&make_comment(), false),
+            derive_comment_outcomes(&make_comment(), CommentOutcomeContext::default()),
             vec![CommentOutcome::New]
         );
     }
@@ -73,7 +138,7 @@ mod tests {
         comment.status = CommentStatus::Resolved;
 
         assert_eq!(
-            derive_comment_outcomes(&comment, false),
+            derive_comment_outcomes(&comment, CommentOutcomeContext::default()),
             vec![CommentOutcome::Accepted, CommentOutcome::Addressed]
         );
     }
@@ -84,24 +149,79 @@ mod tests {
         comment.feedback = Some("reject".to_string());
 
         assert_eq!(
-            derive_comment_outcomes(&comment, false),
+            derive_comment_outcomes(&comment, CommentOutcomeContext::default()),
             vec![CommentOutcome::Rejected]
         );
     }
 
     #[test]
     fn derives_stale_for_open_comments_in_stale_reviews() {
         assert_eq!(
-            derive_comment_outcomes(&make_comment(), true),
+            derive_comment_outcomes(
+                &make_comment(),
+                CommentOutcomeContext {
+                    stale_review: true,
+                    ..CommentOutcomeContext::default()
+                }
+            ),
             vec![CommentOutcome::Stale]
         );
     }
 
+    #[test]
+    fn derives_addressed_for_open_comments_touched_by_follow_up_commits() {
+        assert_eq!(
+            derive_comment_outcomes(
+                &make_comment(),
+                CommentOutcomeContext {
+                    addressed_by_follow_up: true,
+                    ..CommentOutcomeContext::default()
+                }
+            ),
+            vec![CommentOutcome::Addressed]
+        );
+    }
+
     #[test]
     fn dismissed_comments_keep_lifecycle_without_derived_outcomes() {
         let mut comment = make_comment();
         comment.status = CommentStatus::Dismissed;
 
-        assert!(derive_comment_outcomes(&comment, false).is_empty());
+        assert!(derive_comment_outcomes(&comment, CommentOutcomeContext::default()).is_empty());
+    }
+
+    #[test]
+    fn infer_follow_up_marks_open_comments_as_addressed_when_old_line_changes() {
+        let diffs = DiffParser::parse_text_diff(
+            "first\nsecond\nthird\n",
+            "first\nupdated second\nthird\n",
+            PathBuf::from("src/lib.rs"),
+        )
+        .unwrap();
+
+        let mut comment = make_comment();
+        comment.line_number = 2;
+
+        assert_eq!(
+            infer_addressed_by_follow_up_comments(&[comment], &[diffs])
+                .into_iter()
+                .collect::<HashSet<_>>(),
+            HashSet::from(["comment-1".to_string()])
+        );
+    }
+
+    #[test]
+    fn infer_follow_up_ignores_context_only_line_matches() {
+        let diffs = DiffParser::parse_text_diff(
+            "first\nsecond\nthird\n",
+            "first\ninserted\nsecond\nthird\n",
+            PathBuf::from("src/lib.rs"),
+        )
+        .unwrap();
+
+        let mut comment = make_comment();
+        comment.line_number = 2;
+
+        assert!(infer_addressed_by_follow_up_comments(&[comment], &[diffs]).is_empty());
     }
 }

src/core/comment/types.rs

@@ -133,6 +133,13 @@ pub enum CommentOutcome {
     AutoFixed,
 }
 
+#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
+pub struct CommentOutcomeContext {
+    pub stale_review: bool,
+    pub addressed_by_follow_up: bool,
+    pub auto_fixed: bool,
+}
+
 #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Default)]
 pub enum MergeReadiness {
     Ready,