Skip to content

Commit a651ace

Browse files
committed
feat(review): learn from feedback explanations
1 parent ff62123 commit a651ace

File tree

15 files changed

+572
-52
lines changed

15 files changed

+572
-52
lines changed

TODO.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ This roadmap is derived from deep research into Greptile's public docs, blog, MC
2929
5. [ ] Weight reinforcement by reviewer role or trust level when GitHub identity is available.
3030
6. [x] Add rule-level reinforcement decay so old team preferences do not dominate forever.
3131
7. [x] Add path-scoped reinforcement buckets so teams can prefer different standards in `tests/`, `scripts/`, and production code.
32-
8. [ ] Persist explanation text from follow-up feedback replies and mine it into reusable review guidance.
32+
8. [x] Persist explanation text from follow-up feedback replies and mine it into reusable review guidance.
3333
9. [x] Learn "preferred phrasing" for accepted comments so comment tone and specificity improve over time.
3434
10. [x] Backfill existing stored reviews into the new outcome-aware feedback store for cold-start reduction.
3535

src/review/feedback.rs

Lines changed: 71 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,10 @@ pub use record::{
2828
#[allow(unused_imports)]
2929
pub use semantic::{record_semantic_feedback_example, record_semantic_feedback_examples};
3030
#[allow(unused_imports)]
31-
pub use store::{DecayedFeedbackStats, FeedbackPatternStats, FeedbackStore, FeedbackTypeStats};
31+
pub use store::{
32+
DecayedFeedbackStats, FeedbackExplanation, FeedbackPatternStats, FeedbackStore,
33+
FeedbackTypeStats,
34+
};
3235

3336
#[cfg(test)]
3437
mod tests {
@@ -49,6 +52,7 @@ mod tests {
4952
assert!(store.by_category_file_pattern.is_empty());
5053
assert!(store.by_rule.is_empty());
5154
assert!(store.by_rule_file_pattern.is_empty());
55+
assert!(store.explanations_by_comment.is_empty());
5256
}
5357

5458
#[test]
@@ -99,6 +103,19 @@ mod tests {
99103
not_addressed: 5,
100104
},
101105
);
106+
store.explanations_by_comment.insert(
107+
"review-1::comment-1".to_string(),
108+
FeedbackExplanation {
109+
review_id: "review-1".to_string(),
110+
comment_id: "comment-1".to_string(),
111+
action: "accept".to_string(),
112+
category: "Security".to_string(),
113+
rule_id: Some("sec.auth.boundary".to_string()),
114+
file_patterns: vec!["src/**".to_string()],
115+
text: "Tenant boundaries must stay explicit.".to_string(),
116+
updated_at: "2026-03-15T00:00:00Z".to_string(),
117+
},
118+
);
102119

103120
let json = serde_json::to_string(&store).unwrap();
104121
let deserialized: FeedbackStore = serde_json::from_str(&json).unwrap();
@@ -116,6 +133,7 @@ mod tests {
116133
deserialized.by_rule["style.rule"].decayed_total_at(123),
117134
Some(2.0)
118135
);
136+
assert_eq!(deserialized.explanations_by_comment.len(), 1);
119137
}
120138

121139
#[test]
@@ -357,4 +375,56 @@ mod tests {
357375
"Should mention file pattern: {context}"
358376
);
359377
}
378+
379+
#[test]
380+
fn generate_feedback_context_includes_feedback_explanation_guidance() {
381+
let mut store = FeedbackStore::default();
382+
let comment = crate::core::comment::Comment {
383+
id: "comment-1".to_string(),
384+
file_path: Path::new("src/auth.rs").to_path_buf(),
385+
line_number: 10,
386+
content: "Guard the tenant boundary before the query".to_string(),
387+
rule_id: Some("sec.auth.boundary".to_string()),
388+
severity: crate::core::comment::Severity::Error,
389+
category: crate::core::comment::Category::Security,
390+
suggestion: None,
391+
confidence: 0.9,
392+
code_suggestion: None,
393+
tags: Vec::new(),
394+
fix_effort: crate::core::comment::FixEffort::Medium,
395+
feedback: Some("accept".to_string()),
396+
status: crate::core::comment::CommentStatus::Open,
397+
resolved_at: None,
398+
};
399+
400+
assert!(store.record_feedback_explanation(
401+
"review-1",
402+
&comment,
403+
&["src/**"],
404+
"accept",
405+
"This keeps tenant isolation explicit before any data access.",
406+
"2026-03-15T00:00:00Z",
407+
));
408+
assert!(store.record_feedback_explanation(
409+
"review-2",
410+
&crate::core::comment::Comment {
411+
id: "comment-2".to_string(),
412+
..comment.clone()
413+
},
414+
&["src/**"],
415+
"accept",
416+
"Tenant isolation must stay explicit to avoid cross-account reads.",
417+
"2026-03-15T00:01:00Z",
418+
));
419+
420+
let context = generate_feedback_context(&store);
421+
assert!(
422+
context.contains("sec.auth.boundary"),
423+
"Should mention the rule: {context}"
424+
);
425+
assert!(
426+
context.contains("tenant isolation"),
427+
"Should surface explanation-derived guidance: {context}"
428+
);
429+
}
360430
}

