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 answer: ScreenResponse =
342        agent.ask_json(&prompt, &schema::screen(), repo.root(), &effort)?;
343    Ok(answer.entries)
344}
345
346// ---------------------------------------------------------------------------
347// Working the queue
348// ---------------------------------------------------------------------------
349
350/// Where a run of `spar followup` stops.
351#[derive(Debug, Clone, Copy, PartialEq, Eq)]
352pub enum Mode {
353    /// Print the verdicts and touch nothing.
354    ScreenOnly,
355    /// File the issues and stop, leaving them for a later `spar run`.
356    FileOnly,
357    /// File them and work them.
358    Work,
359}
360
361/// What was filed, and what was left behind.
362#[derive(Debug, Default)]
363pub struct Outcome {
364    /// Issues to work, in file order.
365    pub issues: Vec<i64>,
366    /// Left in the file: not reached, not screened, or could not be filed.
367    pub held: usize,
368}
369
370/// Read the queue, screen it, file what still holds, and take the filed entries
371/// out of the file.
372pub fn run(
373    agents: &[Agent],
374    cfg: &Config,
375    repo: &Repo,
376    path: &Path,
377    limit: usize,
378    mode: Mode,
379) -> Result<Outcome> {
380    let mut outcome = Outcome::default();
381
382    // Before any network call, so an empty queue is a purely local no-op.
383    let Ok(original) = std::fs::read_to_string(path) else {
384        log!("no follow-ups recorded in {}", path.display());
385        if repo.followups != Followups::Local {
386            log!(
387                "followups = \"{}\" is configured, so nothing is written to that file.",
388                repo.followups
389            );
390        }
391        return Ok(outcome);
392    };
393    if original.trim().is_empty() {
394        log!("{} is there and empty", path.display());
395        return Ok(outcome);
396    }
397
398    let entries = parse(&original);
399    if entries.is_empty() {
400        // A parser problem must not be reported as an empty queue.
401        log!(
402            "{} has no `## ` headings, so there is nothing to work. An entry is a `## Title` line \
403             and the text under it.",
404            path.display()
405        );
406        return Ok(outcome);
407    }
408
409    let taken: Vec<Entry> = entries.iter().take(limit).cloned().collect();
410    outcome.held += entries.len() - taken.len();
411    if outcome.held > 0 {
412        log!(
413            "{} follow-up(s) recorded, taking the first {limit}. Raise --limit for the rest.",
414            entries.len()
415        );
416    }
417
418    let agent = crate::agent::find(agents, &cfg.first_implementor)?;
419    // An `already_fixed` verdict is uninterpretable without knowing what was
420    // being judged.
421    log!(
422        "screening {} follow-up(s) with {} against {} at {}",
423        taken.len(),
424        agent.name(),
425        repo.git_try(&["rev-parse", "--abbrev-ref", "HEAD"]).trim(),
426        repo.git_try(&["rev-parse", "--short", "HEAD"]).trim(),
427    );
428
429    // A screen that did not happen is not a screen that found nothing: nothing
430    // is filed and the file is not touched.
431    let verdicts = screen(agent, cfg, repo, &taken)?;
432
433    if mode == Mode::ScreenOnly {
434        print_verdicts(&taken, &verdicts);
435        return Ok(outcome);
436    }
437
438    let mut disposed: Vec<Entry> = Vec::new();
439    // What each entry became, so a sibling that says it duplicates one can be
440    // answered after the batch rather than guessed at during it. An entry
441    // pointing at one that was itself dropped, or failed to file, is kept:
442    // archiving it as covered by an issue number that is really an entry
443    // number sends a reader to an unrelated issue and loses both copies.
444    let mut filed_as: BTreeMap<i64, i64> = BTreeMap::new();
445    let mut deferred: Vec<usize> = Vec::new();
446
447    for pass in 0..2 {
448        for (i, entry) in taken.iter().enumerate() {
449            let number = i as i64 + 1;
450            if pass == 0 && disposed.iter().any(|d| d.title == entry.title) {
451                continue;
452            }
453            if pass == 1 && !deferred.contains(&i) {
454                continue;
455            }
456            let Some(verdict) = verdicts.iter().find(|v| v.entry == number) else {
457                // Never read as "drop it". Filing something nobody looked at
458                // defeats the point of the screen; leaving it costs one line.
459                logwarn!(
460                    "no verdict for '{}', leaving it in the file",
461                    first_line(&entry.title)
462                );
463                outcome.held += 1;
464                continue;
465            };
466
467            // An entry that says it duplicates a sibling waits for the sibling to
468            // be dealt with, because what happened to that one decides this one.
469            if pass == 0
470                && verdict.verdict == Screened::Duplicate
471                && verdict.duplicate_of_entry.is_some()
472            {
473                deferred.push(i);
474                continue;
475            }
476            let covered_by = verdict
477                .duplicate_of_entry
478                .and_then(|target| filed_as.get(&target).copied())
479                .or(verdict.duplicate_of_issue);
480            if verdict.verdict == Screened::Duplicate
481                && verdict.duplicate_of_entry.is_some()
482                && covered_by.is_none()
483            {
484                logwarn!(
485                    "'{}' was called a duplicate of entry {}, which was not filed, so it stays in \
486                 the file",
487                    first_line(&entry.title),
488                    verdict.duplicate_of_entry.unwrap_or_default()
489                );
490                outcome.held += 1;
491                continue;
492            }
493
494            // "Duplicate of nothing in particular" is unfalsifiable, and
495            // `find_similar_issue` on the filing path is exactly the check for it.
496            let files = verdict.verdict == Screened::StillRelevant
497                || (verdict.verdict == Screened::Duplicate && !verdict.names_a_duplicate());
498
499            if files {
500                let retitled = !verdict.title.trim().is_empty()
501                    && !verdict
502                        .title
503                        .trim()
504                        .eq_ignore_ascii_case(entry.title.trim());
505                let title = if verdict.title.trim().is_empty() {
506                    entry.title.as_str()
507                } else {
508                    verdict.title.as_str()
509                };
510                match crate::review::file_as_issue(repo, title, &entry.body) {
511                    Ok(filed) => {
512                        log!("  {}", filed.describe(title));
513                        if let Some(n) = filed.number() {
514                            outcome.issues.push(n);
515                            filed_as.insert(number, n);
516                        }
517                        // Archived under the title it was written with, so the next
518                        // run that rediscovers the same defect in the original
519                        // wording finds it here rather than appending it again. The
520                        // new title goes in the verdict, and gets a heading of its
521                        // own, so either wording is recognised.
522                        repo.archive_followup(
523                            &entry.title,
524                            &entry.body,
525                            &format!(
526                                "Filed: {}{}",
527                                filed.note(),
528                                if retitled {
529                                    format!(", retitled to '{}'", title.trim())
530                                } else {
531                                    String::new()
532                                }
533                            ),
534                        );
535                        if retitled {
536                            repo.archive_followup(
537                                title,
538                                &format!("The same entry as '{}'.", entry.title.trim()),
539                                &format!("Filed: {}", filed.note()),
540                            );
541                        }
542                    }
543                    Err(e) => {
544                        logwarn!("could not file '{}': {e}", first_line(title));
545                        outcome.held += 1;
546                        continue;
547                    }
548                }
549            } else {
550                let why = dropped_note(verdict, covered_by);
551                log!("  dropped '{}': {why}", first_line(&entry.title));
552                repo.archive_followup(&entry.title, &entry.body, &format!("Dropped: {why}"));
553            }
554
555            disposed.push(entry.clone());
556            // Written after the entry is dealt with, never before. The window is
557            // one entry wide and it always falls on the side of filing twice rather
558            // than losing one: an entry filed and not yet removed is found again
559            // next run and matched to the issue that was just created, while an
560            // entry removed before it was filed is gone.
561            crate::repo::write_text_atomic(path, &without(&original, &disposed)).map_err(|e| {
562                spar_err!(
563                "{e}\n{} follow-up(s) were already dealt with. Remove them from {} by hand before \
564                 running this again, or they will be filed twice.",
565                disposed.len(),
566                path.display()
567            )
568            })?;
569        }
570    }
571
572    let filed = outcome.issues.len();
573    println!(
574        "\nfollowups: {} screened, {filed} filed{}",
575        taken.len(),
576        summarise(&taken, &verdicts)
577    );
578    if outcome.held > 0 {
579        println!("{} entry(s) left in {}", outcome.held, path.display());
580    }
581    if !disposed.is_empty() {
582        println!(
583            "what was dealt with is in {}",
584            repo.worked_followups_path().display()
585        );
586    }
587    Ok(outcome)
588}
589
590fn first_line(text: &str) -> String {
591    crate::style::clip(text.trim().lines().next().unwrap_or("").trim(), 80)
592}
593
594fn dropped_note(v: &ScreenVerdict, covered_by: Option<i64>) -> String {
595    let reason = v.reason.trim();
596    match (v.verdict, covered_by) {
597        (Screened::Duplicate, Some(n)) if reason.is_empty() => format!("#{n} already covers it"),
598        (Screened::Duplicate, Some(n)) => format!("#{n} already covers it. {reason}"),
599        (_, _) if reason.is_empty() => v.verdict.to_string(),
600        _ => format!("{}. {reason}", v.verdict),
601    }
602}
603
604/// The tail of the summary line, naming each verdict that dropped something.
605fn summarise(taken: &[Entry], verdicts: &[ScreenVerdict]) -> String {
606    let mut counts: Vec<(Screened, usize)> = Vec::new();
607    for v in verdicts {
608        if v.entry < 1 || v.entry as usize > taken.len() {
609            continue;
610        }
611        match counts.iter_mut().find(|(k, _)| *k == v.verdict) {
612            Some((_, n)) => *n += 1,
613            None => counts.push((v.verdict, 1)),
614        }
615    }
616    counts.retain(|(k, _)| *k != Screened::StillRelevant);
617    if counts.is_empty() {
618        return String::new();
619    }
620    let listed: Vec<String> = counts
621        .iter()
622        .map(|(k, n)| format!("{n} {}", k.as_str().replace('_', " ")))
623        .collect();
624    format!(", {}", listed.join(", "))
625}
626
627/// What `--screen-only` prints. `println!`, not `log!`: it is the whole output
628/// of the command, and an entry dropped with no visible record of why is
629/// exactly the failure the archive exists for.
630fn print_verdicts(taken: &[Entry], verdicts: &[ScreenVerdict]) {
631    println!();
632    for (i, entry) in taken.iter().enumerate() {
633        let number = i as i64 + 1;
634        match verdicts.iter().find(|v| v.entry == number) {
635            Some(v) => println!(
636                "  {:<14} {}\n                 {}",
637                v.verdict.as_str(),
638                first_line(&entry.title),
639                v.reason.trim()
640            ),
641            None => println!("  {:<14} {}", "no verdict", first_line(&entry.title)),
642        }
643    }
644    let filed = verdicts
645        .iter()
646        .filter(|v| v.verdict == Screened::StillRelevant)
647        .count();
648    println!(
649        "\n{filed} of {} would be filed. Nothing was written.",
650        taken.len()
651    );
652}
653
654/// The issue numbers a set of outcomes produced, deduplicated and in order.
655pub fn wave(outcome: &Outcome) -> Vec<i64> {
656    outcome
657        .issues
658        .iter()
659        .copied()
660        .collect::<BTreeSet<_>>()
661        .into_iter()
662        .collect()
663}
664
665#[cfg(test)]
666mod tests {
667    use super::*;
668
669    /// Two entries in the shape a real file has: a title, a lead paragraph,
670    /// the four report sections, and the provenance line.
671    const REAL: &str = "\
672## Backend headers never drive commitment CPFP retries
673
674The production ChainWatcher advances monitors and emits block.
675
676## Problem
677
678Configured chain backends route accepted headers through handleNewBlock.
679
680## Reproduction
681
6821. Configure a node with a watcher backend.
6832. Deliver height 101.
684
685## Impact
686
687Nodes do not retry stuck commitment packages on new blocks.
688
689## Expected behavior
690
691Run the pass exactly once for each accepted backend header.
692
693Found while working on #589.
694
695## Overlapping scans can move a recorded spend height backward
696
697## Problem
698
699checkOutputSpend applies its result with no arbitration against a later scan.
700
701## Impact
702
703A stale verdict can overwrite a newer one.
704
705Found while working on #590.
706";
707
708    /// The whole reason this module exists. A follow-up's own sections are
709    /// written at the same heading level as its title, so a naive split on
710    /// `## ` files six issues here, four of them called things like "Impact".
711    #[test]
712    fn an_entry_and_its_sections_are_not_confused_for_each_other() {
713        let entries = parse(REAL);
714        assert_eq!(
715            2,
716            entries.len(),
717            "{:#?}",
718            entries.iter().map(|e| &e.title).collect::<Vec<_>>()
719        );
720        assert!(entries[0].title.starts_with("Backend headers"));
721        assert!(entries[1].title.starts_with("Overlapping scans"));
722        // The sections stay with the entry they belong to.
723        assert!(
724            entries[0].body.contains("## Reproduction"),
725            "{}",
726            entries[0].body
727        );
728        assert!(entries[0].body.contains("Found while working on #589."));
729    }
730
731    /// The heading heuristic alone cannot see a title that collides with a
732    /// section name. The marker is what fixes that going forward, and spar
733    /// writes one above every entry it appends.
734    #[test]
735    fn a_marker_makes_the_boundary_exact() {
736        let text = format!(
737            "{FOLLOWUP_MARKER}\n## Impact\n\nThe first one.\n\n\
738             {FOLLOWUP_MARKER}\n## Problem\n\nThe second one.\n"
739        );
740        let entries = parse(&text);
741        assert_eq!(2, entries.len());
742        assert_eq!("Impact", entries[0].title);
743        assert_eq!("Problem", entries[1].title);
744    }
745
746    /// Keeping the queue by hand is a documented use, and it must not require
747    /// knowing spar's marker.
748    #[test]
749    fn a_hand_written_file_with_no_markers_still_parses() {
750        let text =
751            "## One thing\n\nprose\n\n## Another thing\n\nmore prose\n\n## A third\n\nyet more\n";
752        let entries = parse(text);
753        assert_eq!(3, entries.len());
754        assert_eq!("Another thing", entries[1].title);
755    }
756
757    /// A body is free to carry a snippet, and `style::issue_body` deliberately
758    /// protects fenced blocks from the length budget, so a snippet containing a
759    /// `## ` line would otherwise split the entry that quotes it.
760    #[test]
761    fn a_heading_inside_a_fenced_block_does_not_start_an_entry() {
762        let text = "## Real title\n\n```md\n## Problem\n## Not a title either\n```\n\nprose\n";
763        let entries = parse(text);
764        assert_eq!(1, entries.len(), "{:?}", entries);
765        assert_eq!("Real title", entries[0].title);
766    }
767
768    /// `is_section_heading` compares the whole heading. A prefix match would
769    /// swallow an entry whose title happens to open with a section word.
770    #[test]
771    fn an_entry_whose_title_opens_with_a_section_word_is_still_a_title() {
772        let text =
773            "## First\n\nprose\n\n## Reproduction steps are missing from the docs\n\nprose\n";
774        assert_eq!(2, parse(text).len());
775    }
776
777    /// A fifth section added to a bug report and forgotten here would split
778    /// every entry that used it, and the second half would be filed on its own.
779    #[test]
780    fn the_section_list_covers_every_heading_a_report_writes() {
781        for heading in report_headings() {
782            assert!(
783                is_section_heading(heading),
784                "`## {heading}` would be read as the start of a new follow-up"
785            );
786        }
787    }
788
789    /// A rewrite that re-renders what stays silently deletes whatever a person
790    /// added: a note at the top, an edit inside a body, a trailing reminder.
791    #[test]
792    fn text_the_parser_does_not_own_survives_a_rewrite() {
793        let text = "A note I keep at the top.\n\n\
794                    ## One\n\nfirst\n\n\
795                    ## Two\n\nsecond\n\n\
796                    ## Three\n\nthird\n";
797        let entries = parse(text);
798        assert_eq!(3, entries.len());
799        let out = without(text, &[entries[1].clone()]);
800        assert!(out.starts_with("A note I keep at the top."), "{out}");
801        assert!(out.contains("## One"), "{out}");
802        assert!(!out.contains("## Two"), "{out}");
803        assert!(out.contains("## Three"), "{out}");
804        assert!(out.contains("third"), "{out}");
805    }
806
807    /// The guard against splicing a splice. Spans index the text as it was
808    /// parsed, so removing one entry would shift every later offset, and the
809    /// run rewrites the file once per entry.
810    #[test]
811    fn removing_entries_one_at_a_time_matches_removing_them_at_once() {
812        let entries = parse(REAL);
813        let all_at_once = without(REAL, &entries);
814
815        let mut done = Vec::new();
816        let mut last = String::new();
817        for entry in &entries {
818            done.push(entry.clone());
819            last = without(REAL, &done);
820        }
821        assert_eq!(all_at_once, last);
822        assert!(last.is_empty(), "{last:?}");
823    }
824
825    /// A caller that recorded the same entry twice, or recorded them out of
826    /// order, must not corrupt the file or panic on a backwards slice.
827    #[test]
828    fn without_tolerates_a_repeated_or_unordered_span() {
829        let entries = parse(REAL);
830        let once = without(REAL, &[entries[0].clone()]);
831        let twice = without(REAL, &[entries[0].clone(), entries[0].clone()]);
832        assert_eq!(once, twice);
833
834        let forwards = without(REAL, &[entries[0].clone(), entries[1].clone()]);
835        let backwards = without(REAL, &[entries[1].clone(), entries[0].clone()]);
836        assert_eq!(forwards, backwards);
837    }
838
839    /// An emptied queue is an empty file, not a pile of blank lines that reads
840    /// as content to the next thing that opens it.
841    #[test]
842    fn removing_every_entry_leaves_an_empty_file() {
843        let entries = parse(REAL);
844        assert_eq!("", without(REAL, &entries));
845    }
846
847    /// The link back to the work that found a defect cannot be re-derived, so
848    /// the body has to carry it through unchanged.
849    #[test]
850    fn an_entry_keeps_the_provenance_it_was_written_with() {
851        let entries = parse(REAL);
852        assert!(entries[1].body.ends_with("Found while working on #590."));
853    }
854
855    /// A file edited on Windows must parse the same as one edited anywhere else.
856    #[test]
857    fn crlf_line_endings_parse_the_same_as_lf() {
858        let lf = "## One\n\nfirst\n\n## Two\n\nsecond\n";
859        let crlf = lf.replace('\n', "\r\n");
860        let a = parse(lf);
861        let b = parse(&crlf);
862        assert_eq!(a.len(), b.len());
863        assert_eq!(a[1].title, b[1].title);
864    }
865
866    /// A file whose first heading is a section name holds one entry called
867    /// that, rather than nothing at all.
868    #[test]
869    fn a_file_that_opens_with_a_section_name_still_holds_an_entry() {
870        let entries = parse("## Problem\n\nsomething is wrong\n");
871        assert_eq!(1, entries.len());
872        assert_eq!("Problem", entries[0].title);
873    }
874
875    fn verdict(entry: i64, v: Screened, dup: Option<i64>) -> ScreenVerdict {
876        ScreenVerdict {
877            entry,
878            verdict: v,
879            title: String::new(),
880            reason: "because".into(),
881            duplicate_of_issue: dup,
882            duplicate_of_entry: None,
883        }
884    }
885
886    fn duplicate_of_entry(entry: i64, target: i64) -> ScreenVerdict {
887        ScreenVerdict {
888            entry,
889            verdict: Screened::Duplicate,
890            title: String::new(),
891            reason: "same as the one above".into(),
892            duplicate_of_issue: None,
893            duplicate_of_entry: Some(target),
894        }
895    }
896
897    /// "Duplicate of nothing in particular" is unfalsifiable, and the search on
898    /// the filing path is exactly the check for it. Dropping the entry on that
899    /// verdict would delete a real defect on no evidence.
900    #[test]
901    fn a_duplicate_verdict_with_nothing_to_point_at_would_still_be_filed() {
902        let with_number = verdict(1, Screened::Duplicate, Some(412));
903        let without_number = verdict(1, Screened::Duplicate, None);
904        let files = |v: &ScreenVerdict| {
905            v.verdict == Screened::StillRelevant
906                || (v.verdict == Screened::Duplicate && !v.names_a_duplicate())
907        };
908        assert!(!files(&with_number));
909        assert!(files(&without_number));
910    }
911
912    /// A short answer must never read as "drop the rest". The entries the
913    /// screen did not rule on stay in the file.
914    #[test]
915    fn an_entry_with_no_verdict_is_not_disposed_of() {
916        let entries = parse(REAL);
917        let verdicts = [verdict(1, Screened::AlreadyFixed, None)];
918        let unruled: Vec<usize> = (1..=entries.len())
919            .filter(|n| !verdicts.iter().any(|v| v.entry == *n as i64))
920            .collect();
921        assert_eq!(vec![2], unruled);
922    }
923
924    /// An out of range index used as a slice index panics, and a verdict for an
925    /// entry that does not exist must not shift the ones that do.
926    #[test]
927    fn a_verdict_naming_an_entry_that_does_not_exist_is_ignored() {
928        let entries = parse(REAL);
929        let verdicts = [verdict(9, Screened::AlreadyFixed, None)];
930        assert_eq!("", summarise(&entries, &verdicts));
931    }
932
933    /// The summary is the only place a dropped entry is accounted for, and it
934    /// has to name what happened rather than only how many.
935    #[test]
936    fn the_summary_names_each_verdict_that_dropped_something() {
937        let entries = parse(REAL);
938        let verdicts = vec![
939            verdict(1, Screened::AlreadyFixed, None),
940            verdict(2, Screened::StillRelevant, None),
941        ];
942        let out = summarise(&entries, &verdicts);
943        assert!(out.contains("1 already fixed"), "{out}");
944        assert!(!out.contains("still relevant"), "{out}");
945    }
946
947    /// The reason is the only record of why an entry left the queue, so it has
948    /// to survive into the log line and the archive.
949    #[test]
950    fn a_dropped_entry_carries_its_reason_and_the_issue_it_duplicates() {
951        let v = verdict(1, Screened::Duplicate, Some(412));
952        let note = dropped_note(&v, v.duplicate_of_issue);
953        assert!(note.contains("#412"), "{note}");
954        assert!(note.contains("because"), "{note}");
955    }
956
957    /// An entry number is not an issue number, and one field could not say
958    /// which it was.
959    ///
960    /// Entry 4 duplicating entry 2 was archived as covered by issue #2, an
961    /// unrelated issue, and if entry 2 was then ruled already fixed or failed
962    /// to file, neither copy was filed at all.
963    #[test]
964    fn a_duplicate_of_a_sibling_is_kept_apart_from_a_duplicate_of_an_issue() {
965        let sibling = duplicate_of_entry(4, 2);
966        assert!(sibling.names_a_duplicate(), "it points at something");
967        assert_eq!(None, sibling.duplicate_of_issue);
968
969        // Unresolved, it names nothing to send a reader to.
970        let unresolved = dropped_note(&sibling, None);
971        assert!(!unresolved.contains("#2"), "{unresolved}");
972
973        // Resolved after the batch, it names the issue that entry became.
974        let resolved = dropped_note(&sibling, Some(512));
975        assert!(resolved.contains("#512"), "{resolved}");
976    }
977}
978
979#[cfg(test)]
980mod real_file {
981    use super::*;
982
983    /// The queue one real run left on a real repository, captured verbatim.
984    /// Five follow-ups, twenty-five `## ` lines. This is the file the heuristic
985    /// is measured against rather than guessed at.
986    const CORPUS: &str = include_str!("../tests/fixtures/local_followups.md");
987
988    #[test]
989    fn the_real_queue_parses_as_five_follow_ups_not_twenty_five() {
990        let entries = parse(CORPUS);
991        assert_eq!(
992            5,
993            entries.len(),
994            "{:#?}",
995            entries.iter().map(|e| e.title.as_str()).collect::<Vec<_>>()
996        );
997        for entry in &entries {
998            assert!(
999                !is_section_heading(&entry.title),
1000                "a section was filed as a follow-up: {}",
1001                entry.title
1002            );
1003            assert!(!entry.body.trim().is_empty(), "{} has no body", entry.title);
1004        }
1005    }
1006
1007    /// Every entry in that file ends with the line that says which issue it
1008    /// came out of, and an issue filed from it is the only place that link
1009    /// survives.
1010    #[test]
1011    fn every_entry_in_the_real_queue_keeps_its_provenance() {
1012        for entry in parse(CORPUS) {
1013            assert!(
1014                entry.body.contains("Found while working on #"),
1015                "{} lost its provenance",
1016                entry.title
1017            );
1018        }
1019    }
1020
1021    /// Removing them one at a time, always against the original text, has to
1022    /// end where removing them all at once does, and has to end empty.
1023    #[test]
1024    fn the_real_queue_drains_to_nothing_one_entry_at_a_time() {
1025        let entries = parse(CORPUS);
1026        let mut done = Vec::new();
1027        let mut text = CORPUS.to_string();
1028        for entry in &entries {
1029            done.push(entry.clone());
1030            text = without(CORPUS, &done);
1031        }
1032        assert_eq!("", text);
1033        assert_eq!(without(CORPUS, &entries), text);
1034    }
1035
1036    /// Taking the middle one out leaves the other four intact, including the
1037    /// one a person would notice first if the seams were wrong.
1038    #[test]
1039    fn draining_one_entry_leaves_the_rest_byte_for_byte() {
1040        let entries = parse(CORPUS);
1041        let out = without(CORPUS, &[entries[2].clone()]);
1042        let left = parse(&out);
1043        assert_eq!(4, left.len());
1044        for (before, after) in [(0, 0), (1, 1), (3, 2), (4, 3)] {
1045            assert_eq!(entries[before].title, left[after].title);
1046            assert_eq!(entries[before].body, left[after].body);
1047        }
1048    }
1049}