Skip to content

Commit 21bff1d

Browse files
haasonsaashaasonsaas
committed
feat: add review completeness metrics
1 parent de2378f commit 21bff1d

File tree

16 files changed

+184
-7
lines changed

16 files changed

+184
-7
lines changed

‎TODO.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ This roadmap is derived from deep research into Greptile's public docs, blog, MC
3636
## 2. Review Lifecycle and Merge Readiness
3737

3838
11. [x] Track unresolved vs resolved findings for PR reviews as a first-class lifecycle state.
39-
12. [ ] Add review completeness metrics: total findings, acknowledged findings, fixed findings, stale findings.
39+
12. [x] 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.
4141
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.
@@ -178,4 +178,5 @@ This roadmap is derived from deep research into Greptile's public docs, blog, MC
178178
- [x] Group ReviewView list-mode findings into unresolved, fixed, stale, and informational sections.
179179
- [x] Add ReviewView keyboard shortcuts for next-finding navigation plus accept/reject/resolve actions.
180180
- [x] Add path-scoped reinforcement buckets so feedback can distinguish `tests/**`, `scripts/**`, and broader code areas.
181+
- [x] Add review completeness metrics across summaries, PR readiness, and ReviewView surfaces.
181182
- [ ] Commit and push each validated checkpoint before moving to the next epic.