src/review/feedback/context.rs

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,15 @@
11
use super::store::FeedbackStore;
22

3+
const MIN_EXPLANATION_OBSERVATIONS: usize = 2;
4+
const MAX_EXPLANATION_GUIDANCE_ITEMS: usize = 4;
5+
6+
#[derive(Default)]
7+
struct ExplanationGuidanceBucket {
8+
accepted: usize,
9+
rejected: usize,
10+
snippets: Vec<String>,
11+
}
12+
313
/// Generate feedback context to inject into the review prompt.
414
///
515
/// Scans the feedback store for statistically significant patterns
@@ -39,6 +49,80 @@ pub fn generate_feedback_context(store: &FeedbackStore) -> String {
3949
}
4050
}
4151

52+
let mut explanation_buckets =
53+
std::collections::HashMap::<String, ExplanationGuidanceBucket>::new();
54+
for explanation in store.explanations_by_comment.values() {
55+
let bucket_key = explanation
56+
.rule_id
57+
.as_deref()
58+
.map(|rule_id| format!("rule::{rule_id}"))
59+
.unwrap_or_else(|| format!("category::{}", explanation.category));
60+
let bucket = explanation_buckets.entry(bucket_key).or_default();
61+
if explanation.action == "accept" {
62+
bucket.accepted += 1;
63+
} else {
64+
bucket.rejected += 1;
65+
}
66+
67+
if let Some(snippet) = explanation_snippet(&explanation.text) {
68+
let already_seen = bucket
69+
.snippets
70+
.iter()
71+
.any(|existing| existing.eq_ignore_ascii_case(&snippet));
72+
if !already_seen {
73+
bucket.snippets.push(snippet);
74+
}
75+
}
76+
}
77+
78+
let mut explanation_guidance = explanation_buckets
79+
.into_iter()
80+
.filter_map(|(bucket_key, bucket)| {
81+
let total = bucket.accepted + bucket.rejected;
82+
if total < MIN_EXPLANATION_OBSERVATIONS
83+
|| bucket.snippets.is_empty()
84+
|| bucket.accepted == bucket.rejected
85+
{
86+
return None;
87+
}
88+
89+
let snippets = bucket
90+
.snippets
91+
.into_iter()
92+
.take(2)
93+
.collect::<Vec<_>>()
94+
.join("; ");
95+
if snippets.is_empty() {
96+
return None;
97+
}
98+
99+
let label = format_explanation_bucket_label(&bucket_key);
100+
if bucket.accepted >= bucket.rejected {
101+
Some((
102+
total,
103+
format!(
104+
"- Reviewers accepted similar {label} findings when they noted: {snippets}",
105+
),
106+
))
107+
} else {
108+
Some((
109+
total,
110+
format!(
111+
"- Reviewers rejected similar {label} findings when they noted: {snippets} — avoid flagging them unless the same concern clearly applies",
112+
),
113+
))
114+
}
115+
})
116+
.collect::<Vec<_>>();
117+
explanation_guidance
118+
.sort_by(|left, right| right.0.cmp(&left.0).then_with(|| left.1.cmp(&right.1)));
119+
patterns.extend(
120+
explanation_guidance
121+
.into_iter()
122+
.take(MAX_EXPLANATION_GUIDANCE_ITEMS)
123+
.map(|(_, guidance)| guidance),
124+
);
125+
42126
patterns.truncate(10);
43127

44128
if patterns.is_empty() {
@@ -54,3 +138,33 @@ pub fn generate_feedback_context(store: &FeedbackStore) -> String {
54138
}
55139
context
56140
}
141+
142+
fn explanation_snippet(text: &str) -> Option<String> {
143+
let snippet = text
144+
.replace("\r\n", "\n")
145+
.split(['\n', '.', '!', '?'])
146+
.map(str::trim)
147+
.find(|segment| !segment.is_empty())?
148+
.chars()
149+
.take(140)
150+
.collect::<String>();
151+
152+
if snippet.is_empty() {
153+
None
154+
} else {
155+
Some(snippet)
156+
}
157+
}
158+
159+
fn format_explanation_bucket_label(bucket_key: &str) -> String {
160+
if let Some(rule_id) = bucket_key.strip_prefix("rule::") {
161+
format!("rule `{rule_id}`")
162+
} else if let Some(category) = bucket_key.strip_prefix("category::") {
163+
match category {
164+
"BestPractice" => "best practice".to_string(),
165+
_ => category.to_lowercase(),
166+
}
167+
} else {
168+
"feedback".to_string()
169+
}
170+
}

0 commit comments

Comments
 (0)