1use anyhow::Result;
18
19use crate::queue::{FollowUp, Queue, Source, Task};
20use crate::run::{FollowupRecord, RunState};
21use crate::verdict::{Finding, ReviewVote};
22
23pub const NODE: &str = "followup";
25
26pub const MAX_FOLLOWUP_GENERATION: u32 = 2;
28
29const LINE_WINDOW: u32 = 5;
31
32#[derive(Debug, Default, PartialEq, Eq)]
34pub struct Outcome {
35 pub filed: Vec<String>,
37 pub capped: Vec<String>,
39 pub unfiled: Vec<String>,
41 pub covered: Vec<(String, String)>,
44}
45
46pub async fn after_merge(state: &mut RunState, pr_url: &str) {
49 if !state.config.graph.file_followups {
50 return;
51 }
52 let queue = Queue::open();
53 let outcome = match file(state, pr_url, &queue) {
54 Ok(o) => o,
55 Err(e) => {
56 state.event(NODE, format!("follow-up filing failed: {e:#}"));
57 return;
58 }
59 };
60 if outcome.filed.is_empty()
61 && (state.followup_commented || (state.followups.is_empty() && outcome.covered.is_empty()))
62 {
63 return;
64 }
65 let body = comment_body(state, &outcome);
66 match crate::bump::gh_pr_comment(&state.repo, pr_url, &body).await {
67 Ok(()) => state.followup_commented = true,
68 Err(e) => state.event(NODE, format!("follow-up comment not posted: {e:#}")),
69 }
70}
71
72pub fn file(state: &mut RunState, pr_url: &str, queue: &Queue) -> Result<Outcome> {
76 let mut out = Outcome::default();
77 let Some(round) = state.reviews.last().cloned() else {
78 return Ok(out);
79 };
80 let rejecters: Vec<usize> = round
81 .final_votes()
82 .into_iter()
83 .filter(|(_, _, v)| *v == ReviewVote::Reject)
84 .map(|(seat, _, _)| seat)
85 .collect();
86 let mut chosen: Vec<Finding> = Vec::new();
87 for rec in &round.reviews {
88 let rejected = rejecters.contains(&rec.reviewer);
89 for f in &rec.findings {
90 if f.severity.blocks() || rejected {
91 chosen.push(f.clone());
92 } else {
93 out.unfiled.push(f.id.clone());
94 }
95 }
96 }
97 if chosen.is_empty() {
98 return Ok(out);
99 }
100
101 let origin_task = state.origin.as_ref().and_then(|o| o.task.clone());
102 let parent_gen = match state.followup_generation {
106 Some(g) => g,
107 None => match origin_task.as_deref().map(|t| queue.get(t)) {
108 Some(Ok(t)) => {
109 let g = t.followup.map_or(0, |f| f.generation);
110 state.followup_generation = Some(g);
111 g
112 }
113 Some(Err(_)) => MAX_FOLLOWUP_GENERATION,
114 None => 0,
115 },
116 };
117 if parent_gen >= MAX_FOLLOWUP_GENERATION {
118 out.capped = chosen.iter().map(|f| f.id.clone()).collect();
119 state.event(
120 NODE,
121 format!(
122 "generation cap reached ({parent_gen}); not filing follow-ups for open finding(s): {}",
123 out.capped.join(", ")
124 ),
125 );
126 return Ok(out);
127 }
128
129 let mut done: Vec<String> = state
130 .followups
131 .iter()
132 .flat_map(|r| r.findings.iter().cloned())
133 .collect();
134 for t in queue.list() {
135 if let Some(f) = t.followup
136 && f.run == state.id
137 {
138 done.extend(f.findings.iter().cloned());
139 if !state.followups.iter().any(|r| r.task == t.id) {
140 state.followups.push(FollowupRecord {
141 task: t.id,
142 findings: f.findings,
143 });
144 }
145 }
146 }
147
148 let tasks = queue.list();
149 let mut failure = None;
150 for group in group_findings(chosen) {
151 let ids: Vec<String> = group.iter().map(|f| f.id.clone()).collect();
152 let own = task_id(&state.id, &ids);
153 let covers: Vec<Option<String>> = ids
154 .iter()
155 .map(|id| covering_task(&tasks, &own, &state.id, pr_url, id))
156 .collect();
157 if covers.iter().all(Option::is_some) {
158 for (id, by) in ids.iter().zip(covers.into_iter().flatten()) {
159 state.event(
160 NODE,
161 format!("not filing follow-up for {id}: already covered by task {by}"),
162 );
163 out.covered.push((id.clone(), by));
164 }
165 continue;
166 }
167 if ids.iter().any(|id| done.contains(id)) {
168 continue;
169 }
170 let mut task = build_task(state, pr_url, origin_task.clone(), &group, parent_gen + 1);
171 match queue.create_new(&mut task) {
172 Ok(created) => {
173 if created {
174 out.filed.push(task.id.clone());
175 }
176 state.followups.push(FollowupRecord {
177 task: task.id,
178 findings: ids,
179 });
180 }
181 Err(e) => {
182 failure = Some(e);
183 break;
184 }
185 }
186 }
187 if !out.filed.is_empty() {
188 state.event(
189 NODE,
190 format!(
191 "filed {} follow-up task(s): ids {}",
192 out.filed.len(),
193 out.filed.join(", ")
194 ),
195 );
196 }
197 if let Some(e) = failure {
198 state.event(NODE, format!("follow-up filing stopped: {e:#}"));
199 }
200 Ok(out)
201}
202
203fn covering_task(tasks: &[Task], own: &str, run: &str, pr_url: &str, id: &str) -> Option<String> {
208 tasks
209 .iter()
210 .find(|t| {
211 t.id != own
212 && (t
213 .followup
214 .as_ref()
215 .is_some_and(|f| f.run == run && f.findings.iter().any(|x| x == id))
216 || (names_word(&t.instruction, id) && names_pr(&t.instruction, pr_url)))
217 })
218 .map(|t| t.id.clone())
219}
220
221fn is_name_char(c: char) -> bool {
224 c.is_ascii_alphanumeric() || matches!(c, '_' | '-')
225}
226
227fn names_word(text: &str, needle: &str) -> bool {
229 !needle.is_empty()
230 && text.match_indices(needle).any(|(i, _)| {
231 let before = text[..i].chars().next_back();
232 let after = text[i + needle.len()..].chars().next();
233 before.is_none_or(|c| !is_name_char(c)) && after.is_none_or(|c| !is_name_char(c))
234 })
235}
236
237fn names_pr(text: &str, pr_url: &str) -> bool {
240 let url = pr_url.trim_end_matches('/');
241 let digit_after = |rest: &str| rest.chars().next().is_some_and(|c| c.is_ascii_digit());
242 if !url.is_empty()
243 && text
244 .match_indices(url)
245 .any(|(i, _)| !digit_after(&text[i + url.len()..]))
246 {
247 return true;
248 }
249 let Some(n) = url
250 .rsplit('/')
251 .next()
252 .filter(|n| !n.is_empty() && n.bytes().all(|b| b.is_ascii_digit()))
253 else {
254 return false;
255 };
256 let tag = format!("#{n}");
257 text.match_indices(&tag).any(|(i, _)| {
258 let before = text[..i].chars().next_back();
259 before.is_none_or(|c| !is_name_char(c)) && !digit_after(&text[i + tag.len()..])
260 })
261}
262
263fn group_findings(mut findings: Vec<Finding>) -> Vec<Vec<Finding>> {
267 findings.sort_by(|a, b| (&a.file, a.line, &a.id).cmp(&(&b.file, b.line, &b.id)));
268 let mut groups: Vec<Vec<Finding>> = Vec::new();
269 for f in findings {
270 let hit = groups
271 .iter()
272 .position(|g| g.iter().any(|o| same_defect(o, &f)));
273 match hit {
274 Some(i) => groups[i].push(f),
275 None => groups.push(vec![f]),
276 }
277 }
278 for g in &mut groups {
279 g.sort_by(|a, b| a.id.cmp(&b.id));
280 }
281 groups
282}
283
284fn same_defect(a: &Finding, b: &Finding) -> bool {
285 match (&a.file, &b.file) {
286 (Some(x), Some(y)) => {
287 x == y
288 && match (a.line, b.line) {
289 (Some(l), Some(m)) => l.abs_diff(m) <= LINE_WINDOW,
290 (None, None) => true,
291 _ => false,
292 }
293 }
294 (None, None) => normalize(&a.title) == normalize(&b.title),
295 _ => false,
296 }
297}
298
299fn normalize(title: &str) -> String {
300 title
301 .to_lowercase()
302 .split(|c: char| !c.is_alphanumeric())
303 .filter(|w| !w.is_empty())
304 .collect::<Vec<_>>()
305 .join(" ")
306}
307
308fn task_id(run: &str, ids: &[String]) -> String {
312 let mut h: u64 = 0xcbf2_9ce4_8422_2325;
313 for b in ids.join("+").bytes() {
314 h ^= u64::from(b);
315 h = h.wrapping_mul(0x0100_0000_01b3);
316 }
317 format!("{run}-f{:05x}", h & 0xf_ffff)
318}
319
320fn location(f: &Finding) -> String {
321 match (&f.file, f.line) {
322 (Some(p), Some(l)) => format!("{p}:{l}"),
323 (Some(p), None) => p.clone(),
324 _ => "(no location)".to_owned(),
325 }
326}
327
328fn build_task(
329 state: &RunState,
330 pr_url: &str,
331 origin_task: Option<String>,
332 group: &[Finding],
333 generation: u32,
334) -> Task {
335 let ids: Vec<String> = group.iter().map(|f| f.id.clone()).collect();
336 let first = &group[0];
337 let title = format!(
338 "follow-up: {} ({})",
339 crate::queue::title_from(&first.title, 80),
340 location(first)
341 );
342 let mut s = format!(
343 "This is a follow-up to a change that was already merged: run {run} merged \
344 {pr_url} while the review finding(s) below were still open. Branch from the \
345 current main - do not use the merged branch - and fix the defect described, \
346 keeping the change small. If a finding no longer applies to current main, say so \
347 and change nothing for it.\n\n\
348 Merged run: {run}\nPull request: {pr_url}\n",
349 run = state.id,
350 );
351 if let Some(t) = &origin_task {
352 s.push_str(&format!("Original task: {t}\n"));
353 }
354 s.push_str("\n## Findings\n");
355 for f in group {
356 s.push_str(&format!(
357 "\n### {} [{:?}] {}\n\nLocation: {}\n\n{}\n",
358 f.id,
359 f.severity,
360 f.title,
361 location(f),
362 f.detail.trim()
363 ));
364 }
365 let mut task = Task::new(
366 title,
367 s,
368 state.repo.clone(),
369 Source::Agent {
370 run: state.id.clone(),
371 node: NODE.to_owned(),
372 },
373 );
374 task.id = task_id(&state.id, &ids);
375 task.solo = true;
376 task.followup = Some(FollowUp {
377 run: state.id.clone(),
378 origin_task,
379 pr: pr_url.to_owned(),
380 findings: ids,
381 generation,
382 });
383 task
384}
385
386fn comment_body(state: &RunState, out: &Outcome) -> String {
387 let mut s = format!(
388 "<!-- magi-followup run={} -->\nThis pull request merged with review findings still open.",
389 state.id
390 );
391 if state.followups.is_empty() {
392 s.push_str(" Each is already covered by another task:\n");
393 } else {
394 s.push_str(" They were filed as follow-up tasks:\n\n");
395 for r in &state.followups {
396 s.push_str(&format!("- `{}`: {}\n", r.task, r.findings.join(", ")));
397 }
398 if !out.covered.is_empty() {
399 s.push_str("\nNot filed, already covered:\n");
400 }
401 }
402 if !out.covered.is_empty() {
403 s.push('\n');
404 }
405 for (id, by) in &out.covered {
406 s.push_str(&format!("- {id}: already covered by task `{by}`\n"));
407 }
408 if !out.unfiled.is_empty() {
409 s.push_str(&format!(
410 "\nMinor/nit findings that were only listed, not filed: {}\n",
411 out.unfiled.join(", ")
412 ));
413 }
414 s
415}
416
417#[cfg(test)]
418mod tests {
419 use super::*;
420 use crate::config::Config;
421 use crate::queue::Queue;
422 use crate::run::ReviewRecord;
423 use crate::run::{Origin, ReviewRound, RunState};
424 use crate::verdict::{Finding, ReviewVote, Severity};
425 use std::path::PathBuf;
426
427 fn finding(id: &str, sev: Severity, file: &str, line: u32) -> Finding {
428 Finding {
429 id: id.to_owned(),
430 severity: sev,
431 file: Some(file.to_owned()),
432 line: Some(line),
433 title: format!("title {id}"),
434 detail: format!("detail of {id}"),
435 }
436 }
437
438 fn record(seat: usize, findings: Vec<Finding>, vote: ReviewVote) -> ReviewRecord {
439 ReviewRecord {
440 attempts: 0,
441 reviewer: seat,
442 agent: "alpha".to_owned(),
443 summary: String::new(),
444 findings,
445 vote: Some(vote),
446 failed: None,
447 duration_ms: 0,
448 }
449 }
450
451 fn round(reviews: Vec<ReviewRecord>) -> ReviewRound {
452 ReviewRound {
453 round: 3,
454 head: "deadbee".to_owned(),
455 verified_head: None,
456 verified_at: None,
457 reviews,
458 e2e: Vec::new(),
459 verify_retried: false,
460 e2e_deferred: false,
461 e2e_defer_reason: None,
462 fix: None,
463 blocking: 0,
464 answered: 2,
465 expected: 2,
466 clean: false,
467 progressed: false,
468 vote_split: false,
469 reconsideration: Vec::new(),
470 verdict: None,
471 }
472 }
473
474 fn merged(reviews: Vec<ReviewRecord>) -> RunState {
475 let mut s = RunState::new(
476 PathBuf::from("/repo"),
477 "main".to_owned(),
478 "abc1234".to_owned(),
479 "do it".to_owned(),
480 Config::default(),
481 );
482 s.reviews.push(round(reviews));
483 s
484 }
485
486 fn two_seats() -> Vec<ReviewRecord> {
487 vec![
488 record(
489 1,
490 vec![
491 finding("R3-1-1", Severity::Major, "src/waiter.rs", 211),
492 finding("R3-1-2", Severity::Minor, "src/a.rs", 1),
493 ],
494 ReviewVote::Reject,
495 ),
496 record(
497 2,
498 vec![
499 finding("R3-2-1", Severity::Major, "src/waiter.rs", 213),
500 finding("R3-2-2", Severity::Nit, "src/b.rs", 9),
501 ],
502 ReviewVote::Approve,
503 ),
504 ]
505 }
506
507 #[test]
508 fn one_task_per_distinct_defect_with_the_findings_in_full() {
509 let dir = tempfile::tempdir().unwrap();
510 let q = Queue::at(dir.path().join("queue"));
511 let mut s = merged(two_seats());
512 let out = file(&mut s, "https://example.invalid/o/r/pull/9", &q).unwrap();
513 assert_eq!(out.filed.len(), 2, "{out:?}");
516 assert_eq!(out.unfiled, vec!["R3-2-2".to_owned()]);
517 let tasks = q.list();
518 let t = tasks
519 .iter()
520 .find(|t| t.followup.as_ref().unwrap().findings.len() == 2)
521 .unwrap();
522 assert!(t.solo);
523 assert_eq!(t.followup.as_ref().unwrap().generation, 1);
524 assert_eq!(
525 t.source,
526 Source::Agent {
527 run: s.id.clone(),
528 node: NODE.to_owned()
529 }
530 );
531 for needle in [
532 "R3-1-1",
533 "R3-2-1",
534 "src/waiter.rs:211",
535 "detail of R3-2-1",
536 "pull/9",
537 &s.id,
538 "current main",
539 ] {
540 assert!(
541 t.instruction.contains(needle),
542 "{needle}: {}",
543 t.instruction
544 );
545 }
546 assert!(
547 s.events
548 .iter()
549 .any(|e| e.message.contains("filed 2 follow-up"))
550 );
551 assert_eq!(s.followups.len(), 2);
552 }
553
554 #[test]
555 fn filing_twice_does_not_duplicate() {
556 let dir = tempfile::tempdir().unwrap();
557 let q = Queue::at(dir.path().join("queue"));
558 let mut s = merged(two_seats());
559 file(&mut s, "u", &q).unwrap();
560 let again = file(&mut s, "u", &q).unwrap();
561 assert!(again.filed.is_empty());
562 assert_eq!(q.list().len(), 2);
563 s.followups.clear();
565 let third = file(&mut s, "u", &q).unwrap();
566 assert!(third.filed.is_empty());
567 assert_eq!(q.list().len(), 2);
568 assert_eq!(s.followups.len(), 2, "records are restored from the queue");
569 }
570
571 #[test]
572 fn a_clean_merge_files_nothing() {
573 let dir = tempfile::tempdir().unwrap();
574 let q = Queue::at(dir.path().join("queue"));
575 let mut s = merged(vec![record(
576 1,
577 vec![finding("R1-1-1", Severity::Minor, "a.rs", 1)],
578 ReviewVote::Approve,
579 )]);
580 let out = file(&mut s, "u", &q).unwrap();
581 assert!(out.filed.is_empty());
582 assert!(q.list().is_empty());
583 assert!(s.followups.is_empty());
584 }
585
586 #[test]
587 fn a_failing_queue_is_an_error_event_not_a_status_change() {
588 let dir = tempfile::tempdir().unwrap();
589 let root = dir.path().join("queue");
590 std::fs::write(&root, "not a directory").unwrap();
591 let q = Queue::at(root);
592 let mut s = merged(two_seats());
593 s.status = crate::run::RunStatus::Merged;
594 let _ = file(&mut s, "u", &q);
595 assert_eq!(s.status, crate::run::RunStatus::Merged);
596 assert!(
597 s.events
598 .iter()
599 .any(|e| e.node == NODE && e.message.contains("stopped"))
600 );
601 assert!(s.followups.is_empty());
602 }
603
604 #[test]
605 fn the_generation_cap_stops_the_chain() {
606 let dir = tempfile::tempdir().unwrap();
607 let q = Queue::at(dir.path().join("queue"));
608 let mut parent = Task::new(
609 "p".to_owned(),
610 "i".to_owned(),
611 PathBuf::from("/repo"),
612 Source::Human,
613 );
614 parent.followup = Some(FollowUp {
615 run: "r0".to_owned(),
616 origin_task: None,
617 pr: "u".to_owned(),
618 findings: vec!["R1-1-1".to_owned()],
619 generation: MAX_FOLLOWUP_GENERATION,
620 });
621 q.put(&mut parent).unwrap();
622 let mut s = merged(two_seats());
623 s.origin = Some(Origin {
624 by: crate::run::StartedBy::Operator,
625 task: Some(parent.id.clone()),
626 });
627 s.origin = Some(Origin {
628 by: crate::run::StartedBy::Operator,
629 task: Some("gone".to_owned()),
630 });
631 s.followup_generation = Some(MAX_FOLLOWUP_GENERATION);
632 let out = file(&mut s, "u", &q).unwrap();
633 assert!(out.filed.is_empty());
634 assert!(!out.capped.is_empty());
635 assert_eq!(q.list().len(), 1);
636 assert!(
637 s.events
638 .iter()
639 .any(|e| e.message.contains("generation cap"))
640 );
641 }
642
643 #[test]
644 fn a_deeper_task_files_one_generation_further() {
645 let dir = tempfile::tempdir().unwrap();
646 let q = Queue::at(dir.path().join("queue"));
647 let mut parent = Task::new(
648 "p".to_owned(),
649 "i".to_owned(),
650 PathBuf::from("/repo"),
651 Source::Human,
652 );
653 q.put(&mut parent).unwrap();
654 let mut s = merged(two_seats());
655 s.origin = Some(Origin {
656 by: crate::run::StartedBy::Operator,
657 task: Some(parent.id.clone()),
658 });
659 file(&mut s, "u", &q).unwrap();
660 let t = q.list().into_iter().find(|t| t.followup.is_some()).unwrap();
661 assert_eq!(t.followup.unwrap().origin_task, Some(parent.id));
662 }
663
664 const PR: &str = "https://github.com/o/r/pull/473";
665
666 fn manual(text: &str) -> Task {
667 Task::new(
668 "manual".to_owned(),
669 text.to_owned(),
670 PathBuf::from("/repo"),
671 Source::Human,
672 )
673 }
674
675 fn filed_with(text: &str) -> (Outcome, RunState, Queue, String, tempfile::TempDir) {
677 let dir = tempfile::tempdir().unwrap();
678 let q = Queue::at(dir.path().join("queue"));
679 let mut t = manual(text);
680 t.status = crate::queue::TaskStatus::Done;
681 q.put(&mut t).unwrap();
682 let mut s = merged(two_seats());
683 let out = file(&mut s, PR, &q).unwrap();
684 (out, s, q, t.id, dir)
685 }
686
687 #[test]
688 fn a_task_naming_the_pr_url_and_ids_covers_the_group() {
689 let (out, s, _q, id, _d) = filed_with(&format!("Fix R3-1-1 and R3-2-1 from {PR}"));
690 assert_eq!(out.filed.len(), 1, "{out:?}");
692 assert_eq!(
693 out.covered,
694 vec![
695 ("R3-1-1".to_owned(), id.clone()),
696 ("R3-2-1".to_owned(), id.clone())
697 ]
698 );
699 assert!(out.capped.is_empty());
700 assert!(
701 s.events
702 .iter()
703 .any(|e| e.message.contains("already covered"))
704 );
705 let body = comment_body(&s, &out);
706 assert!(body.contains(&format!("R3-1-1: already covered by task `{id}`")));
707 }
708
709 #[test]
710 fn the_hash_form_covers_but_a_prefix_number_does_not() {
711 let (out, ..) = filed_with("R3-1-1 R3-2-1 fixed in PR #473");
712 assert_eq!(out.covered.len(), 2);
713 let (out, ..) = filed_with("R3-1-1 R3-2-1 fixed in #47");
714 assert!(out.covered.is_empty());
715 assert_eq!(out.filed.len(), 2);
716 let (out, ..) = filed_with("R3-1-1 R3-2-1 fixed in #4731");
717 assert!(out.covered.is_empty());
718 }
719
720 #[test]
721 fn an_id_without_the_pr_does_not_cover() {
722 let (out, ..) = filed_with("R3-1-1 and R3-2-1 are bad");
723 assert!(out.covered.is_empty());
724 assert_eq!(out.filed.len(), 2);
725 }
726
727 #[test]
728 fn an_id_prefix_does_not_cover() {
729 let (out, ..) = filed_with(&format!("R3-1-10 R3-2-10 in {PR}"));
730 assert!(out.covered.is_empty());
731 }
732
733 #[test]
734 fn partial_coverage_still_files() {
735 let (out, ..) = filed_with(&format!("R3-1-1 in {PR}"));
736 assert!(out.covered.is_empty());
737 assert_eq!(out.filed.len(), 2);
738 }
739
740 #[test]
741 fn another_followups_findings_cover() {
742 let dir = tempfile::tempdir().unwrap();
743 let q = Queue::at(dir.path().join("queue"));
744 let mut s = merged(two_seats());
745 let mut other = manual("x");
746 other.followup = Some(FollowUp {
747 run: s.id.clone(),
748 origin_task: None,
749 pr: PR.to_owned(),
750 findings: vec!["R3-1-1".to_owned(), "R3-2-1".to_owned()],
751 generation: 1,
752 });
753 q.put(&mut other).unwrap();
754 let out = file(&mut s, PR, &q).unwrap();
755 assert_eq!(out.covered.len(), 2, "{out:?}");
756 assert_eq!(out.filed.len(), 1);
757 }
758
759 #[test]
760 fn a_fully_covered_comment_lists_only_the_covered() {
761 let (out, mut s, ..) = filed_with(&format!("R3-1-1 R3-2-1 {PR}"));
762 s.followups.clear();
763 let body = comment_body(&s, &out);
764 assert!(body.contains("already covered by task"));
765 assert!(!body.contains("filed as follow-up"));
766 }
767
768 #[test]
769 fn an_id_next_to_japanese_prose_still_covers() {
770 let (out, ..) = filed_with("PR #473 の R3-1-1を修正、R3-2-1も対応");
771 assert_eq!(out.covered.len(), 2, "{out:?}");
772 }
773}