Skip to main content

spar/
followups.rs

1//! The local follow-up queue, and working it.
2//!
3//! `.spar/followups.md` is what `followups = "local"` writes instead of filing
4//! on the tracker. Until now nothing read it back, so a review's out of scope
5//! findings accumulated in a file with no way to act on them. `spar followup`
6//! reads it, asks one agent whether each entry is still true of the code as it
7//! is now, files the survivors, and hands them to the same pipeline `spar run`
8//! uses, which means both agents still triage each one before anything is
9//! implemented.
10//!
11//! Most of the care here is in the parser, for one reason. An entry is written
12//! as `## <title>`, and `review::issue_report` writes that entry's own sections
13//! as `## Problem`, `## Reproduction`, `## Impact` and `## Expected behavior`,
14//! at the same heading level. A real file had 25 `##` lines and 5 entries, so a
15//! naive split would file twenty issues, four of them titled "Impact".
16
17use std::collections::BTreeMap;
18use std::collections::BTreeSet;
19use std::ops::Range;
20use std::path::Path;
21use std::sync::LazyLock;
22
23use regex::Regex;
24
25use crate::agent::Agent;
26use crate::config::{Call, Config, Followups};
27use crate::error::Result;
28use crate::model::{Finding, ScreenResponse, ScreenVerdict, Screened};
29use crate::repo::{Repo, FOLLOWUP_MARKER};
30use crate::{log, logwarn, schema, spar_err};
31
32static BLANK_RUN: LazyLock<Regex> =
33    LazyLock::new(|| Regex::new(r"\n{3,}").expect("blank run pattern"));
34
35// ---------------------------------------------------------------------------
36// The file format
37// ---------------------------------------------------------------------------
38
39/// One entry as it sits in the note file.
40#[derive(Debug, Clone, PartialEq, Eq)]
41pub struct Entry {
42    /// The `## ` heading text, marker excluded.
43    pub title: String,
44    /// Everything under the heading up to the next entry, trimmed. Keeps the
45    /// report sections and the `Found while working on #N.` line.
46    pub body: String,
47    /// Byte range in the source covering the whole entry, marker included.
48    ///
49    /// The rewrite removes spans out of the original text rather than
50    /// re-rendering what stays, which is what makes a hand edited file safe.
51    pub span: Range<usize>,
52}
53
54/// The headings `review::issue_report` writes inside an entry.
55///
56/// Taken from `Finding::report_sections` rather than written out again. A fifth
57/// section added there and forgotten here would split every entry that used it
58/// in two, and the second half would be filed as its own issue.
59fn report_headings() -> Vec<&'static str> {
60    Finding {
61        problem: Some("x".into()),
62        reproduction: Some("x".into()),
63        impact: Some("x".into()),
64        expected: Some("x".into()),
65        ..Finding::default()
66    }
67    .report_sections()
68    .into_iter()
69    .map(|(heading, _)| heading)
70    .collect()
71}
72
73/// Whether a `## ` heading names a section of a bug report rather than an
74/// entry's title.
75///
76/// Compares the whole heading, so `## Reproduction steps are missing` is a
77/// title and not a section. The extra names are what a model writing
78/// `new_issue_body` free hand produces, which the schema does not constrain.
79fn is_section_heading(text: &str) -> bool {
80    let got = text.trim().trim_end_matches(':').trim().to_lowercase();
81    report_headings().iter().any(|h| h.to_lowercase() == got)
82        || matches!(
83            got.as_str(),
84            "expected behaviour"
85                | "expected"
86                | "actual result"
87                | "actual results"
88                | "actual behavior"
89                | "actual behaviour"
90                | "steps to reproduce"
91                | "summary"
92        )
93}
94
95/// Every line with the byte offset it starts at.
96fn lines_with_offsets(text: &str) -> impl Iterator<Item = (usize, &str)> {
97    let mut at = 0usize;
98    text.split_inclusive('\n').map(move |line| {
99        let start = at;
100        at += line.len();
101        (start, line.trim_end_matches(['\n', '\r']))
102    })
103}
104
105/// Split a note file into entries.
106///
107/// Two rules, in order:
108///
109/// 1. A `<!-- spar:followup -->` line always starts an entry. spar writes one
110///    above every entry it appends, so a file written from this release on is
111///    read exactly rather than guessed at, and the first `## ` line after the
112///    marker is that entry's title rather than a boundary.
113/// 2. Otherwise a `## ` line starts an entry unless it names one of the
114///    sections a bug report is written in, or there is nothing open for it to
115///    be a section of.
116///
117/// Rule 2 is generous about what counts as a section, on purpose, because the
118/// two ways to be wrong are not the same size. Mistaking a section for a title
119/// splits one follow-up into four and files an issue called "Impact" carrying a
120/// fragment: visible, embarrassing, and on somebody else's tracker. Mistaking a
121/// title for a section merges two follow-ups into one issue that carries both:
122/// fat, and recoverable by reading it. Nothing is lost.
123///
124/// A heading inside a fenced code block is never a boundary. A body is free to
125/// carry a snippet, `style::issue_body` deliberately protects fenced blocks
126/// from the length budget, and a snippet containing a `## ` line would
127/// otherwise split the entry that quotes it.
128///
129/// Rejected as a third signal: "an entry follows a `Found while working on #N.`
130/// line". Provenance is not guaranteed, a hand written entry has none, and a
131/// parser that leans on prose is one an edit breaks.
132pub fn parse(text: &str) -> Vec<Entry> {
133    // Entry start, and where its `## ` title line starts if it has one.
134    let mut opens: Vec<(usize, Option<usize>)> = Vec::new();
135    let mut open = false;
136    let mut awaiting_title = false;
137    let mut fenced = false;
138
139    for (offset, line) in lines_with_offsets(text) {
140        let trimmed = line.trim_start();
141        if trimmed.starts_with("```") || trimmed.starts_with("~~~") {
142            fenced = !fenced;
143            continue;
144        }
145        if fenced {
146            continue;
147        }
148        if trimmed.starts_with(FOLLOWUP_MARKER) {
149            opens.push((offset, None));
150            open = true;
151            awaiting_title = true;
152            continue;
153        }
154        let Some(heading) = trimmed.strip_prefix("## ") else {
155            continue;
156        };
157        if awaiting_title {
158            if let Some(last) = opens.last_mut() {
159                last.1 = Some(offset);
160            }
161            awaiting_title = false;
162            continue;
163        }
164        // Nothing is open, so this cannot be a section of anything: a file
165        // whose first heading is `## Problem` holds one entry called Problem.
166        if open && is_section_heading(heading) {
167            continue;
168        }
169        opens.push((offset, Some(offset)));
170        open = true;
171    }
172
173    let mut out = Vec::with_capacity(opens.len());
174    for (i, (start, title_at)) in opens.iter().enumerate() {
175        let end = opens.get(i + 1).map(|(s, _)| *s).unwrap_or(text.len());
176        let (title, body_from) = match title_at {
177            Some(at) => {
178                let line_end = text[*at..end].find('\n').map(|n| at + n + 1).unwrap_or(end);
179                let heading = text[*at..line_end]
180                    .trim()
181                    .trim_start_matches("## ")
182                    .trim()
183                    .to_string();
184                (heading, line_end)
185            }
186            // A marker with no heading under it. Keep the entry rather than
187            // dropping it: the body is still somebody's finding.
188            None => (String::new(), *start),
189        };
190        out.push(Entry {
191            title,
192            body: text[body_from..end].trim().to_string(),
193            span: *start..end,
194        });
195    }
196    out
197}
198
199/// The text with these entries removed, blank lines closed up at the seams.
200///
201/// Removal is by byte span out of the text as it was parsed, never by
202/// re-rendering what stays. Anything in the file spar did not write, a preamble,
203/// an entry in a shape spar does not recognise, an edit somebody made to a body,
204/// comes back out exactly as it went in.
205///
206/// Spans are sorted and merged first, so a caller may pass them in any order and
207/// may pass the same one twice. That is what makes it correct to rewrite the
208/// file once per entry against the *original* text: splicing a splice would
209/// shift every later offset.
210pub fn without(text: &str, removed: &[Entry]) -> String {
211    let mut spans: Vec<Range<usize>> = removed.iter().map(|e| e.span.clone()).collect();
212    spans.sort_by_key(|s| s.start);
213
214    let mut out = String::with_capacity(text.len());
215    let mut cursor = 0usize;
216    for span in spans {
217        // Already covered by an earlier span. Skipping rather than slicing
218        // backwards, because a panic here would take the queue with it.
219        if span.start < cursor {
220            cursor = cursor.max(span.end);
221            continue;
222        }
223        out.push_str(&text[cursor..span.start]);
224        cursor = span.end;
225    }
226    out.push_str(&text[cursor..]);
227
228    let joined = BLANK_RUN.replace_all(out.trim_end(), "\n\n").to_string();
229    if joined.trim().is_empty() {
230        String::new()
231    } else {
232        format!("{joined}\n")
233    }
234}
235
236// ---------------------------------------------------------------------------
237// Screening
238// ---------------------------------------------------------------------------
239
240const SCREEN_PROMPT: &str = "\
241Below are follow-ups recorded against this repository while other work was going
242on. Each was a real finding when it was written. Time has passed and the code has
243moved: some are already fixed, some describe behaviour that no longer exists, and
244some were never worth the interruption.
245
246Read the code in your working directory before judging each one. Do not modify
247anything. The current checkout is what \"now\" means. Judge against it, not
248against what the entry says the code used to do.
249
250For each entry decide:
251- verdict: still_relevant, already_fixed, not_worth_it, or duplicate.
252  - still_relevant: the defect is still there. It becomes a GitHub issue.
253  - already_fixed: go and look. Name the function or the change that fixed it,
254    so somebody reading this can check you.
255  - not_worth_it: real, still there, and not worth a maintainer's queue.
256  - duplicate: something else already covers it. An open issue goes in
257    duplicate_of_issue, another entry in this list goes in duplicate_of_entry
258    by its number here. They are separate fields because an entry is not an
259    issue: an entry you point at may itself be dropped, and this one is then
260    kept rather than lost with it.
261- reason: one sentence. For anything but still_relevant this is the only record
262  of why the entry was dropped, so give the reason rather than the verdict
263  restated.
264- title: the entry's title, which becomes the issue title. Copy it across unless
265  it is wrong or says nothing.
266
267Say still_relevant when you are unsure. What survives is triaged by both agents
268afterwards and can still be declined there. What you drop here is dropped.
269
270Entries:
271";
272
273/// The queue as the prompt carries it, and what would not fit.
274struct Rendered {
275    text: String,
276    /// Left in the file for a later run, because the queue did not fit in one
277    /// prompt.
278    deferred: usize,
279}
280
281/// Render the queue under one budget.
282///
283/// The same shape as `triage::render` and for the same reason, which is worth
284/// saying rather than sharing: whole entries wait rather than every entry losing
285/// its tail, because a verdict here *deletes* the entry, so judging one on part
286/// of what it says is worse than not having reached it yet.
287///
288/// Unlike an issue body there is no per entry cut at all. Every entry already
289/// went through `style::issue_body` on the way in, and unlike an issue the entry
290/// is all there is.
291fn render(entries: &[Entry], cfg: &Config) -> Rendered {
292    let mut parts: Vec<String> = Vec::new();
293    let mut total = 0usize;
294    let mut deferred = 0usize;
295
296    for (i, entry) in entries.iter().enumerate() {
297        if deferred > 0 {
298            deferred += 1;
299            continue;
300        }
301        let block = format!("{}. {}\n{}", i + 1, entry.title, entry.body);
302        let len = block.chars().count();
303        // The first entry goes in whatever its size. A queue of one that does
304        // not fit is a command that does nothing, forever.
305        if !parts.is_empty() && total + len > cfg.loop_cfg.max_triage_chars {
306            deferred += 1;
307            continue;
308        }
309        total += len;
310        parts.push(block);
311    }
312
313    Rendered {
314        text: parts.join("\n\n"),
315        deferred,
316    }
317}
318
319/// One agent's verdict on every entry that fits in one prompt.
320///
321/// One call for the whole queue rather than one per entry. A repo aware call per
322/// entry is the dominant cost, and `duplicate` is a judgement across entries as
323/// well as against the tracker: an agent shown one entry at a time cannot say
324/// that this is the same defect as the one above it.
325pub fn screen(
326    agent: &Agent,
327    cfg: &Config,
328    repo: &Repo,
329    entries: &[Entry],
330) -> Result<Vec<ScreenVerdict>> {
331    let rendered = render(entries, cfg);
332    if rendered.deferred > 0 {
333        logwarn!(
334            "the queue did not fit in one screening prompt, so {} entry(s) were left in the file \
335             for a later run",
336            rendered.deferred
337        );
338    }
339    let prompt = format!("{SCREEN_PROMPT}{}", rendered.text);
340    let effort = agent.effort(cfg, Call::Screen);
341    let checkout = repo.read_checkout("follow-up screen")?;
342    let answer: ScreenResponse =
343        agent.ask_json(&prompt, &schema::screen(), checkout.path(), &effort)?;
344    Ok(answer.entries)
345}
346
347// ---------------------------------------------------------------------------
348// Working the queue
349// ---------------------------------------------------------------------------
350
351/// Where a run of `spar followup` stops.
352#[derive(Debug, Clone, Copy, PartialEq, Eq)]
353pub enum Mode {
354    /// Print the verdicts and touch nothing.
355    ScreenOnly,
356    /// File the issues and stop, leaving them for a later `spar run`.
357    FileOnly,
358    /// File them and work them.
359    Work,
360}
361
362/// What was filed, and what was left behind.
363#[derive(Debug, Default)]
364pub struct Outcome {
365    /// Issues to work, in file order.
366    pub issues: Vec<i64>,
367    /// Left in the file: not reached, not screened, or could not be filed.
368    pub held: usize,
369}
370
371/// Read the queue, screen it, file what still holds, and take the filed entries
372/// out of the file.
373pub fn run(
374    agents: &[Agent],
375    cfg: &Config,
376    repo: &Repo,
377    path: &Path,
378    limit: usize,
379    mode: Mode,
380) -> Result<Outcome> {
381    let mut outcome = Outcome::default();
382
383    // Before any network call, so an empty queue is a purely local no-op.
384    let Ok(original) = std::fs::read_to_string(path) else {
385        log!("no follow-ups recorded in {}", path.display());
386        if repo.followups != Followups::Local {
387            log!(
388                "followups = \"{}\" is configured, so nothing is written to that file.",
389                repo.followups
390            );
391        }
392        return Ok(outcome);
393    };
394    if original.trim().is_empty() {
395        log!("{} is there and empty", path.display());
396        return Ok(outcome);
397    }
398
399    let entries = parse(&original);
400    if entries.is_empty() {
401        // A parser problem must not be reported as an empty queue.
402        log!(
403            "{} has no `## ` headings, so there is nothing to work. An entry is a `## Title` line \
404             and the text under it.",
405            path.display()
406        );
407        return Ok(outcome);
408    }
409
410    let taken: Vec<Entry> = entries.iter().take(limit).cloned().collect();
411    outcome.held += entries.len() - taken.len();
412    if outcome.held > 0 {
413        log!(
414            "{} follow-up(s) recorded, taking the first {limit}. Raise --limit for the rest.",
415            entries.len()
416        );
417    }
418
419    let agent = crate::agent::find(agents, &cfg.first_implementor)?;
420    // An `already_fixed` verdict is uninterpretable without knowing what was
421    // being judged.
422    log!(
423        "screening {} follow-up(s) with {} against {} at {}",
424        taken.len(),
425        agent.name(),
426        repo.git_try(&["rev-parse", "--abbrev-ref", "HEAD"]).trim(),
427        repo.git_try(&["rev-parse", "--short", "HEAD"]).trim(),
428    );
429
430    // A screen that did not happen is not a screen that found nothing: nothing
431    // is filed and the file is not touched.
432    let verdicts = screen(agent, cfg, repo, &taken)?;
433
434    if mode == Mode::ScreenOnly {
435        print_verdicts(&taken, &verdicts);
436        return Ok(outcome);
437    }
438
439    let mut disposed: Vec<Entry> = Vec::new();
440    // What each entry became, so a sibling that says it duplicates one can be
441    // answered after the batch rather than guessed at during it. An entry
442    // pointing at one that was itself dropped, or failed to file, is kept:
443    // archiving it as covered by an issue number that is really an entry
444    // number sends a reader to an unrelated issue and loses both copies.
445    let mut filed_as: BTreeMap<i64, i64> = BTreeMap::new();
446    let mut deferred: Vec<usize> = Vec::new();
447
448    for pass in 0..2 {
449        for (i, entry) in taken.iter().enumerate() {
450            let number = i as i64 + 1;
451            if pass == 0 && disposed.iter().any(|d| d.title == entry.title) {
452                continue;
453            }
454            if pass == 1 && !deferred.contains(&i) {
455                continue;
456            }
457            let Some(verdict) = verdicts.iter().find(|v| v.entry == number) else {
458                // Never read as "drop it". Filing something nobody looked at
459                // defeats the point of the screen; leaving it costs one line.
460                logwarn!(
461                    "no verdict for '{}', leaving it in the file",
462                    first_line(&entry.title)
463                );
464                outcome.held += 1;
465                continue;
466            };
467
468            // An entry that says it duplicates a sibling waits for the sibling to
469            // be dealt with, because what happened to that one decides this one.
470            if pass == 0
471                && verdict.verdict == Screened::Duplicate
472                && verdict.duplicate_of_entry.is_some()
473            {
474                deferred.push(i);
475                continue;
476            }
477            let covered_by = verdict
478                .duplicate_of_entry
479                .and_then(|target| filed_as.get(&target).copied())
480                .or(verdict.duplicate_of_issue);
481            if verdict.verdict == Screened::Duplicate
482                && verdict.duplicate_of_entry.is_some()
483                && covered_by.is_none()
484            {
485                logwarn!(
486                    "'{}' was called a duplicate of entry {}, which was not filed, so it stays in \
487                 the file",
488                    first_line(&entry.title),
489                    verdict.duplicate_of_entry.unwrap_or_default()
490                );
491                outcome.held += 1;
492                continue;
493            }
494
495            // "Duplicate of nothing in particular" is unfalsifiable, and
496            // `find_similar_issue` on the filing path is exactly the check for it.
497            let files = verdict.verdict == Screened::StillRelevant
498                || (verdict.verdict == Screened::Duplicate && !verdict.names_a_duplicate());
499
500            if files {
501                let retitled = !verdict.title.trim().is_empty()
502                    && !verdict
503                        .title
504                        .trim()
505                        .eq_ignore_ascii_case(entry.title.trim());
506                let title = if verdict.title.trim().is_empty() {
507                    entry.title.as_str()
508                } else {
509                    verdict.title.as_str()
510                };
511                match crate::review::file_as_issue(repo, title, &entry.body) {
512                    Ok(filed) => {
513                        log!("  {}", filed.describe(title));
514                        if let Some(n) = filed.number() {
515                            outcome.issues.push(n);
516                            filed_as.insert(number, n);
517                        }
518                        // Archived under the title it was written with, so the next
519                        // run that rediscovers the same defect in the original
520                        // wording finds it here rather than appending it again. The
521                        // new title goes in the verdict, and gets a heading of its
522                        // own, so either wording is recognised.
523                        repo.archive_followup(
524                            &entry.title,
525                            &entry.body,
526                            &format!(
527                                "Filed: {}{}",
528                                filed.note(),
529                                if retitled {
530                                    format!(", retitled to '{}'", title.trim())
531                                } else {
532                                    String::new()
533                                }
534                            ),
535                        );
536                        if retitled {
537                            repo.archive_followup(
538                                title,
539                                &format!("The same entry as '{}'.", entry.title.trim()),
540                                &format!("Filed: {}", filed.note()),
541                            );
542                        }
543                    }
544                    Err(e) => {
545                        logwarn!("could not file '{}': {e}", first_line(title));
546                        outcome.held += 1;
547                        continue;
548                    }
549                }
550            } else {
551                let why = dropped_note(verdict, covered_by);
552                log!("  dropped '{}': {why}", first_line(&entry.title));
553                repo.archive_followup(&entry.title, &entry.body, &format!("Dropped: {why}"));
554            }
555
556            disposed.push(entry.clone());
557            // Written after the entry is dealt with, never before. The window is
558            // one entry wide and it always falls on the side of filing twice rather
559            // than losing one: an entry filed and not yet removed is found again
560            // next run and matched to the issue that was just created, while an
561            // entry removed before it was filed is gone.
562            crate::repo::write_text_atomic(path, &without(&original, &disposed)).map_err(|e| {
563                spar_err!(
564                "{e}\n{} follow-up(s) were already dealt with. Remove them from {} by hand before \
565                 running this again, or they will be filed twice.",
566                disposed.len(),
567                path.display()
568            )
569            })?;
570        }
571    }
572
573    let filed = outcome.issues.len();
574    println!(
575        "\nfollowups: {} screened, {filed} filed{}",
576        taken.len(),
577        summarise(&taken, &verdicts)
578    );
579    if outcome.held > 0 {
580        println!("{} entry(s) left in {}", outcome.held, path.display());
581    }
582    if !disposed.is_empty() {
583        println!(
584            "what was dealt with is in {}",
585            repo.worked_followups_path().display()
586        );
587    }
588    Ok(outcome)
589}
590
591fn first_line(text: &str) -> String {
592    crate::style::clip(text.trim().lines().next().unwrap_or("").trim(), 80)
593}
594
595fn dropped_note(v: &ScreenVerdict, covered_by: Option<i64>) -> String {
596    let reason = v.reason.trim();
597    match (v.verdict, covered_by) {
598        (Screened::Duplicate, Some(n)) if reason.is_empty() => format!("#{n} already covers it"),
599        (Screened::Duplicate, Some(n)) => format!("#{n} already covers it. {reason}"),
600        (_, _) if reason.is_empty() => v.verdict.to_string(),
601        _ => format!("{}. {reason}", v.verdict),
602    }
603}
604
605/// The tail of the summary line, naming each verdict that dropped something.
606fn summarise(taken: &[Entry], verdicts: &[ScreenVerdict]) -> String {
607    let mut counts: Vec<(Screened, usize)> = Vec::new();
608    for v in verdicts {
609        if v.entry < 1 || v.entry as usize > taken.len() {
610            continue;
611        }
612        match counts.iter_mut().find(|(k, _)| *k == v.verdict) {
613            Some((_, n)) => *n += 1,
614            None => counts.push((v.verdict, 1)),
615        }
616    }
617    counts.retain(|(k, _)| *k != Screened::StillRelevant);
618    if counts.is_empty() {
619        return String::new();
620    }
621    let listed: Vec<String> = counts
622        .iter()
623        .map(|(k, n)| format!("{n} {}", k.as_str().replace('_', " ")))
624        .collect();
625    format!(", {}", listed.join(", "))
626}
627
628/// What `--screen-only` prints. `println!`, not `log!`: it is the whole output
629/// of the command, and an entry dropped with no visible record of why is
630/// exactly the failure the archive exists for.
631fn print_verdicts(taken: &[Entry], verdicts: &[ScreenVerdict]) {
632    println!();
633    for (i, entry) in taken.iter().enumerate() {
634        let number = i as i64 + 1;
635        match verdicts.iter().find(|v| v.entry == number) {
636            Some(v) => println!(
637                "  {:<14} {}\n                 {}",
638                v.verdict.as_str(),
639                first_line(&entry.title),
640                v.reason.trim()
641            ),
642            None => println!("  {:<14} {}", "no verdict", first_line(&entry.title)),
643        }
644    }
645    let filed = verdicts
646        .iter()
647        .filter(|v| v.verdict == Screened::StillRelevant)
648        .count();
649    println!(
650        "\n{filed} of {} would be filed. Nothing was written.",
651        taken.len()
652    );
653}
654
655/// The issue numbers a set of outcomes produced, deduplicated and in order.
656pub fn wave(outcome: &Outcome) -> Vec<i64> {
657    outcome
658        .issues
659        .iter()
660        .copied()
661        .collect::<BTreeSet<_>>()
662        .into_iter()
663        .collect()
664}
665
666#[cfg(test)]
667mod tests {
668    use super::*;
669
670    /// Two entries in the shape a real file has: a title, a lead paragraph,
671    /// the four report sections, and the provenance line.
672    const REAL: &str = "\
673## Backend headers never drive commitment CPFP retries
674
675The production ChainWatcher advances monitors and emits block.
676
677## Problem
678
679Configured chain backends route accepted headers through handleNewBlock.
680
681## Reproduction
682
6831. Configure a node with a watcher backend.
6842. Deliver height 101.
685
686## Impact
687
688Nodes do not retry stuck commitment packages on new blocks.
689
690## Expected behavior
691
692Run the pass exactly once for each accepted backend header.
693
694Found while working on #589.
695
696## Overlapping scans can move a recorded spend height backward
697
698## Problem
699
700checkOutputSpend applies its result with no arbitration against a later scan.
701
702## Impact
703
704A stale verdict can overwrite a newer one.
705
706Found while working on #590.
707";
708
709    /// The whole reason this module exists. A follow-up's own sections are
710    /// written at the same heading level as its title, so a naive split on
711    /// `## ` files six issues here, four of them called things like "Impact".
712    #[test]
713    fn an_entry_and_its_sections_are_not_confused_for_each_other() {
714        let entries = parse(REAL);
715        assert_eq!(
716            2,
717            entries.len(),
718            "{:#?}",
719            entries.iter().map(|e| &e.title).collect::<Vec<_>>()
720        );
721        assert!(entries[0].title.starts_with("Backend headers"));
722        assert!(entries[1].title.starts_with("Overlapping scans"));
723        // The sections stay with the entry they belong to.
724        assert!(
725            entries[0].body.contains("## Reproduction"),
726            "{}",
727            entries[0].body
728        );
729        assert!(entries[0].body.contains("Found while working on #589."));
730    }
731
732    /// The heading heuristic alone cannot see a title that collides with a
733    /// section name. The marker is what fixes that going forward, and spar
734    /// writes one above every entry it appends.
735    #[test]
736    fn a_marker_makes_the_boundary_exact() {
737        let text = format!(
738            "{FOLLOWUP_MARKER}\n## Impact\n\nThe first one.\n\n\
739             {FOLLOWUP_MARKER}\n## Problem\n\nThe second one.\n"
740        );
741        let entries = parse(&text);
742        assert_eq!(2, entries.len());
743        assert_eq!("Impact", entries[0].title);
744        assert_eq!("Problem", entries[1].title);
745    }
746
747    /// Keeping the queue by hand is a documented use, and it must not require
748    /// knowing spar's marker.
749    #[test]
750    fn a_hand_written_file_with_no_markers_still_parses() {
751        let text =
752            "## One thing\n\nprose\n\n## Another thing\n\nmore prose\n\n## A third\n\nyet more\n";
753        let entries = parse(text);
754        assert_eq!(3, entries.len());
755        assert_eq!("Another thing", entries[1].title);
756    }
757
758    /// A body is free to carry a snippet, and `style::issue_body` deliberately
759    /// protects fenced blocks from the length budget, so a snippet containing a
760    /// `## ` line would otherwise split the entry that quotes it.
761    #[test]
762    fn a_heading_inside_a_fenced_block_does_not_start_an_entry() {
763        let text = "## Real title\n\n```md\n## Problem\n## Not a title either\n```\n\nprose\n";
764        let entries = parse(text);
765        assert_eq!(1, entries.len(), "{:?}", entries);
766        assert_eq!("Real title", entries[0].title);
767    }
768
769    /// `is_section_heading` compares the whole heading. A prefix match would
770    /// swallow an entry whose title happens to open with a section word.
771    #[test]
772    fn an_entry_whose_title_opens_with_a_section_word_is_still_a_title() {
773        let text =
774            "## First\n\nprose\n\n## Reproduction steps are missing from the docs\n\nprose\n";
775        assert_eq!(2, parse(text).len());
776    }
777
778    /// A fifth section added to a bug report and forgotten here would split
779    /// every entry that used it, and the second half would be filed on its own.
780    #[test]
781    fn the_section_list_covers_every_heading_a_report_writes() {
782        for heading in report_headings() {
783            assert!(
784                is_section_heading(heading),
785                "`## {heading}` would be read as the start of a new follow-up"
786            );
787        }
788    }
789
790    /// A rewrite that re-renders what stays silently deletes whatever a person
791    /// added: a note at the top, an edit inside a body, a trailing reminder.
792    #[test]
793    fn text_the_parser_does_not_own_survives_a_rewrite() {
794        let text = "A note I keep at the top.\n\n\
795                    ## One\n\nfirst\n\n\
796                    ## Two\n\nsecond\n\n\
797                    ## Three\n\nthird\n";
798        let entries = parse(text);
799        assert_eq!(3, entries.len());
800        let out = without(text, &[entries[1].clone()]);
801        assert!(out.starts_with("A note I keep at the top."), "{out}");
802        assert!(out.contains("## One"), "{out}");
803        assert!(!out.contains("## Two"), "{out}");
804        assert!(out.contains("## Three"), "{out}");
805        assert!(out.contains("third"), "{out}");
806    }
807
808    /// The guard against splicing a splice. Spans index the text as it was
809    /// parsed, so removing one entry would shift every later offset, and the
810    /// run rewrites the file once per entry.
811    #[test]
812    fn removing_entries_one_at_a_time_matches_removing_them_at_once() {
813        let entries = parse(REAL);
814        let all_at_once = without(REAL, &entries);
815
816        let mut done = Vec::new();
817        let mut last = String::new();
818        for entry in &entries {
819            done.push(entry.clone());
820            last = without(REAL, &done);
821        }
822        assert_eq!(all_at_once, last);
823        assert!(last.is_empty(), "{last:?}");
824    }
825
826    /// A caller that recorded the same entry twice, or recorded them out of
827    /// order, must not corrupt the file or panic on a backwards slice.
828    #[test]
829    fn without_tolerates_a_repeated_or_unordered_span() {
830        let entries = parse(REAL);
831        let once = without(REAL, &[entries[0].clone()]);
832        let twice = without(REAL, &[entries[0].clone(), entries[0].clone()]);
833        assert_eq!(once, twice);
834
835        let forwards = without(REAL, &[entries[0].clone(), entries[1].clone()]);
836        let backwards = without(REAL, &[entries[1].clone(), entries[0].clone()]);
837        assert_eq!(forwards, backwards);
838    }
839
840    /// An emptied queue is an empty file, not a pile of blank lines that reads
841    /// as content to the next thing that opens it.
842    #[test]
843    fn removing_every_entry_leaves_an_empty_file() {
844        let entries = parse(REAL);
845        assert_eq!("", without(REAL, &entries));
846    }
847
848    /// The link back to the work that found a defect cannot be re-derived, so
849    /// the body has to carry it through unchanged.
850    #[test]
851    fn an_entry_keeps_the_provenance_it_was_written_with() {
852        let entries = parse(REAL);
853        assert!(entries[1].body.ends_with("Found while working on #590."));
854    }
855
856    /// A file edited on Windows must parse the same as one edited anywhere else.
857    #[test]
858    fn crlf_line_endings_parse_the_same_as_lf() {
859        let lf = "## One\n\nfirst\n\n## Two\n\nsecond\n";
860        let crlf = lf.replace('\n', "\r\n");
861        let a = parse(lf);
862        let b = parse(&crlf);
863        assert_eq!(a.len(), b.len());
864        assert_eq!(a[1].title, b[1].title);
865    }
866
867    /// A file whose first heading is a section name holds one entry called
868    /// that, rather than nothing at all.
869    #[test]
870    fn a_file_that_opens_with_a_section_name_still_holds_an_entry() {
871        let entries = parse("## Problem\n\nsomething is wrong\n");
872        assert_eq!(1, entries.len());
873        assert_eq!("Problem", entries[0].title);
874    }
875
876    fn verdict(entry: i64, v: Screened, dup: Option<i64>) -> ScreenVerdict {
877        ScreenVerdict {
878            entry,
879            verdict: v,
880            title: String::new(),
881            reason: "because".into(),
882            duplicate_of_issue: dup,
883            duplicate_of_entry: None,
884        }
885    }
886
887    fn duplicate_of_entry(entry: i64, target: i64) -> ScreenVerdict {
888        ScreenVerdict {
889            entry,
890            verdict: Screened::Duplicate,
891            title: String::new(),
892            reason: "same as the one above".into(),
893            duplicate_of_issue: None,
894            duplicate_of_entry: Some(target),
895        }
896    }
897
898    /// "Duplicate of nothing in particular" is unfalsifiable, and the search on
899    /// the filing path is exactly the check for it. Dropping the entry on that
900    /// verdict would delete a real defect on no evidence.
901    #[test]
902    fn a_duplicate_verdict_with_nothing_to_point_at_would_still_be_filed() {
903        let with_number = verdict(1, Screened::Duplicate, Some(412));
904        let without_number = verdict(1, Screened::Duplicate, None);
905        let files = |v: &ScreenVerdict| {
906            v.verdict == Screened::StillRelevant
907                || (v.verdict == Screened::Duplicate && !v.names_a_duplicate())
908        };
909        assert!(!files(&with_number));
910        assert!(files(&without_number));
911    }
912
913    /// A short answer must never read as "drop the rest". The entries the
914    /// screen did not rule on stay in the file.
915    #[test]
916    fn an_entry_with_no_verdict_is_not_disposed_of() {
917        let entries = parse(REAL);
918        let verdicts = [verdict(1, Screened::AlreadyFixed, None)];
919        let unruled: Vec<usize> = (1..=entries.len())
920            .filter(|n| !verdicts.iter().any(|v| v.entry == *n as i64))
921            .collect();
922        assert_eq!(vec![2], unruled);
923    }
924
925    /// An out of range index used as a slice index panics, and a verdict for an
926    /// entry that does not exist must not shift the ones that do.
927    #[test]
928    fn a_verdict_naming_an_entry_that_does_not_exist_is_ignored() {
929        let entries = parse(REAL);
930        let verdicts = [verdict(9, Screened::AlreadyFixed, None)];
931        assert_eq!("", summarise(&entries, &verdicts));
932    }
933
934    /// The summary is the only place a dropped entry is accounted for, and it
935    /// has to name what happened rather than only how many.
936    #[test]
937    fn the_summary_names_each_verdict_that_dropped_something() {
938        let entries = parse(REAL);
939        let verdicts = vec![
940            verdict(1, Screened::AlreadyFixed, None),
941            verdict(2, Screened::StillRelevant, None),
942        ];
943        let out = summarise(&entries, &verdicts);
944        assert!(out.contains("1 already fixed"), "{out}");
945        assert!(!out.contains("still relevant"), "{out}");
946    }
947
948    /// The reason is the only record of why an entry left the queue, so it has
949    /// to survive into the log line and the archive.
950    #[test]
951    fn a_dropped_entry_carries_its_reason_and_the_issue_it_duplicates() {
952        let v = verdict(1, Screened::Duplicate, Some(412));
953        let note = dropped_note(&v, v.duplicate_of_issue);
954        assert!(note.contains("#412"), "{note}");
955        assert!(note.contains("because"), "{note}");
956    }
957
958    /// An entry number is not an issue number, and one field could not say
959    /// which it was.
960    ///
961    /// Entry 4 duplicating entry 2 was archived as covered by issue #2, an
962    /// unrelated issue, and if entry 2 was then ruled already fixed or failed
963    /// to file, neither copy was filed at all.
964    #[test]
965    fn a_duplicate_of_a_sibling_is_kept_apart_from_a_duplicate_of_an_issue() {
966        let sibling = duplicate_of_entry(4, 2);
967        assert!(sibling.names_a_duplicate(), "it points at something");
968        assert_eq!(None, sibling.duplicate_of_issue);
969
970        // Unresolved, it names nothing to send a reader to.
971        let unresolved = dropped_note(&sibling, None);
972        assert!(!unresolved.contains("#2"), "{unresolved}");
973
974        // Resolved after the batch, it names the issue that entry became.
975        let resolved = dropped_note(&sibling, Some(512));
976        assert!(resolved.contains("#512"), "{resolved}");
977    }
978}
979
980#[cfg(test)]
981mod real_file {
982    use super::*;
983
984    /// The queue one real run left on a real repository, captured verbatim.
985    /// Five follow-ups, twenty-five `## ` lines. This is the file the heuristic
986    /// is measured against rather than guessed at.
987    const CORPUS: &str = include_str!("../tests/fixtures/local_followups.md");
988
989    #[test]
990    fn the_real_queue_parses_as_five_follow_ups_not_twenty_five() {
991        let entries = parse(CORPUS);
992        assert_eq!(
993            5,
994            entries.len(),
995            "{:#?}",
996            entries.iter().map(|e| e.title.as_str()).collect::<Vec<_>>()
997        );
998        for entry in &entries {
999            assert!(
1000                !is_section_heading(&entry.title),
1001                "a section was filed as a follow-up: {}",
1002                entry.title
1003            );
1004            assert!(!entry.body.trim().is_empty(), "{} has no body", entry.title);
1005        }
1006    }
1007
1008    /// Every entry in that file ends with the line that says which issue it
1009    /// came out of, and an issue filed from it is the only place that link
1010    /// survives.
1011    #[test]
1012    fn every_entry_in_the_real_queue_keeps_its_provenance() {
1013        for entry in parse(CORPUS) {
1014            assert!(
1015                entry.body.contains("Found while working on #"),
1016                "{} lost its provenance",
1017                entry.title
1018            );
1019        }
1020    }
1021
1022    /// Removing them one at a time, always against the original text, has to
1023    /// end where removing them all at once does, and has to end empty.
1024    #[test]
1025    fn the_real_queue_drains_to_nothing_one_entry_at_a_time() {
1026        let entries = parse(CORPUS);
1027        let mut done = Vec::new();
1028        let mut text = CORPUS.to_string();
1029        for entry in &entries {
1030            done.push(entry.clone());
1031            text = without(CORPUS, &done);
1032        }
1033        assert_eq!("", text);
1034        assert_eq!(without(CORPUS, &entries), text);
1035    }
1036
1037    /// Taking the middle one out leaves the other four intact, including the
1038    /// one a person would notice first if the seams were wrong.
1039    #[test]
1040    fn draining_one_entry_leaves_the_rest_byte_for_byte() {
1041        let entries = parse(CORPUS);
1042        let out = without(CORPUS, &[entries[2].clone()]);
1043        let left = parse(&out);
1044        assert_eq!(4, left.len());
1045        for (before, after) in [(0, 0), (1, 1), (3, 2), (4, 3)] {
1046            assert_eq!(entries[before].title, left[after].title);
1047            assert_eq!(entries[before].body, left[after].body);
1048        }
1049    }
1050}