‎src/commands/pr/readiness.rs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,12 @@ fn format_pr_readiness_markdown(snapshot: &PrReadinessSnapshot) -> String {
102102
"- Lifecycle: {} open · {} resolved · {} dismissed\n",
103103
summary.open_comments, summary.resolved_comments, summary.dismissed_comments
104104
));
105+
output.push_str(&format!(
106+
"- Completeness: {} acknowledged · {} fixed · {} stale\n",
107+
summary.completeness.acknowledged_findings,
108+
summary.completeness.fixed_findings,
109+
summary.completeness.stale_findings
110+
));
105111
output.push_str(&format!("- Verification: {}\n", summary.verification.state));
106112
if !summary.readiness_reasons.is_empty() {
107113
output.push_str("- Readiness reasons:\n");
@@ -139,6 +145,10 @@ mod tests {
139145
summary.open_comments = 3;
140146
summary.resolved_comments = 1;
141147
summary.dismissed_comments = 1;
148+
summary.completeness.total_findings = 5;
149+
summary.completeness.acknowledged_findings = 2;
150+
summary.completeness.fixed_findings = 1;
151+
summary.completeness.stale_findings = 3;
142152
summary.verification.state = ReviewVerificationState::Inconclusive;
143153
summary.readiness_reasons = vec!["new commits landed after this review".to_string()];
144154
let snapshot = PrReadinessSnapshot {
@@ -165,6 +175,7 @@ mod tests {
165175
assert!(output.contains("Current head: `0123456789ab`"));
166176
assert!(output.contains("Merge readiness: Needs attention"));
167177
assert!(output.contains("Open blockers: 2"));
178+
assert!(output.contains("Completeness: 2 acknowledged · 1 fixed · 3 stale"));
168179
assert!(output.contains("new commits landed after this review"));
169180
}
170181

‎src/core/comment.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ use tags::extract_tags;
3737
pub use identity::compute_comment_id;
3838
pub use types::{
3939
Category, CodeSuggestion, Comment, CommentStatus, FixEffort, MergeReadiness, RawComment,
40-
ReviewSummary, ReviewVerificationState, ReviewVerificationSummary, Severity,
40+
ReviewCompletenessSummary, ReviewSummary, ReviewVerificationState, ReviewVerificationSummary,
41+
Severity,
4142
};
4243

4344
pub struct CommentSynthesizer;

‎src/core/comment/summary.rs‎

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
use std::collections::{HashMap, HashSet};
22

33
use super::{
4-
Category, Comment, CommentStatus, MergeReadiness, ReviewSummary, ReviewVerificationState,
5-
ReviewVerificationSummary, Severity,
4+
Category, Comment, CommentStatus, MergeReadiness, ReviewCompletenessSummary, ReviewSummary,
5+
ReviewVerificationState, ReviewVerificationSummary, Severity,
66
};
77

88
pub(super) fn generate_summary(comments: &[Comment]) -> ReviewSummary {
@@ -65,6 +65,12 @@ pub(super) fn generate_summary(comments: &[Comment]) -> ReviewSummary {
6565
resolved_comments,
6666
dismissed_comments,
6767
open_blockers,
68+
completeness: build_completeness_summary(
69+
comments.len(),
70+
resolved_comments,
71+
dismissed_comments,
72+
0,
73+
),
6874
merge_readiness: default_merge_readiness(open_blockers),
6975
verification: ReviewVerificationSummary::default(),
7076
readiness_reasons: Vec::new(),
@@ -93,6 +99,17 @@ pub(super) fn apply_review_runtime_state(
9399
mut summary: ReviewSummary,
94100
stale_review: bool,
95101
) -> ReviewSummary {
102+
summary.completeness = build_completeness_summary(
103+
summary.total_comments,
104+
summary.resolved_comments,
105+
summary.dismissed_comments,
106+
if stale_review {
107+
summary.open_comments
108+
} else {
109+
0
110+
},
111+
);
112+
96113
let mut reasons = Vec::new();
97114
if matches!(
98115
summary.verification.state,
@@ -112,6 +129,20 @@ pub(super) fn apply_review_runtime_state(
112129
summary
113130
}
114131

132+
fn build_completeness_summary(
133+
total_findings: usize,
134+
resolved_comments: usize,
135+
dismissed_comments: usize,
136+
stale_findings: usize,
137+
) -> ReviewCompletenessSummary {
138+
ReviewCompletenessSummary {
139+
total_findings,
140+
acknowledged_findings: resolved_comments + dismissed_comments,
141+
fixed_findings: resolved_comments,
142+
stale_findings,
143+
}
144+
}
145+
115146
fn default_merge_readiness(open_blockers: usize) -> MergeReadiness {
116147
if open_blockers == 0 {
117148
MergeReadiness::Ready
@@ -238,6 +269,10 @@ mod tests {
238269
assert_eq!(summary.resolved_comments, 1);
239270
assert_eq!(summary.dismissed_comments, 1);
240271
assert_eq!(summary.open_blockers, 1);
272+
assert_eq!(summary.completeness.total_findings, 3);
273+
assert_eq!(summary.completeness.acknowledged_findings, 2);
274+
assert_eq!(summary.completeness.fixed_findings, 1);
275+
assert_eq!(summary.completeness.stale_findings, 0);
241276
assert_eq!(summary.open_by_severity.get("Error"), Some(&1));
242277
assert_eq!(summary.merge_readiness, MergeReadiness::NeedsAttention);
243278
assert_eq!(
@@ -344,9 +379,34 @@ mod tests {
344379
let summary = apply_review_runtime_state(generate_summary(&comments), true);
345380
assert_eq!(summary.open_blockers, 0);
346381
assert_eq!(summary.merge_readiness, MergeReadiness::NeedsReReview);
382+
assert_eq!(summary.completeness.stale_findings, 0);
347383
assert_eq!(
348384
summary.readiness_reasons,
349385
vec!["new commits landed after this review".to_string()]
350386
);
351387
}
388+
389+
#[test]
390+
fn stale_review_counts_open_findings_in_completeness() {
391+
let comments = vec![
392+
make_comment(
393+
"open-warning",
394+
Severity::Warning,
395+
Category::Bug,
396+
CommentStatus::Open,
397+
),
398+
make_comment(
399+
"resolved-info",
400+
Severity::Info,
401+
Category::Documentation,
402+
CommentStatus::Resolved,
403+
),
404+
];
405+
406+
let summary = apply_review_runtime_state(generate_summary(&comments), true);
407+
assert_eq!(summary.completeness.total_findings, 2);
408+
assert_eq!(summary.completeness.acknowledged_findings, 1);
409+
assert_eq!(summary.completeness.fixed_findings, 1);
410+
assert_eq!(summary.completeness.stale_findings, 1);
411+
}
352412
}

‎src/core/comment/types.rs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,27 @@ pub struct ReviewSummary {
5656
#[serde(default)]
5757
pub open_blockers: usize,
5858
#[serde(default)]
59+
pub completeness: ReviewCompletenessSummary,
60+
#[serde(default)]
5961
pub merge_readiness: MergeReadiness,
6062
#[serde(default)]
6163
pub verification: ReviewVerificationSummary,
6264
#[serde(default, skip_serializing_if = "Vec::is_empty")]
6365
pub readiness_reasons: Vec<String>,
6466
}
6567

68+
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Default)]
69+
pub struct ReviewCompletenessSummary {
70+
#[serde(default)]
71+
pub total_findings: usize,
72+
#[serde(default)]
73+
pub acknowledged_findings: usize,
74+
#[serde(default)]
75+
pub fixed_findings: usize,
76+
#[serde(default)]
77+
pub stale_findings: usize,
78+
}
79+
6680
#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Default)]
6781
pub enum CommentStatus {
6882
#[default]

‎src/output/format.rs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,15 @@ pub async fn output_comments(
3232
Ok(())
3333
}
3434

35+
fn format_completeness(summary: &core::comment::ReviewSummary) -> String {
36+
format!(
37+
"{} acknowledged · {} fixed · {} stale",
38+
summary.completeness.acknowledged_findings,
39+
summary.completeness.fixed_findings,
40+
summary.completeness.stale_findings
41+
)
42+
}
43+
3544
pub fn format_as_patch(comments: &[core::Comment]) -> String {
3645
let mut output = String::new();
3746
for comment in comments {
@@ -87,6 +96,10 @@ pub fn format_as_markdown(comments: &[core::Comment], rule_priority: &[String])
8796
"📌 **Lifecycle:** {} open · {} resolved · {} dismissed\n\n",
8897
summary.open_comments, summary.resolved_comments, summary.dismissed_comments
8998
));
99+
output.push_str(&format!(
100+
"📎 **Completeness:** {}\n\n",
101+
format_completeness(&summary)
102+
));
90103
output.push_str(&format!(
91104
"⛔ **Open Blockers:** {}\n\n",
92105
summary.open_blockers
@@ -307,6 +320,10 @@ pub fn format_smart_review_output(
307320
"📌 **Lifecycle:** {} open · {} resolved · {} dismissed\n\n",
308321
summary.open_comments, summary.resolved_comments, summary.dismissed_comments
309322
));
323+
output.push_str(&format!(
324+
"📎 **Completeness:** {}\n\n",
325+
format_completeness(summary)
326+
));
310327
output.push_str(&format!(
311328
"⛔ **Open Blockers:** {}\n\n",
312329
summary.open_blockers
@@ -916,6 +933,12 @@ mod tests {
916933
resolved_comments: 0,
917934
dismissed_comments: 0,
918935
open_blockers: 2,
936+
completeness: crate::core::comment::ReviewCompletenessSummary {
937+
total_findings: 2,
938+
acknowledged_findings: 0,
939+
fixed_findings: 0,
940+
stale_findings: 0,
941+
},
919942
merge_readiness: crate::core::comment::MergeReadiness::NeedsAttention,
920943
verification: crate::core::comment::ReviewVerificationSummary::default(),
921944
readiness_reasons: Vec::new(),
@@ -939,6 +962,7 @@ mod tests {
939962
assert!(output.contains("Executive Summary"));
940963
assert!(output.contains("7.5/10"));
941964
assert!(output.contains("Critical Issues"));
965+
assert!(output.contains("Completeness"));
942966
assert!(output.contains("Fix bugs"));
943967
}
944968

‎src/review/rule_helpers/reporting.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,7 @@ mod tests {
211211
let body = build_pr_summary_comment_body(&[c], &[]);
212212
assert!(body.contains("DiffScope Review Summary"));
213213
assert!(body.contains("Total issues: 1"));
214+
assert!(body.contains("Completeness: 0 acknowledged / 0 fixed / 0 stale"));
214215
assert!(body.contains("sec.xss"));
215216
}
216217

‎src/review/rule_helpers/reporting/summary.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,12 @@ pub fn build_pr_summary_comment_body(
2222
"- Lifecycle: {} open / {} resolved / {} dismissed\n",
2323
summary.open_comments, summary.resolved_comments, summary.dismissed_comments
2424
));
25+
body.push_str(&format!(
26+
"- Completeness: {} acknowledged / {} fixed / {} stale\n",
27+
summary.completeness.acknowledged_findings,
28+
summary.completeness.fixed_findings,
29+
summary.completeness.stale_findings
30+
));
2531
body.push_str(&format!("- Open blockers: {}\n", summary.open_blockers));
2632
body.push_str(&format!(
2733
"- Blocking open: {} | Informational open: {}\n",

‎src/server/github.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -706,11 +706,14 @@ async fn create_check_run(
706706
};
707707

708708
let summary_text = format!(
709-
"**Score:** {:.1}/10 | **Findings:** {} | **Files:** {} | **Readiness:** {}\n\n{}{}",
709+
"**Score:** {:.1}/10 | **Findings:** {} | **Files:** {} | **Readiness:** {}\n**Completeness:** {} acknowledged | {} fixed | {} stale\n\n{}{}",
710710
summary.overall_score,
711711
summary.total_comments,
712712
summary.files_reviewed,
713713
summary.merge_readiness,
714+
summary.completeness.acknowledged_findings,
715+
summary.completeness.fixed_findings,
716+
summary.completeness.stale_findings,
714717
if summary.recommendations.is_empty() {
715718
String::new()
716719
} else {

‎src/server/storage_json.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -540,6 +540,12 @@ mod tests {
540540
resolved_comments: 0,
541541
dismissed_comments: 0,
542542
open_blockers: 2,
543+
completeness: crate::core::comment::ReviewCompletenessSummary {
544+
total_findings: 2,
545+
acknowledged_findings: 0,
546+
fixed_findings: 0,
547+
stale_findings: 0,
548+
},
543549
merge_readiness: crate::core::comment::MergeReadiness::NeedsAttention,
544550
verification: crate::core::comment::ReviewVerificationSummary::default(),
545551
readiness_reasons: Vec::new(),

0 commit comments

Comments
 (0)