spar/

review.rs

//! Alternating custody until a PR converges.
//!
//! Roles are not fixed. Whoever holds the PR may implement, review, fix, or
//! file follow-ups, and then hands custody to the other. An agent never
//! reviews its own most recent edit, and custody follows the commit that
//! landed rather than the action a reviewer asked for: a call that returns is
//! not a call that wrote anything.
//!
//! Three failure modes are handled explicitly here, because each one breaks a
//! naive loop:
//!
//! - **The nitpick spiral.** Round 6 findings are worse than round 1 findings
//!   and a loop that counts objections cannot tell. Only `blocking` gates.
//! - **Re-litigation.** A refuted point re-raised forever never terminates.
//!   Refutations are hashed into a ledger carried across rounds.
//! - **Approval drift.** Optimising for "get approved" pressures the author
//!   into accepting wrong review comments, so refutation is blessed and the
//!   merge gate is blocking-findings-empty, not reviewer-satisfied.

use std::path::{Path, PathBuf};

use crate::agent::{self, Agent};
use crate::config::{Config, Drafts, Followups, PrComments};
use crate::error::{Result, SparError};
use crate::jsonx::finding_key;
use crate::model::{
    Action, Dispute, Finding, Followup, Implementation, Issue, IssueRun, Ledger, LedgerEntry,
    NextAction, PersistedState, PlanItem, PrView, ResponseDoc, Review, Settled, Severity,
    SkippedItem, Status, STATE_VERSION,
};
use crate::repo::Repo;
use crate::style::{self, Style};
use crate::{log, logdim, logwarn, schema, spar_err};

// ---------------------------------------------------------------------------
// Prompts
// ---------------------------------------------------------------------------

const IMPLEMENT_PROMPT: &str = "\
Implement GitHub issue #{number} in this repository.

Title: {title}
URL: {url}

{body}

That is the issue body as filed. The discussion since is not included, so read
the thread at the URL above if the body leaves anything open. If you cannot
reach the network, work from what is here.

Do the work, then commit it on the current branch. Make focused commits with
clear messages. Do not push, do not open a PR, and do not merge; the harness
handles that.

Then report it. Your answer becomes the pull request description, and the
reviewer reads that cold, with nothing but the diff and a link to the issue:
say what you found wrong, what the change does about it, and how they confirm
it for themselves. Say what you actually ran, not what could be run.

If after reading the code you conclude this issue should not be implemented,
make no commits and set not_worth_doing, with the reason.";

const REVIEW_PROMPT: &str = "\
Review the changes on this branch against `{base}`. They implement issue
#{number}: {title}

Review thoroughly: correctness, edge cases, error handling, security, and
whether the change actually resolves the issue. Read surrounding code, do not
only read the diff.

Label every finding by severity, and be honest about which is which:
- blocking: the PR should not merge as is. Real defects only.
- non-blocking: a genuine improvement that need not gate this PR.
- nit: style or taste.

Confirm anything you label blocking before you label it. Run the code,
reproduce the failure, or point at the exact line that breaks, and say in the
detail what you did to confirm it. When you need to run something to check a
claim, write a scratch file and run that, rather than passing a long program on
the command line: it is easier to read back, easier to rerun, and less likely to
be refused by a sandbox or a safety filter part way through your work. An unverified blocking finding is worse than
one you never raised: it stalls a good PR and teaches the author to stop
believing you. If you suspect a problem but could not confirm it, say so and
label it non-blocking.

Set in_scope=false for a real defect that exists, that this PR did not cause, and
that is worth somebody stopping to fix. Each one becomes a tracked item a
maintainer has to read and triage, so the bar is a defect and not an observation.
A thorough reviewer can always find something adjacent to what it is reading;
that is not a reason to file it. If you are not sure it is worth a maintainer's
time, leave in_scope true and say your piece in the finding.

Reviewing one issue should not manufacture ten more. If you find yourself with
several out of scope findings, keep the ones that would bite somebody and drop
the rest.

Then choose next_action:
- merge: no blocking findings, the PR is good.
- fix_myself: there are blocking findings and you will fix them directly.
- hand_back: there are blocking findings the author should address.
{settled}";

const FIX_PROMPT: &str = "\
You reviewed this branch and chose to fix the blocking findings yourself.
Implement those fixes now and commit them.

Your findings:
{findings}

Commit your changes. Do not push, do not merge.";

const RESPOND_PROMPT: &str = "\
Here is a review of your PR for issue #{number}.

{findings}

For each point, choose exactly one disposition:
- fixed: the point is valid and in scope. Fix it and commit.
- refuted: the point is wrong, or not worth acting on. Explain why. Refuting is
  a legitimate outcome; do not accept a review comment you believe is incorrect
  just to get the PR approved.
- filed_issue: the point is valid but unrelated to this PR. Supply
  new_issue_title and new_issue_body; the harness files it and skips duplicates.

Copy each finding's title and file across exactly as given, so your answer can
be matched back to the review.

Commit any fixes. Do not push, do not merge.";

// ---------------------------------------------------------------------------
// Evidence
// ---------------------------------------------------------------------------

/// What the branch looked like at one point in a round.
///
/// Untracked files are deliberately not dirt. The review prompt asks for a
/// scratch file when a claim needs running to check it, so counting one as a
/// mutation would reject every review that did as it was told.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Snapshot {
    pub head: String,
    /// Tracked files differing from the index or the head.
    pub dirty: bool,
}

impl Snapshot {
    /// Whether a commit landed between the two. An empty head means git could
    /// not be read, which is not evidence that anything was written.
    pub fn landed_over(&self, before: &Snapshot) -> bool {
        !self.head.is_empty() && self.head != before.head
    }
}

pub fn snapshot(repo: &Repo, work_dir: &Path) -> Snapshot {
    Snapshot {
        head: repo
            .git_try_at(Some(work_dir), &["rev-parse", "HEAD"])
            .trim()
            .to_string(),
        dirty: !repo
            .git_try_at(
                Some(work_dir),
                &["status", "--porcelain", "--untracked-files=no"],
            )
            .trim()
            .is_empty(),
    }
}

/// Copy the tracked edits in the tree to somewhere they can be got back from.
///
/// `git stash create` writes them as a dangling commit and, unlike `git stash
/// push`, leaves the stash stack alone: the stack belongs to whoever is working
/// in the repository, and every worktree of it shares the same one. `None` when
/// there was nothing to save.
pub fn park(repo: &Repo, work_dir: &Path) -> Option<String> {
    let saved = repo
        .git_try_at(Some(work_dir), &["stash", "create"])
        .trim()
        .to_string();
    (!saved.is_empty()).then_some(saved)
}

/// Reset the tree to `target`, saving what that throws away.
///
/// With `--no-worktrees` the checkout is the user's own, and nothing here can
/// tell an edit an agent left behind from one a person made while a call was
/// running. So the discard is never silent and never final: the changes are
/// parked first and the log says how to put them back.
fn reset_saving(repo: &Repo, work_dir: &Path, target: &str) {
    let parked = park(repo, work_dir);
    if let Err(e) = repo.git_at(Some(work_dir), &["reset", "--hard", target]) {
        logdim!("could not roll the working tree back: {e}");
        return;
    }
    if let Some(saved) = parked {
        logdim!("`git stash apply {saved}` puts the discarded changes back");
    }
}

/// Put the branch back where the review found it.
///
/// Nothing here was ever pushed: the loop pushes at the end of a round, so the
/// head a review starts from is the head the pull request already has. What is
/// discarded is therefore only what the review wrote after being told not to,
/// and keeping it would hand the reviewer its own commit to review next round.
///
/// Returns the state afterwards, which equals `before` when the rollback took.
/// The caller compares, because a rollback that did not take means the reviewer
/// wrote the head and custody has to follow it there.
pub fn undo_edits(repo: &Repo, work_dir: &Path, before: &Snapshot) -> Snapshot {
    let current = snapshot(repo, work_dir);
    if before.head.is_empty() {
        return current;
    }
    if current.landed_over(before) {
        logdim!(
            "the commits being rolled back are still at {}",
            current.head
        );
    }
    reset_saving(repo, work_dir, &before.head);
    snapshot(repo, work_dir)
}

/// Drop what a call left uncommitted, keeping whatever it committed.
///
/// Only commits reach the pull request, but the next review reads the working
/// tree, so an edit left behind is code the reviewer judges and the diff does
/// not have. That is how an agent comes to approve a fix of its own that
/// nobody else can see.
pub fn drop_uncommitted(repo: &Repo, work_dir: &Path) -> Snapshot {
    let current = snapshot(repo, work_dir);
    if !current.dirty || current.head.is_empty() {
        return current;
    }
    reset_saving(repo, work_dir, &current.head);
    snapshot(repo, work_dir)
}

/// A worktree is only worth keeping when a person has to look at it locally.
/// Anything else strands a checked-out branch that blocks
/// `gh pr merge --delete-branch`, and since auto_merge is off by default,
/// keeping it on anything but "merged" leaks one per run.
fn should_release(cfg: &Config, status: Status) -> bool {
    if !cfg.loop_cfg.worktrees || cfg.loop_cfg.keep_worktrees {
        return false;
    }
    !matches!(status, Status::Escalated | Status::Error)
}

// ---------------------------------------------------------------------------
// One issue, start to finish
// ---------------------------------------------------------------------------

pub fn run_issue(
    agents: &[Agent],
    cfg: &Config,
    repo: &Repo,
    item: &PlanItem,
    issue: &Issue,
    ledger: &mut Ledger,
) -> IssueRun {
    // Continue an existing PR rather than implementing over the top of it.
    //
    // Without this, a second `spar run 42` deletes the local branch, rebuilds
    // it from the base, implements from scratch, and force pushes. The lease
    // holds because the remote tracking ref survives the local branch being
    // deleted, so the push succeeds and the previous round's work is gone from
    // the PR with nothing to say it ever existed.
    if let Some(existing) = repo.open_pr_for_issue(item.issue) {
        log!(
            "#{}: {} is already open, continuing it instead of implementing again",
            item.issue,
            existing.url
        );
        return resume_pr(agents, cfg, repo, existing.number, None);
    }

    let mut state = IssueRun::new(item.issue, item.title.clone());
    let base = cfg.base_branch().to_string();

    let prepared = if cfg.loop_cfg.worktrees {
        repo.worktree_add(item.issue, &base)
    } else {
        let branch = repo.branch_for_issue(item.issue);
        let start = format!("origin/{base}");
        repo.git(&["checkout", "-B", &branch, &start])
            .map(|_| (repo.root().to_path_buf(), branch))
    };

    let (work_dir, branch) = match prepared {
        Ok(pair) => pair,
        Err(e) => {
            state.status = Status::Error;
            state.notes.push(e.to_string());
            log!("#{} failed: {e}", item.issue);
            return state;
        }
    };

    let outcome = implement_and_review(
        agents, cfg, repo, item, issue, ledger, &mut state, &work_dir, &branch,
    );
    if let Err(e) = outcome {
        state.status = Status::Error;
        state.notes.push(e.to_string());
        log!("#{} failed: {e}", item.issue);
    }

    if should_release(cfg, state.status) {
        repo.worktree_remove(item.issue);
    }
    state
}

#[allow(clippy::too_many_arguments)]
fn implement_and_review(
    agents: &[Agent],
    cfg: &Config,
    repo: &Repo,
    item: &PlanItem,
    issue: &Issue,
    ledger: &mut Ledger,
    state: &mut IssueRun,
    work_dir: &Path,
    branch: &str,
) -> Result<()> {
    let number = item.issue;
    let holder = cfg.first_implementor.clone();
    let implementor = agent::find(agents, &holder)?;
    let base = cfg.base_branch().to_string();

    log!("#{number}: {holder} implementing");
    // Fixing triage alone would have been worse than fixing neither: an issue
    // correctly judged worth doing on its whole text, then built from the first
    // few thousand characters of it, raises confidence without raising
    // fidelity.
    let (body, shortened) = issue.body_for_prompt(cfg.loop_cfg.max_issue_chars);
    if shortened {
        logwarn!(
            "#{number}: the issue body was shortened to fit the prompt. Raise max_issue_chars if \
             the rest matters."
        );
    }
    let prompt = implement_prompt(number, &item.title, &issue.url, &body);
    let answer: Result<Implementation> = implementor.ask_json(
        &prompt,
        &schema::implementation(),
        work_dir,
        cfg.effort_for_round(&implementor.spec, 1).as_deref(),
    );

    // A call that fails with commits on the branch is not the same as one that
    // fails with nothing to show. The agent commits as it goes and reports at
    // the end, so the usual failure here is the report, not the work, and
    // returning the error would leave the commits unpushed on a local branch
    // that the next `spar run` deletes. The review loop is what the round is
    // for and it needs the diff, not the summary.
    let mut work = match answer {
        Ok(work) => work,
        Err(e) if repo.has_changes(work_dir, &base) => {
            logwarn!(
                "#{number}: {holder} failed after committing: {e}\nContinuing from the commits, \
                 with a pull request body written from their messages."
            );
            state
                .notes
                .push(format!("{holder} failed after committing: {e}"));
            from_commits(repo, work_dir, &base)
        }
        Err(e) => return Err(e),
    };

    if work.not_worth_doing || !repo.has_changes(work_dir, &base) {
        state.status = Status::Abandoned;
        let reason = no_pr_note(&work, &repo.style);
        state.notes.push(reason.clone());
        if let Err(e) = repo.comment_issue(number, &reason) {
            logdim!("could not comment on #{number}: {e}");
        }
        return Ok(());
    }

    // A body that leads with nothing is a body nobody reads past. The issue
    // title is a poor substitute for a sentence about the change, and a better
    // one than a blank first line.
    if work.summary.trim().is_empty() {
        work.summary = item.title.clone();
    }

    repo.rewrite_commits_if_needed(work_dir, &base)?;
    repo.push(work_dir, branch)?;

    let pr = match repo.pr_for_branch(branch) {
        Some(existing) => existing,
        None => {
            let body = pr_body(number, &work, &repo.style);
            repo.create_pr(
                work_dir,
                branch,
                &base,
                &format!("{} (#{number})", item.title),
                &body,
            )?
        }
    };
    state.pr = Some(pr.url.clone());
    log!("#{number}: PR {}", pr.url);

    let ctx = LoopCtx {
        work_dir: work_dir.to_path_buf(),
        branch: branch.to_string(),
        pr_number: pr.number,
        label: format!("#{number}"),
        subject: number,
        title: item.title.clone(),
        start_round: 1,
        holder: cfg.other(&holder),
        release: Release::Issue(number),
    };
    review_loop(agents, cfg, repo, &ctx, state, ledger)
}

// ---------------------------------------------------------------------------
// Resuming an existing PR
// ---------------------------------------------------------------------------

/// Pick up an existing PR and continue the loop.
///
/// The PR need not have been created by spar. Anything with a branch and a diff
/// can be reviewed, including work a person or a different tool started, which
/// is also the cheapest way to adopt spar: no agent writes a feature from
/// scratch, it only reviews what already exists.
pub fn resume_pr(
    agents: &[Agent],
    cfg: &Config,
    repo: &Repo,
    pr_number: i64,
    holder_override: Option<&str>,
) -> IssueRun {
    let failed = |e: SparError| {
        log!("PR #{pr_number} failed: {e}");
        let mut state = IssueRun::new(pr_number, format!("PR #{pr_number}"));
        state.status = Status::Error;
        state.notes.push(e.to_string());
        state
    };

    let pr = match repo.pr_view(pr_number) {
        Ok(pr) => pr,
        Err(e) => return failed(e),
    };

    // A pull request from a fork cannot be pushed to, so the loop that fixes
    // things cannot run on it. Reviewing it is still the useful thing, and it
    // is what a maintainer wants from an outside contribution anyway, so do
    // that rather than refusing.
    if pr.is_cross_repository {
        log!("PR #{pr_number} comes from a fork, reviewing it without changing it");
        return crate::review_only::review_pr(agents, cfg, repo, pr_number, false);
    }

    match resume_inner(agents, cfg, repo, pr, holder_override) {
        Ok(state) => state,
        Err(e) => failed(e),
    }
}

fn resume_inner(
    agents: &[Agent],
    cfg: &Config,
    repo: &Repo,
    pr: PrView,
    holder_override: Option<&str>,
) -> Result<IssueRun> {
    let pr_number = pr.number;
    if !pr.is_open() {
        return Err(spar_err!("PR #{pr_number} is {}", pr.state.to_lowercase()));
    }

    let subject = pr
        .closing_issues_references
        .first()
        .map(|r| r.number)
        .unwrap_or(pr_number);

    let saved = repo.read_state(&pr);
    let mut ledger: Ledger = saved.as_ref().map(|s| s.ledger.clone()).unwrap_or_default();
    let start_round = saved.as_ref().map(|s| s.round + 1).unwrap_or(1);

    let default_holder = cfg.other(&cfg.first_implementor);
    let mut holder = holder_override
        .map(str::to_string)
        .or_else(|| saved.as_ref().map(|s| s.next_actor.clone()))
        .unwrap_or_else(|| default_holder.clone());
    if !cfg.has_agent(&holder) {
        log!("state named unknown agent '{holder}', using {default_holder}");
        holder = default_holder;
    }

    match &saved {
        Some(_) => log!(
            "PR #{pr_number}: resuming at round {start_round}, {} settled point(s), next up {holder}",
            ledger.len()
        ),
        None => log!("PR #{pr_number}: no prior spar state, starting fresh with {holder}"),
    }

    let mut state = IssueRun::new(subject, pr.title.clone());
    state.pr = Some(pr.url.clone());
    if let Some(s) = &saved {
        state.filed = s.filed.clone();
    }

    let (work_dir, branch) = repo.worktree_for_pr(&pr)?;
    let ctx = LoopCtx {
        work_dir,
        branch,
        pr_number,
        label: format!("PR #{pr_number}"),
        subject,
        title: pr.title.clone(),
        start_round,
        holder,
        release: Release::Pr(pr_number),
    };

    let outcome = review_loop(agents, cfg, repo, &ctx, &mut state, &mut ledger);
    if let Err(e) = outcome {
        state.status = Status::Error;
        state.notes.push(e.to_string());
        log!("PR #{pr_number} failed: {e}");
    }
    if should_release(cfg, state.status) {
        repo.release_pr_worktree(pr_number);
    }
    Ok(state)
}

// ---------------------------------------------------------------------------
// The loop
// ---------------------------------------------------------------------------

#[derive(Debug, Clone, Copy)]
enum Release {
    Issue(i64),
    Pr(i64),
}

struct LoopCtx {
    work_dir: PathBuf,
    branch: String,
    pr_number: i64,
    label: String,
    subject: i64,
    title: String,
    start_round: u32,
    holder: String,
    release: Release,
}

impl LoopCtx {
    fn release(&self, repo: &Repo) {
        match self.release {
            Release::Issue(n) => repo.worktree_remove(n),
            Release::Pr(n) => repo.release_pr_worktree(n),
        }
    }
}

fn review_loop(
    agents: &[Agent],
    cfg: &Config,
    repo: &Repo,
    ctx: &LoopCtx,
    state: &mut IssueRun,
    ledger: &mut Ledger,
) -> Result<()> {
    let base = cfg.base_branch().to_string();
    // Never the agent that made the last commit, on entry and after every
    // round. An approval or a deadlock ends the round with nothing edited, so
    // those paths persist it unchanged.
    let mut holder = ctx.holder.clone();

    // `max_rounds` is a budget for this invocation, not a lifetime cap on the
    // pull request. Running spar again on a PR that already spent its rounds is
    // a deliberate act by a person who has looked at it, so it gets a fresh
    // budget rather than an error telling them to raise a number they cannot
    // see from the outside.
    let (first, last_allowed) = round_window(ctx.start_round, cfg.loop_cfg.max_rounds);
    let mut last_round = first.saturating_sub(1);

    for round in first..=last_allowed {
        last_round = round;
        state.rounds = round;
        let reviewer = agent::find(agents, &holder)?;
        let effort = cfg.effort_for_round(&reviewer.spec, round);
        log!(
            "{}: round {round}, {holder} reviewing ({})",
            ctx.label,
            effort.as_deref().unwrap_or("default effort")
        );

        let prompt = REVIEW_PROMPT
            .replace("{base}", &base)
            .replace("{number}", &ctx.subject.to_string())
            .replace("{title}", &ctx.title)
            .replace("{settled}", &settled_block(ledger));
        let before_review = snapshot(repo, &ctx.work_dir);
        let review: Review = reviewer.review(
            &base,
            &prompt,
            &schema::review(),
            &ctx.work_dir,
            effort.as_deref(),
        )?;

        // Who actually wrote the head this round, which is the only thing that
        // decides who reviews it next. None so far: a review is not supposed to
        // write anything.
        let mut editor: Option<String> = None;
        let review_wrote = snapshot(repo, &ctx.work_dir) != before_review;
        if review_wrote {
            logwarn!(
                "{}: {holder} changed the branch while reviewing it, which the review prompt \
                 forbids. Rolling it back.",
                ctx.label
            );
            if undo_edits(repo, &ctx.work_dir, &before_review).head != before_review.head {
                state
                    .notes
                    .push(format!("{holder} committed during its own review"));
                editor = Some(holder.clone());
            }
        }

        let blocking: Vec<Finding> = review
            .findings
            .iter()
            .filter(|f| f.blocks())
            .cloned()
            .collect();

        if repo.style.pr_comments == PrComments::Rounds {
            if let Err(e) = repo.comment_pr(
                ctx.pr_number,
                &review_comment(&holder, round, &review, &repo.style),
            ) {
                logdim!("could not post the review comment: {e}");
            }
        }

        // Filed every round, not only on approval: a run that escalates or runs
        // out of rounds would otherwise drop these on the floor. Filing
        // deduplicates by title, so repeats across rounds are free.
        file_out_of_scope(repo, &review.findings, ctx.subject, state, cfg);
        file_nonblocking(repo, &review.findings, ctx.subject, state, cfg);

        if check_relitigation(ledger, &blocking, state) {
            state.status = Status::Escalated;
            post_outcome(
                repo,
                ctx.pr_number,
                state,
                ledger,
                Ending::Deadlocked(&blocking),
            );
            persist(repo, ctx.pr_number, state, ledger, round, &holder);
            return Ok(());
        }

        if approval_stands(&blocking, review_wrote) {
            state.status = Status::Approved;
            post_outcome(repo, ctx.pr_number, state, ledger, Ending::Approved);
            persist(repo, ctx.pr_number, state, ledger, round, &holder);
            // Before the merge, not after: a draft cannot be merged, and the
            // state the draft was signalling, that two agents were still
            // arguing about it, has just stopped being true.
            if cfg.loop_cfg.drafts == Drafts::UntilApproved && repo.mark_ready(ctx.pr_number) {
                log!("{}: out of draft", ctx.label);
            }
            if cfg.loop_cfg.auto_merge {
                // Release the worktree first. `gh pr merge --delete-branch`
                // fails if anything still has the branch checked out, and it
                // fails *after* merging, so the merge lands while the command
                // reports failure.
                ctx.release(repo);
                repo.merge_pr(ctx.pr_number)?;
                state.status = Status::Merged;
                repo.clear_state(ctx.pr_number); // nothing left to resume
                log!("{}: merged", ctx.label);
            } else {
                log!("{}: approved, awaiting human merge", ctx.label);
            }
            return Ok(());
        }

        if blocking.is_empty() {
            // Nothing blocking, but the branch it said that about is not the
            // branch that is there now. Falling through gives the next round
            // whatever the rollback left: the same reviewer when it took, the
            // other agent when the review's commit survived it.
            logwarn!(
                "{}: {holder} found nothing blocking on a branch it had changed itself, so the \
                 approval does not carry.",
                ctx.label
            );
            state.notes.push(format!(
                "{holder} passed the branch in round {round} after editing it; the edit was rolled \
                 back and the approval did not stand"
            ));
        } else if review.next_action == NextAction::FixMyself {
            log!("{}: {holder} fixing its own findings", ctx.label);
            let prompt = FIX_PROMPT.replace("{findings}", &findings_for_prompt(&blocking));
            let before_fix = snapshot(repo, &ctx.work_dir);
            reviewer.ask(&prompt, &ctx.work_dir, effort.as_deref())?;
            match editor_after(repo, &ctx.work_dir, &before_fix, &ctx.label, &holder) {
                Some(who) => editor = Some(who),
                None => {
                    // Handing over here is what the bug was: the head is still
                    // the author's, so the author would be reading its own work.
                    logwarn!(
                        "{}: {holder} said it would fix its own findings and committed nothing, \
                         so it keeps the pull request.",
                        ctx.label
                    );
                    state.notes.push(format!(
                        "{holder} chose to fix its own findings in round {round} and committed \
                         nothing"
                    ));
                }
            }
        } else {
            let author_name = cfg.other(&holder);
            let author = agent::find(agents, &author_name)?;
            log!(
                "{}: handing {} finding(s) to {author_name}",
                ctx.label,
                blocking.len()
            );
            let prompt = RESPOND_PROMPT
                .replace("{number}", &ctx.subject.to_string())
                .replace("{findings}", &findings_for_prompt(&blocking));
            let before_response = snapshot(repo, &ctx.work_dir);
            let response: ResponseDoc = author.ask_json(
                &prompt,
                &schema::response(),
                &ctx.work_dir,
                cfg.effort_for_round(&author.spec, round).as_deref(),
            )?;
            if let Some(who) = editor_after(
                repo,
                &ctx.work_dir,
                &before_response,
                &ctx.label,
                &author_name,
            ) {
                editor = Some(who);
            } else if response
                .dispositions
                .iter()
                .any(|d| d.action == Action::Fixed)
            {
                logwarn!(
                    "{}: {author_name} reported fixes but committed nothing, so the diff does not \
                     have them.",
                    ctx.label
                );
            }
            apply_dispositions(
                repo,
                cfg,
                &response,
                &blocking,
                ledger,
                state,
                round,
                ctx.subject,
                ctx.pr_number,
                &author_name,
            );
        }

        if editor.is_some() {
            repo.rewrite_commits_if_needed(&ctx.work_dir, &base)?;
            repo.push(&ctx.work_dir, &ctx.branch)?;
        }
        holder = next_reviewer(cfg, &holder, editor.as_deref());
        persist(repo, ctx.pr_number, state, ledger, round, &holder);
    }

    state.status = Status::Escalated;
    state
        .notes
        .push(exhausted_note(ctx.start_round, last_round));
    post_outcome(repo, ctx.pr_number, state, ledger, Ending::OutOfRounds);
    persist(repo, ctx.pr_number, state, ledger, last_round, &holder);
    Ok(())
}

/// Whether a review with nothing blocking can end the run.
///
/// A review that wrote to the branch judged a tree the rollback then takes
/// away, so "nothing blocking" was said about code that is not there any more:
/// a reviewer that quietly fixes what it finds and reports clean would merge
/// the defect it fixed. Another round on the restored branch is cheaper than
/// that.
fn approval_stands(blocking: &[Finding], review_wrote: bool) -> bool {
    blocking.is_empty() && !review_wrote
}

/// Who reviews the next round: never the agent that wrote the head it will
/// read.
///
/// `editor` is whoever moved HEAD this round, observed rather than inferred
/// from `next_action`. The two came apart in both directions: a `fix_myself`
/// call that returned without committing handed the author its own commit back,
/// and a reviewer that committed during `hand_back` kept a PR whose head it had
/// written.
///
/// Nothing landing at all leaves the head with the author, which by this rule's
/// own invariant is not the reviewer, so the reviewer keeps the pull request and
/// reads the same commit again.
fn next_reviewer(cfg: &Config, reviewer: &str, editor: Option<&str>) -> String {
    match editor {
        Some(editor) => cfg.other(editor),
        None => reviewer.to_string(),
    }
}

/// Who wrote the head after a call that was asked to commit, if anybody did.
///
/// A call that returns successfully is not evidence of a commit, and custody is
/// decided on this answer, so it is read from git rather than taken from the
/// agent's word for it. Anything it left uncommitted goes the same way as a
/// review's edits, and for the same reason: the round it hands over is the diff
/// on the branch, not the state of somebody's checkout.
fn editor_after(
    repo: &Repo,
    work_dir: &Path,
    before: &Snapshot,
    label: &str,
    who: &str,
) -> Option<String> {
    let after = snapshot(repo, work_dir);
    if after.dirty {
        logwarn!(
            "{label}: {who} left tracked files uncommitted. Only commits are pushed and the next \
             review reads the tree, so they are discarded."
        );
        drop_uncommitted(repo, work_dir);
    }
    after.landed_over(before).then(|| who.to_string())
}

/// The inclusive range of round numbers this invocation will work through.
///
/// Round numbers keep counting up across sessions so the ledger and the PR
/// history stay coherent, while the budget resets each time a person chooses to
/// run spar again.
fn round_window(start_round: u32, budget: u32) -> (u32, u32) {
    (start_round, start_round + budget.saturating_sub(1))
}

/// How many rounds this invocation spent, and how many the PR has seen in
/// total. A resumed PR that stops at round 8 did not have 8 rounds of budget,
/// and saying so would misreport both the cost and the history.
fn spent(start_round: u32, last_round: u32) -> (u32, u32) {
    (last_round.saturating_sub(start_round) + 1, last_round)
}

fn exhausted_note(start_round: u32, last_round: u32) -> String {
    let (this_run, total) = spent(start_round, last_round);
    if this_run == total {
        format!("no convergence after {this_run} rounds")
    } else {
        format!("no convergence after {this_run} more rounds ({total} in total)")
    }
}

fn persist(
    repo: &Repo,
    pr_number: i64,
    state: &IssueRun,
    ledger: &Ledger,
    round: u32,
    next_actor: &str,
) {
    let payload = PersistedState {
        version: STATE_VERSION,
        round,
        next_actor: next_actor.to_string(),
        status: state.status,
        ledger: ledger.clone(),
        filed: state.filed.clone(),
    };
    if let Err(e) = repo.write_state(pr_number, &payload) {
        logdim!("could not persist state for PR #{pr_number}: {e}");
    }
}

// ---------------------------------------------------------------------------
// The ledger
// ---------------------------------------------------------------------------

fn settled_block(ledger: &Ledger) -> String {
    if ledger.is_empty() {
        return String::new();
    }
    let lines: Vec<String> = ledger
        .values()
        .map(|e| match e.outcome {
            Settled::Refuted => format!("- {}: refuted because {}", e.title, e.reasoning),
            Settled::Filed => format!(
                "- {}: out of scope here, and filed. {}",
                e.title, e.reasoning
            ),
            Settled::Dropped => format!(
                "- {}: out of scope here, and not filed. {}",
                e.title, e.reasoning
            ),
        })
        .collect();
    format!(
        "\nThe following points were already raised and settled, by a refutation or by a \
         follow-up issue. Treat them as settled. Do not raise them again unless you have new \
         evidence:\n{}",
        lines.join("\n")
    )
}

/// Record a point as settled, keeping any re-raise count it already carries.
/// Answering the same point a second time does not reset the argument, and
/// zeroing the count here would put the escalation guard out of reach: the
/// count is spent every round and rebuilt from nothing every round.
fn settle(ledger: &mut Ledger, key: String, entry: LedgerEntry) {
    let reraised = ledger.get(&key).map(|e| e.reraised).unwrap_or(0);
    ledger.insert(key, LedgerEntry { reraised, ..entry });
}

/// A settled point raised twice more goes to a person rather than looping
/// forever.
fn check_relitigation(ledger: &mut Ledger, blocking: &[Finding], state: &mut IssueRun) -> bool {
    let mut escalate = false;
    for finding in blocking {
        let key = finding_key(&finding.title, &finding.file);
        if let Some(entry) = ledger.get_mut(&key) {
            entry.reraised += 1;
            if entry.reraised >= 2 {
                state.notes.push(format!(
                    "'{}' was settled and re-raised twice; escalating.",
                    finding.title
                ));
                escalate = true;
            }
        }
    }
    escalate
}

fn normalise(text: &str) -> String {
    text.to_lowercase()
        .chars()
        .filter(|c| c.is_ascii_alphanumeric() || c.is_whitespace())
        .collect::<String>()
        .split_whitespace()
        .collect::<Vec<_>>()
        .join(" ")
}

/// Match a disposition back to the finding it answers, so the ledger key it
/// records is the same key the next round's finding will hash to. Without this
/// the re-litigation guard is dead code for any finding that names a file.
/// Whether two titles name the same point, ignoring wording noise.
pub(crate) fn same_point(a: &str, b: &str) -> bool {
    normalise(a) == normalise(b)
}

fn matching_finding<'a>(findings: &'a [Finding], title: &str) -> Option<&'a Finding> {
    let wanted = normalise(title);
    findings.iter().find(|f| normalise(&f.title) == wanted)
}

#[allow(clippy::too_many_arguments)]
fn apply_dispositions(
    repo: &Repo,
    cfg: &Config,
    response: &ResponseDoc,
    blocking: &[Finding],
    ledger: &mut Ledger,
    state: &mut IssueRun,
    round: u32,
    subject: i64,
    pr_number: i64,
    author: &str,
) {
    let mut fixed = Vec::new();
    let mut refuted = Vec::new();
    let mut filed = Vec::new();

    for d in &response.dispositions {
        let source = matching_finding(blocking, &d.title);
        let file = source
            .map(|f| f.file.clone())
            .filter(|f| !f.trim().is_empty())
            .unwrap_or_else(|| d.file.clone());
        // Hash the *reviewer's* wording, not the author's. `matching_finding`
        // is deliberately looser than `finding_key` (it ignores hyphens, dots,
        // slashes, and underscores), so an author who writes "multibyte" where
        // the reviewer wrote "multi-byte" matches here and yet hashes to a
        // different key. Recording that key means next round's lookup misses
        // and the re-litigation guard tracks nothing at all.
        let canonical = source.map(|f| f.title.as_str()).unwrap_or(d.title.as_str());
        let title = style::title(canonical, &repo.style);

        match d.action {
            Action::Refuted => {
                let reasoning = style::summary(&d.reasoning, &repo.style);
                settle(
                    ledger,
                    finding_key(canonical, &file),
                    LedgerEntry {
                        title: title.clone(),
                        file: file.clone(),
                        reasoning: reasoning.clone(),
                        round,
                        reraised: 0,
                        outcome: Settled::Refuted,
                    },
                );
                state.disputes.push(Dispute {
                    title: title.clone(),
                    reasoning: reasoning.clone(),
                });
                refuted.push(format!("{title}. {reasoning}"));
            }
            Action::FiledIssue => {
                let new_title = d
                    .new_issue_title
                    .clone()
                    .filter(|t| !t.trim().is_empty())
                    .unwrap_or_else(|| d.title.clone());
                let new_body = d
                    .new_issue_body
                    .clone()
                    .filter(|b| !b.trim().is_empty())
                    .unwrap_or_else(|| d.reasoning.clone());
                let recorded = file_followup(repo, &new_title, &new_body, subject, cfg, state);
                if let Some(url) = recorded.url() {
                    state.filed.push(url.to_string());
                    filed.push(url.to_string());
                }
                // Settled like a refutation, because it ends the same way: the
                // code will not change for this point on this branch. Without
                // the entry the reviewer keeping the PR raises it again next
                // round, the author files a duplicate, and the round budget
                // goes on one point nobody disagrees about.
                //
                // Unless nothing holds the point, in which case there is no
                // entry to write: see `filed_entry`.
                let Some((outcome, reasoning)) =
                    filed_entry(&recorded, &style::summary(&d.reasoning, &repo.style))
                else {
                    logwarn!(
                        "'{title}' was not recorded anywhere, so it stays open for the next round"
                    );
                    continue;
                };
                settle(
                    ledger,
                    finding_key(canonical, &file),
                    LedgerEntry {
                        title: title.clone(),
                        file: file.clone(),
                        reasoning,
                        round,
                        reraised: 0,
                        outcome,
                    },
                );
            }
            Action::Fixed => fixed.push(title),
        }
    }

    if repo.style.pr_comments == PrComments::Rounds {
        let comment = disposition_comment(author, response, &fixed, &refuted, &filed, &repo.style);
        if let Some(text) = comment {
            if let Err(e) = repo.comment_pr(pr_number, &text) {
                logdim!("could not post the disposition comment: {e}");
            }
        }
    }
}

/// What the ledger should say about a point the author moved out of this pull
/// request, and whether it should say anything at all.
///
/// Nothing, for a follow-up that failed. An entry tells every later round the
/// point was dealt with, and it outlives the run: recording one for a write
/// that never happened suppresses a real defect for good, on the strength of a
/// transient error.
fn filed_entry(recorded: &Followup, reasoning: &str) -> Option<(Settled, String)> {
    let (outcome, tail) = match recorded {
        Followup::Recorded(reference) => (
            Settled::Filed,
            format!("Tracked in {}.", as_reference(reference)),
        ),
        Followup::Covered(reference) => (
            Settled::Filed,
            format!("Already covered by {}.", as_reference(reference)),
        ),
        Followup::Dropped(why) => (Settled::Dropped, format!("Not filed anywhere: {why}.")),
        Followup::Failed => return None,
    };
    let reasoning = match reasoning.trim() {
        "" => tail,
        said => format!("{said} {tail}"),
    };
    Some((outcome, reasoning))
}

// ---------------------------------------------------------------------------
// Follow-ups
// ---------------------------------------------------------------------------

/// Record a finding that is real but out of scope for this PR.
///
/// On your own repository an issue is the right home. On a large repository
/// that is not yours it is somebody else's notification and somebody else's
/// triage queue, so `local` keeps the same information in `.spar/followups.md`
/// and `none` drops it.
///
/// The answer says which of those happened, because the caller settles the
/// point on it. A failure and a deliberate drop look identical from the outside
/// and mean opposite things to the next round.
pub fn file_followup(
    repo: &Repo,
    title: &str,
    body: &str,
    source: i64,
    cfg: &Config,
    state: &IssueRun,
) -> Followup {
    if repo.followups == Followups::None {
        return Followup::Dropped("follow-ups are off for this repository");
    }
    // A backstop against a run that will not stop finding things. Silent
    // truncation is not on offer: what was dropped is said out loud.
    if state.filed.len() >= cfg.loop_cfg.max_followups {
        logwarn!(
            "already recorded {} follow-ups, not recording '{}'. Raise max_followups if you want \
             them all.",
            state.filed.len(),
            style::title(title, &repo.style)
        );
        return Followup::Dropped("this run had already recorded as many follow-ups as it may");
    }
    // The exact string that will land on GitHub. Searching for anything else
    // means the duplicate check can never hit, and every round files another
    // copy of the same follow-up.
    //
    // A title the style gate cannot clean is a failure rather than a drop: the
    // next round words the point differently, and that wording may pass.
    let title = match repo.clean_title(title) {
        Ok(title) => title,
        Err(e) => {
            logdim!("could not clean a follow-up title: {e}");
            return Followup::Failed;
        }
    };
    if title.trim().is_empty() {
        logdim!("nothing left of a follow-up title after cleaning it");
        return Followup::Failed;
    }
    // Not style::body: that is the budget for a pull request comment, read with
    // the diff in front of you. This is a work item somebody picks up cold.
    let body = format!(
        "{}\n\nFound while working on #{source}.",
        style::issue_body(body, &repo.style)
    );

    if repo.followups == Followups::Local {
        return repo.append_local_followup(&title, &body);
    }

    match file_as_issue(repo, &title, &body) {
        Ok(filed) => filed.into(),
        Err(e) => {
            logdim!("could not file a follow-up for '{title}': {e}");
            Followup::Failed
        }
    }
}

/// What happened to one finding on the way to the tracker.
#[derive(Debug, Clone)]
pub enum Filed {
    /// A new issue.
    Opened(i64, String),
    /// An open issue already covered it, and this pass had something to add.
    AddedTo(i64, String),
    /// An open issue already covered it, and this pass added nothing.
    Covered(i64, String),
    /// A closed issue already covered it. Nothing was written.
    AlreadyClosed(i64, String),
}

impl From<Filed> for Followup {
    fn from(filed: Filed) -> Self {
        match filed {
            Filed::Opened(_, url) | Filed::AddedTo(_, url) | Filed::Covered(_, url) => {
                Followup::Recorded(url)
            }
            // Covered rather than recorded: the point is genuinely tracked, so
            // raising it again is waste, but the issue holding it is closed and
            // must not be handed out as work.
            Filed::AlreadyClosed(_, url) => Followup::Covered(url),
        }
    }
}

impl Filed {
    pub fn url(&self) -> Option<&str> {
        match self {
            Filed::Opened(_, url) | Filed::AddedTo(_, url) | Filed::Covered(_, url) => Some(url),
            // The work is done and closed. Reporting it as filed would put it
            // back into a wave to be implemented again.
            Filed::AlreadyClosed(_, _) => None,
        }
    }

    /// The issue to work, when there is one to work.
    pub fn number(&self) -> Option<i64> {
        match self {
            Filed::Opened(n, _) | Filed::AddedTo(n, _) | Filed::Covered(n, _) => Some(*n),
            Filed::AlreadyClosed(_, _) => None,
        }
    }

    /// One clause saying where it went, for a log line or an archive entry.
    pub fn note(&self) -> String {
        match self {
            Filed::Opened(n, _) => format!("#{n}"),
            Filed::AddedTo(n, _) => format!("added to #{n}"),
            Filed::Covered(n, _) => format!("#{n} already says this"),
            Filed::AlreadyClosed(n, _) => format!("#{n} covers it and is closed"),
        }
    }

    pub fn describe(&self, title: &str) -> String {
        let title = style::clip(title.trim(), 80);
        match self {
            Filed::Opened(n, _) => format!("filed #{n}: {title}"),
            Filed::AddedTo(n, _) => format!("added to #{n}: {title}"),
            Filed::Covered(n, _) => format!("#{n} already says this: {title}"),
            Filed::AlreadyClosed(n, _) => format!("#{n} covers it and is closed: {title}"),
        }
    }
}

/// File an issue, or add to the one that already covers it.
///
/// Exact title matching let duplicates through: two agents, or two runs a week
/// apart, never word one defect identically, and a real run filed two that had
/// to be closed by hand. Filing a second copy is the complaint; silently
/// dropping the new wording is not much better, because a later pass often
/// carries evidence the first did not.
///
/// The title arrives cleaned by the caller, and it has to: searching for
/// anything but the exact string that will land on GitHub means the duplicate
/// check can never hit.
pub fn file_as_issue(repo: &Repo, title: &str, body: &str) -> Result<Filed> {
    let title = repo.clean_title(title)?;
    if title.trim().is_empty() {
        return Err(spar_err!("nothing left of the title after cleaning it"));
    }
    if let Some(existing) = repo.find_similar_issue(&title, body) {
        let known = format!("{} {}", existing.title, existing.body);
        if !existing.open {
            return Ok(Filed::AlreadyClosed(existing.number, existing.url));
        }
        if crate::textsim::adds_information(body, &known) {
            repo.comment_issue(existing.number, body)?;
            return Ok(Filed::AddedTo(existing.number, existing.url));
        }
        return Ok(Filed::Covered(existing.number, existing.url));
    }
    let url = repo.create_issue(&title, body)?;
    let number = filed_issue_number(&url)
        .ok_or_else(|| spar_err!("filed an issue but could not read its number from {url}"))?;
    Ok(Filed::Opened(number, url))
}

fn file_out_of_scope(
    repo: &Repo,
    findings: &[Finding],
    subject: i64,
    state: &mut IssueRun,
    cfg: &Config,
) {
    for finding in findings.iter().filter(|f| !f.in_scope) {
        let body = issue_report(finding);
        let recorded = file_followup(repo, &finding.title, &body, subject, cfg, state);
        if let Some(url) = recorded.url() {
            state.filed.push(url.to_string());
        }
    }
}

/// A finding written as a bug report, when it carries the parts of one.
///
/// The thread gets one line; an issue gets the whole thing under headings, in
/// the order somebody reads a bug report: what is wrong, how to see it, what it
/// costs, what it should do instead. A finding with none of those falls back to
/// its detail, which is every finding that was never going to be filed.
pub fn issue_report(finding: &Finding) -> String {
    let sections = finding.report_sections();
    if sections.is_empty() {
        return finding.detail.clone();
    }
    let mut out: Vec<String> = sections
        .iter()
        .map(|(heading, text)| format!("## {heading}\n\n{text}"))
        .collect();
    // Keep the one line summary when it says something the sections do not,
    // rather than dropping it or repeating it.
    if !finding.detail.trim().is_empty()
        && !sections
            .iter()
            .any(|(_, text)| crate::textsim::same_point(text, &finding.detail))
    {
        out.insert(0, finding.detail.trim().to_string());
    }
    out.join("\n\n")
}

/// Non-blocking findings become follow-ups so they do not gate the merge.
///
/// Nits are excluded by default. On a shared repository a filed nit is somebody
/// else's notification and somebody else's triage queue: an early run on a
/// production codebase opened an issue titled "Log wording". Worth saying in
/// the PR thread, not worth an issue.
fn file_nonblocking(
    repo: &Repo,
    findings: &[Finding],
    subject: i64,
    state: &mut IssueRun,
    cfg: &Config,
) {
    for finding in findings {
        let keep = match finding.severity {
            Severity::NonBlocking => cfg.loop_cfg.file_non_blocking,
            Severity::Nit => cfg.loop_cfg.file_nits,
            Severity::Blocking => false,
        };
        if !keep || !finding.in_scope {
            continue;
        }
        let recorded = file_followup(repo, &finding.title, &finding.detail, subject, cfg, state);
        if let Some(url) = recorded.url() {
            state.filed.push(url.to_string());
        }
    }
}

// ---------------------------------------------------------------------------
// What a human actually reads
// ---------------------------------------------------------------------------
//
// spar composes every comment itself from structured fields, rather than
// forwarding whatever prose a model produced. That is the only reliable way to
// keep a PR thread readable: the model supplies facts, the harness supplies the
// shape, and each field is held to a budget on the way out.

fn bullets(lines: &[String]) -> String {
    lines
        .iter()
        .map(|l| format!("- {l}"))
        .collect::<Vec<_>>()
        .join("\n")
}

fn located(finding: &Finding, style: &Style) -> String {
    let title = style::title(&finding.title, style);
    match finding.where_at() {
        "general" => title,
        file => format!("{title} ({file})"),
    }
}

/// How the run ended, which is the only thing about the run a reader needs.
pub enum Ending<'a> {
    /// Nothing blocks a merge.
    Approved,
    /// The round budget ran out. The last round's fixes were pushed but never
    /// reviewed, which is the part a maintainer has to know.
    OutOfRounds,
    /// A point was refuted and raised again anyway. Nobody is going to break
    /// the tie but a person.
    Deadlocked(&'a [Finding]),
}

/// Post the one comment a run leaves behind, if it has anything to say.
///
/// Everything spar used to write here was an account of its own working: which
/// agent spoke, which round it was, how many findings of each severity, that it
/// had stopped. None of that is about the code. Worse, the running commentary
/// could contradict itself, ending a thread with "5 fixed" immediately followed
/// by "no convergence", which reads as a failure rather than as fixes nobody
/// has checked yet.
///
/// So the loop is silent and this says what is left: what is unresolved, what
/// was argued down, and where the follow-ups went.
pub fn post_outcome(
    repo: &Repo,
    pr_number: i64,
    state: &IssueRun,
    ledger: &Ledger,
    ending: Ending<'_>,
) {
    if repo.style.pr_comments != PrComments::Outcome {
        return;
    }
    let Some(text) = outcome_comment(state, ledger, &ending, &repo.style) else {
        return;
    };
    if let Err(e) = repo.comment_pr(pr_number, &text) {
        logdim!("could not post the outcome comment: {e}");
    }
}

/// How a point was settled and why: this run's disputes first, then the ledger,
/// which is what survives across a resume.
fn settled_as(finding: &Finding, state: &IssueRun, ledger: &Ledger) -> Option<(Settled, String)> {
    if let Some(d) = state
        .disputes
        .iter()
        .find(|d| same_point(&d.title, &finding.title))
    {
        if !d.reasoning.trim().is_empty() {
            return Some((Settled::Refuted, d.reasoning.clone()));
        }
    }
    ledger
        .get(&finding_key(&finding.title, &finding.file))
        .filter(|entry| !entry.reasoning.trim().is_empty())
        .map(|entry| (entry.outcome, entry.reasoning.clone()))
}

/// `#123` from a filed issue URL, falling back to the URL when it does not look
/// like one. Shorter, and GitHub renders it as a link either way.
/// The issue number a filed follow-up URL points at, when it is one. Local
/// notes and anything unparseable yield nothing.
pub fn filed_issue_number(filed: &str) -> Option<i64> {
    filed
        .rsplit('/')
        .next()
        .and_then(|tail| tail.parse::<i64>().ok())
        .filter(|n| *n > 0)
}

fn as_reference(url: &str) -> String {
    match url.rsplit('/').next().and_then(|n| n.parse::<u64>().ok()) {
        Some(number) => format!("#{number}"),
        None => url.to_string(),
    }
}

pub fn outcome_comment(
    state: &IssueRun,
    ledger: &Ledger,
    ending: &Ending<'_>,
    style: &Style,
) -> Option<String> {
    let mut out: Vec<String> = Vec::new();
    // Points rendered in the deadlock block, so the refutation list below does
    // not print the same title a second time.
    let mut already: Vec<String> = Vec::new();

    match ending {
        Ending::Approved => {
            if state.disputes.is_empty() && state.filed.is_empty() {
                // A clean approval with nothing outstanding needs no comment.
                // The absence of objections is the message.
                return None;
            }
            out.push("Reviewed, nothing blocking a merge.".into());
        }
        Ending::OutOfRounds => out.push(
            "Not signed off: the last round of fixes was pushed but has not been reviewed.".into(),
        ),
        Ending::Deadlocked(points) => {
            // Rendered once, with the argument attached. A deadlocked point is
            // by definition one that was settled earlier, so the reasoning is
            // the whole reason a person is being asked to look. On a resumed
            // run `state.disputes` is empty (only `filed` is restored), so the
            // ledger is the only place that argument survives.
            let lines: Vec<String> = points
                .iter()
                .map(|f| {
                    let where_at = match f.where_at() {
                        "general" => String::new(),
                        file => format!(" ({file})"),
                    };
                    let title = style::title(&f.title, style);
                    already.push(title.clone());
                    match settled_as(f, state, ledger) {
                        Some((Settled::Refuted, reason)) => format!(
                            "{title}{where_at}. Refuted as: {}",
                            style::summary(&reason, style)
                        ),
                        Some((Settled::Filed, reason)) => format!(
                            "{title}{where_at}. Filed as out of scope: {}",
                            style::summary(&reason, style)
                        ),
                        // Never "filed": nothing holds this point but the
                        // comment you are reading.
                        Some((Settled::Dropped, reason)) => format!(
                            "{title}{where_at}. Out of scope here, and not filed: {}",
                            style::summary(&reason, style)
                        ),
                        None => format!("{title}{where_at}"),
                    }
                })
                .collect();
            out.push("Needs your decision. The reviewers could not settle this:".into());
            out.push(bullets(&lines));
        }
    }

    let disputes: Vec<&crate::model::Dispute> = state
        .disputes
        .iter()
        .filter(|d| !already.iter().any(|t| same_point(t, &d.title)))
        .collect();
    if !disputes.is_empty() {
        // The one thing invisible anywhere else. The diff shows what was fixed;
        // nothing shows what was argued down, or why.
        let lines: Vec<String> = disputes
            .iter()
            .map(|d| {
                format!(
                    "{}. {}",
                    style::title(&d.title, style),
                    style::sentence(&d.reasoning, style)
                )
            })
            .collect();
        out.push(format!("Raised and refuted:\n{}", bullets(&lines)));
    }

    if !state.filed.is_empty() {
        let refs: Vec<String> = state.filed.iter().map(|u| as_reference(u)).collect();
        out.push(format!("Filed separately: {}", refs.join(", ")));
    }

    Some(out.join("\n\n"))
}

/// What the implementor is asked, with the issue in front of it.
///
/// The body is passed rather than only the link, because one of the two agents
/// cannot follow a link: codex runs under `-s workspace-write`, which has no
/// network at all, so a URL alone would leave it judging the title. The link is
/// there for the agent that can follow it, and for the comments spar does not
/// fetch.
fn implement_prompt(number: i64, title: &str, url: &str, body: &str) -> String {
    IMPLEMENT_PROMPT
        .replace("{number}", &number.to_string())
        .replace("{title}", title)
        .replace("{url}", url)
        .replace("{body}", body)
}

/// The pull request body.
///
/// What it closes, then the change in one sentence, then what was wrong, then
/// only the sections that have something in them. The lead is two paragraphs
/// rather than two headings: a heading over a single sentence is a label on a
/// label, and those two parts are the ones every body has.
///
/// GitHub renders the file count and the plus and minus figures immediately
/// above this, so neither appears here.
pub fn pr_body(issue: i64, work: &Implementation, style: &Style) -> String {
    let mut parts = vec![format!("Closes #{issue}")];

    for lead in [&work.summary, &work.problem] {
        let text = style::sentence(lead, style);
        if !text.is_empty() {
            parts.push(text);
        }
    }
    parts.extend(section("What changed", &work.changes, style));
    parts.extend(section("How to test", &work.testing, style));

    let notes = style::sentence(work.notes.as_deref().unwrap_or_default(), style);
    if !notes.is_empty() {
        parts.push(format!("## Notes\n\n{notes}"));
    }

    style::body(&parts.join("\n\n"), style)
}

/// A headed list, or nothing at all when there is nothing to list.
///
/// Nothing at all on purpose. A heading with an empty body under it reads as a
/// section somebody forgot to write, which is worse than the absence, and a
/// small change that needs no change list should not be made to look like one
/// that is missing its.
fn section(heading: &str, lines: &[String], style: &Style) -> Option<String> {
    let items: Vec<String> = lines
        .iter()
        .map(|line| style::summary(line, style))
        .filter(|line| !line.is_empty())
        .collect();
    if items.is_empty() {
        return None;
    }
    Some(format!("## {heading}\n\n{}", bullets(&items)))
}

/// A pull request body for work whose author never got to describe it.
///
/// The implement call failed after the commits were made, so what those commits
/// say about themselves is the only account of them there is. It is a poor one,
/// and better than an empty body over work nobody would otherwise know was
/// there; the note says as much, so a reviewer does not read the list as the
/// author's own summary.
pub fn from_commits(repo: &Repo, work_dir: &Path, base: &str) -> Implementation {
    Implementation {
        changes: repo.commit_subjects(work_dir, "HEAD", base),
        notes: Some(
            "The implement call failed after these commits were made, so this body is assembled \
             from their messages rather than written by their author. Read the diff."
                .to_string(),
        ),
        ..Implementation::default()
    }
}

/// What gets posted on an issue that produced no pull request.
///
/// The agent's own reason when it gave one, since that is the part written for
/// the person who opened the issue. Never the summary: an issue that produced
/// no commits has no change for a summary to describe, and one that claims
/// otherwise is worse than a flat sentence saying nothing happened.
fn no_pr_note(work: &Implementation, style: &Style) -> String {
    let reason = style::sentence(&work.reason, style);
    if !reason.is_empty() {
        return reason;
    }
    if work.not_worth_doing {
        "Left alone after reading the code, with no reason given.".to_string()
    } else {
        "Nothing was committed, so there is nothing to review.".to_string()
    }
}

/// One review, as a reviewer would write it if they were in a hurry: a count
/// line, a sentence, and one bullet per finding. Only blocking findings carry
/// their detail, because only those are something the author has to act on now.
pub fn review_comment(holder: &str, round: u32, review: &Review, style: &Style) -> String {
    let by = |severity: Severity| -> Vec<&Finding> {
        review
            .findings
            .iter()
            .filter(|f| f.severity == severity && f.in_scope)
            .collect()
    };
    let blocking = by(Severity::Blocking);
    let non_blocking = by(Severity::NonBlocking);
    let nits = by(Severity::Nit);
    let out_of_scope: Vec<&Finding> = review.findings.iter().filter(|f| !f.in_scope).collect();

    let mut counts = Vec::new();
    if !blocking.is_empty() {
        counts.push(format!("{} blocking", blocking.len()));
    }
    if !non_blocking.is_empty() {
        counts.push(format!("{} non-blocking", non_blocking.len()));
    }
    if !nits.is_empty() {
        counts.push(format!("{} nit", nits.len()));
    }
    if !out_of_scope.is_empty() {
        counts.push(format!("{} out of scope", out_of_scope.len()));
    }
    let headline = if counts.is_empty() {
        "no findings".to_string()
    } else {
        counts.join(", ")
    };

    let _ = (holder, round, headline);
    let mut out = Vec::new();
    let summary = style::summary(&review.summary, style);
    if !summary.is_empty() {
        out.push(summary);
    }

    if !blocking.is_empty() {
        let lines: Vec<String> = blocking
            .iter()
            .map(|f| {
                let detail = style::detail(&f.detail, style);
                if detail.is_empty() {
                    located(f, style)
                } else {
                    format!("{}. {detail}", located(f, style))
                }
            })
            .collect();
        out.push(format!("blocking\n{}", bullets(&lines)));
    }

    // Everything below is filed as a follow-up, so the thread only needs the
    // title: the detail lives on the issue where it can be acted on.
    for (label, group) in [
        ("non-blocking", &non_blocking),
        ("nits", &nits),
        ("out of scope", &out_of_scope),
    ] {
        if group.is_empty() {
            continue;
        }
        let lines: Vec<String> = group.iter().map(|f| located(f, style)).collect();
        out.push(format!("{label}\n{}", bullets(&lines)));
    }

    out.join("\n\n")
}

/// One response to a review. Refutations carry their reasoning because that is
/// the whole argument; fixes are a list of titles because the diff says the
/// rest.
pub fn disposition_comment(
    author: &str,
    response: &ResponseDoc,
    fixed: &[String],
    refuted: &[String],
    filed: &[String],
    style: &Style,
) -> Option<String> {
    if fixed.is_empty() && refuted.is_empty() && filed.is_empty() {
        return None;
    }
    let mut counts = Vec::new();
    if !fixed.is_empty() {
        counts.push(format!("{} fixed", fixed.len()));
    }
    if !refuted.is_empty() {
        counts.push(format!("{} refuted", refuted.len()));
    }
    if !filed.is_empty() {
        counts.push(format!("{} filed", filed.len()));
    }

    let _ = (author, counts);
    let mut out = Vec::new();
    let summary = style::summary(&response.summary, style);
    if !summary.is_empty() {
        out.push(summary);
    }
    if !refuted.is_empty() {
        out.push(format!("refuted\n{}", bullets(refuted)));
    }
    if !fixed.is_empty() {
        out.push(format!("fixed\n{}", bullets(fixed)));
    }
    if !filed.is_empty() {
        out.push(format!("filed\n{}", bullets(filed)));
    }
    Some(out.join("\n\n"))
}

/// What is posted on an issue both agents declined.
/// What is posted on an issue both reviewers declined.
///
/// Just the reasons. GitHub already shows that it was closed as not planned,
/// and which model held which opinion is a fact about the run rather than about
/// the issue. Duplicates are collapsed, since two reviewers reaching the same
/// conclusion often reach it in the same words.
pub fn skip_comment(item: &SkippedItem, style: &Style) -> String {
    let reasons = item
        .reasons
        .values()
        .map(|reason| style::sentence(reason, style));
    // Two reviewers declining one issue almost always decline it for the same
    // reason, worded differently. On the run that prompted this, both cited the
    // issue it duplicated and the reader saw the point twice.
    let lines = crate::textsim::dedupe_by(reasons, crate::textsim::same_reason);
    bullets(&lines)
}

/// Findings as a model should see them: full detail, since this one is not for
/// a human to read.
pub(crate) fn findings_for_prompt(findings: &[Finding]) -> String {
    if findings.is_empty() {
        return "(none)".to_string();
    }
    findings
        .iter()
        .map(|f| {
            let scope = if f.in_scope { "" } else { " [out of scope]" };
            format!(
                "- [{}]{scope} {} ({})\n  {}",
                f.severity,
                f.title,
                f.where_at(),
                f.detail
            )
        })
        .collect::<Vec<_>>()
        .join("\n")
}

#[cfg(test)]
mod tests {
    use super::*;
    use crate::model::Verdict;

    fn style() -> Style {
        Style::default()
    }

    fn finding(severity: &str, title: &str, detail: &str, file: &str, in_scope: bool) -> Finding {
        Finding {
            severity: Severity::parse_lenient(severity).unwrap(),
            title: title.into(),
            detail: detail.into(),
            file: file.into(),
            in_scope,
            ..Default::default()
        }
    }

    fn review(summary: &str, findings: Vec<Finding>) -> Review {
        Review {
            verdict: Verdict::Approve,
            next_action: NextAction::Merge,
            summary: summary.into(),
            findings,
        }
    }

    // -- worktree release ------------------------------------------------

    fn cfg_with(worktrees: bool, keep: bool) -> Config {
        let text = "[agents.a]\ncommand = [\"x\"]\n[agents.b]\ncommand = [\"y\"]\n";
        let mut cfg = crate::config::parse(text).unwrap();
        cfg.loop_cfg.worktrees = worktrees;
        cfg.loop_cfg.keep_worktrees = keep;
        cfg
    }

    #[test]
    fn a_worktree_is_released_on_every_finished_outcome() {
        let cfg = cfg_with(true, false);
        for status in [Status::Approved, Status::Merged, Status::Abandoned] {
            assert!(should_release(&cfg, status), "{status}");
        }
    }

    /// Releasing only on "merged" leaked one worktree per run, because
    /// auto_merge is off by default and runs end at "approved".
    #[test]
    fn a_worktree_is_kept_only_where_a_human_has_to_look() {
        let cfg = cfg_with(true, false);
        assert!(!should_release(&cfg, Status::Escalated));
        assert!(!should_release(&cfg, Status::Error));
    }

    #[test]
    fn the_keep_flag_overrides_everything() {
        assert!(!should_release(&cfg_with(true, true), Status::Approved));
    }

    #[test]
    fn nothing_is_released_when_worktrees_are_off() {
        assert!(!should_release(&cfg_with(false, false), Status::Approved));
    }

    // -- custody ---------------------------------------------------------

    /// The reviewer fixed the findings itself, so it wrote the head and the
    /// other agent takes round 2.
    #[test]
    fn fixing_your_own_findings_hands_the_pr_over() {
        let cfg = cfg_with(true, false);
        assert_eq!("a", next_reviewer(&cfg, "b", Some("b")));
        assert_eq!("b", next_reviewer(&cfg, "a", Some("a")));
    }

    /// The author wrote the head, so the reviewer keeps the PR. Flipping here
    /// gave the author its own fix to review in round 2, and an approval of it
    /// ended the loop.
    #[test]
    fn handing_back_keeps_the_reviewer_for_the_next_round() {
        let cfg = cfg_with(true, false);
        assert_eq!("b", next_reviewer(&cfg, "b", Some("a")));
        assert_eq!("a", next_reviewer(&cfg, "a", Some("b")));
    }

    /// Whoever holds round 2 did not write what it is reading, whoever wrote
    /// it. `a` implements, so `b` reviews round 1.
    #[test]
    fn nobody_reviews_their_own_edit() {
        let cfg = cfg_with(true, false);
        let round_1 = cfg.other(&cfg.first_implementor);
        assert_eq!("b", round_1);
        for editor in ["a", "b"] {
            assert_ne!(editor, next_reviewer(&cfg, &round_1, Some(editor)));
        }
    }

    /// The `fix_myself` half of the bug. The reviewer said it would fix its own
    /// findings and the call returned without committing, so the head is still
    /// the author's and handing over would put the author in front of its own
    /// work.
    #[test]
    fn a_fix_that_committed_nothing_leaves_the_pr_where_it_is() {
        let cfg = cfg_with(true, false);
        assert_eq!("b", next_reviewer(&cfg, "b", None));
        assert_eq!("a", next_reviewer(&cfg, "a", None));
    }

    /// The `hand_back` half. The reviewer committed while reviewing and the
    /// author answered without committing, so the head is the reviewer's and
    /// keeping it would have it read its own commit.
    #[test]
    fn a_reviewer_that_wrote_the_head_gives_the_pr_up() {
        let cfg = cfg_with(true, false);
        assert_eq!("a", next_reviewer(&cfg, "b", Some("b")));
    }

    /// A reviewer that fixes what it finds and then reports nothing blocking
    /// approved its own fix, and the rollback takes that fix out again. The
    /// head that would merge is not the head that passed.
    #[test]
    fn a_review_that_wrote_cannot_approve_what_is_left() {
        assert!(!approval_stands(&[], true));
    }

    #[test]
    fn a_clean_review_of_an_untouched_branch_approves() {
        assert!(approval_stands(&[], false));
    }

    #[test]
    fn a_blocking_finding_never_approves() {
        let blocking = vec![finding("blocking", "Broken", "detail", "src/x.rs", true)];
        assert!(!approval_stands(&blocking, false));
    }

    /// Custody is decided on what git says, not on the call returning.
    #[test]
    fn only_a_moved_head_counts_as_a_commit() {
        let before = Snapshot {
            head: "abc".into(),
            dirty: false,
        };
        assert!(!Snapshot {
            head: "abc".into(),
            dirty: true,
        }
        .landed_over(&before));
        assert!(Snapshot {
            head: "def".into(),
            dirty: false,
        }
        .landed_over(&before));
        // git could not be read, which is not evidence that anything landed.
        assert!(!Snapshot {
            head: String::new(),
            dirty: false,
        }
        .landed_over(&before));
    }

    // -- round budget ----------------------------------------------------

    /// A fresh PR gets rounds 1 through max_rounds.
    #[test]
    fn a_fresh_run_starts_at_one() {
        assert_eq!((1, 3), round_window(1, 3));
        assert_eq!((1, 5), round_window(1, 5));
    }

    /// The budget is per invocation, not a lifetime cap. Running spar again on
    /// a PR that already spent five rounds gives it five more, because a person
    /// looked at it and chose to.
    #[test]
    fn a_resumed_run_gets_a_full_fresh_budget() {
        assert_eq!((6, 10), round_window(6, 5));
        assert_eq!((11, 13), round_window(11, 3));
    }

    #[test]
    fn a_budget_of_one_is_a_single_round() {
        assert_eq!((6, 6), round_window(6, 1));
    }

    #[test]
    fn round_numbers_keep_counting_across_sessions() {
        // Three sessions of three rounds each land on 1..3, 4..6, 7..9.
        let mut start = 1;
        let mut seen = Vec::new();
        for _ in 0..3 {
            let (first, last) = round_window(start, 3);
            seen.push((first, last));
            start = last + 1;
        }
        assert_eq!(vec![(1, 3), (4, 6), (7, 9)], seen);
    }

    // -- the ledger ------------------------------------------------------

    fn ledger_with(title: &str, file: &str) -> Ledger {
        let mut ledger = Ledger::new();
        ledger.insert(
            finding_key(title, file),
            LedgerEntry {
                title: title.into(),
                file: file.into(),
                reasoning: "no".into(),
                round: 1,
                reraised: 0,
                outcome: Settled::Refuted,
            },
        );
        ledger
    }

    #[test]
    fn a_point_refuted_and_re_raised_twice_escalates() {
        let mut ledger = ledger_with("nit about naming", "a.rs");
        let mut state = IssueRun::new(1, "t");
        let blocking = vec![finding("blocking", "nit about naming", "d", "a.rs", true)];
        assert!(!check_relitigation(&mut ledger, &blocking, &mut state));
        assert!(check_relitigation(&mut ledger, &blocking, &mut state));
    }

    #[test]
    fn an_untracked_finding_does_not_escalate() {
        let mut state = IssueRun::new(1, "t");
        let blocking = vec![finding("blocking", "brand new", "d", "a.rs", true)];
        assert!(!check_relitigation(
            &mut Ledger::new(),
            &blocking,
            &mut state
        ));
    }

    /// The key a refutation records has to be the key the next round's finding
    /// hashes to. Recording it without the file made the guard dead code for
    /// every finding that named one, which is nearly all of them.
    #[test]
    fn a_refutation_lands_on_the_key_the_next_round_will_look_up() {
        let blocking = vec![finding("blocking", "Unbounded loop", "d", "src/x.rs", true)];
        let recorded = finding_key(&blocking[0].title, &blocking[0].file);

        let matched = matching_finding(&blocking, "unbounded loop!").expect("should match");
        assert_eq!(recorded, finding_key("unbounded loop!", &matched.file));
    }

    /// `matching_finding` ignores hyphens, dots, slashes, and underscores;
    /// `finding_key` keeps them. A disposition that differs only in those
    /// characters therefore matches its finding while hashing to a different
    /// key, so recording the author's wording made the guard track nothing.
    #[test]
    fn the_ledger_key_uses_the_reviewers_wording_not_the_authors() {
        let findings = vec![finding(
            "blocking",
            "Panic on multi-byte input",
            "d",
            "src/style.rs",
            true,
        )];
        let reworded = "Panic on multibyte input";

        let source = matching_finding(&findings, reworded).expect("still matches");
        assert_ne!(
            finding_key(reworded, &source.file),
            finding_key(&source.title, &source.file),
            "the two spellings must genuinely hash apart, or this test proves nothing"
        );

        // What apply_dispositions records, and what the next round looks up.
        let recorded = finding_key(&source.title, &source.file);
        let looked_up = finding_key(&findings[0].title, &findings[0].file);
        assert_eq!(recorded, looked_up);
    }

    #[test]
    fn a_disposition_matches_its_finding_despite_wording_noise() {
        let findings = vec![finding(
            "blocking",
            "Unbounded loop!",
            "d",
            "src/x.rs",
            true,
        )];
        assert!(matching_finding(&findings, "unbounded loop").is_some());
        assert!(matching_finding(&findings, "something else").is_none());
    }

    #[test]
    fn the_settled_block_is_empty_when_nothing_is_settled() {
        assert_eq!("", settled_block(&Ledger::new()));
    }

    #[test]
    fn the_settled_block_names_each_refutation() {
        let block = settled_block(&ledger_with("a point", "x.rs"));
        assert!(block.contains("a point"));
        assert!(block.contains("settled"));
    }

    /// A point the author moved to its own issue is done with on this branch.
    /// Leaving it out of the block let the reviewer that keeps the PR raise it
    /// again every round until the budget ran out.
    #[test]
    fn a_filed_point_is_settled_too() {
        let mut ledger = ledger_with("out of scope", "x.rs");
        for entry in ledger.values_mut() {
            entry.outcome = Settled::Filed;
            entry.reasoning = "Tracked in #9.".into();
        }
        let block = settled_block(&ledger);
        assert!(block.contains("out of scope"));
        assert!(block.contains("#9"));
    }

    /// The author answers the point again every round it is re-raised, so
    /// recording the answer must not wipe the count that ends the argument.
    #[test]
    fn answering_a_point_again_keeps_its_re_raise_count() {
        let mut ledger = ledger_with("a point", "x.rs");
        let entry = ledger.values().next().unwrap().clone();
        let key = finding_key("a point", "x.rs");
        let mut state = IssueRun::new(1, "t");
        let blocking = vec![finding("blocking", "a point", "d", "x.rs", true)];

        assert!(!check_relitigation(&mut ledger, &blocking, &mut state));
        settle(&mut ledger, key, entry);
        assert!(check_relitigation(&mut ledger, &blocking, &mut state));
    }

    // -- brevity ---------------------------------------------------------

    #[test]
    /// No agent name, no round number, and no count of things listed below.
    /// The reader wants the review, not an account of who produced it.
    fn a_clean_review_is_just_the_verdict() {
        let text = review_comment("codex", 1, &review("Looks correct.", vec![]), &style());
        assert_eq!("Looks correct.", text);
    }

    #[test]
    fn a_review_leads_with_the_counts() {
        let text = review_comment(
            "codex",
            2,
            &review(
                "One real problem.",
                vec![
                    finding(
                        "blocking",
                        "Loop never terminates",
                        "Confirmed by running it.",
                        "src/a.rs",
                        true,
                    ),
                    finding("non-blocking", "Name is vague", "d", "src/b.rs", true),
                    finding("nit", "Log wording", "d", "", true),
                ],
            ),
            &style(),
        );
        assert!(text.starts_with("One real problem."), "{text}");
        assert!(!text.contains("codex"), "no agent name: {text}");
        assert!(!text.contains("round 2"), "no round number: {text}");
    }

    /// Only blocking findings carry their detail into the thread. Everything
    /// else is filed, and the detail belongs on the issue.
    #[test]
    fn only_blocking_findings_carry_their_detail() {
        let text = review_comment(
            "codex",
            1,
            &review(
                "s",
                vec![
                    finding("blocking", "Loop", "BLOCKING DETAIL", "a.rs", true),
                    finding("non-blocking", "Name", "NONBLOCKING DETAIL", "b.rs", true),
                ],
            ),
            &style(),
        );
        assert!(text.contains("BLOCKING DETAIL"), "{text}");
        assert!(!text.contains("NONBLOCKING DETAIL"), "{text}");
    }

    #[test]
    /// A finding's explanation is what the author acts on. Cutting it to save
    /// characters leaves them nothing to act on and saves nothing worth having.
    fn a_thorough_explanation_reaches_the_author_intact() {
        let detail = "Reproduced by running the 429 test with max_attempts unset. ".repeat(8);
        let text = review_comment(
            "codex",
            1,
            &review(
                "One problem.",
                vec![finding("blocking", "T", &detail, "a.rs", true)],
            ),
            &style(),
        );
        assert!(
            text.contains(detail.trim()),
            "the explanation was cut:\n{text}"
        );
    }

    /// A runaway is still bounded, just nowhere near tightly.
    #[test]
    fn a_runaway_model_is_still_bounded() {
        let long = "filler words. ".repeat(20_000);
        let text = review_comment(
            "codex",
            1,
            &review(&long, vec![finding("blocking", "T", &long, "a.rs", true)]),
            &style(),
        );
        assert!(
            text.len() < 30_000,
            "review comment was {} chars",
            text.len()
        );
    }

    #[test]
    fn a_general_finding_has_no_empty_parenthesis() {
        let text = review_comment(
            "codex",
            1,
            &review("s", vec![finding("blocking", "Something", "d", "", true)]),
            &style(),
        );
        assert!(!text.contains("()"), "{text}");
        assert!(!text.contains("(general)"), "{text}");
    }

    #[test]
    fn out_of_scope_findings_are_counted_separately() {
        let text = review_comment(
            "codex",
            1,
            &review(
                "s",
                vec![finding("blocking", "Old bug", "d", "a.rs", false)],
            ),
            &style(),
        );
        assert!(text.contains("out of scope"), "{text}");
        assert!(text.contains("Old bug"), "{text}");
    }

    #[test]
    fn a_disposition_comment_leads_with_counts_and_keeps_refutations() {
        let response = ResponseDoc {
            summary: "Two of three were right.".into(),
            dispositions: vec![],
        };
        let text = disposition_comment(
            "claude",
            &response,
            &["Fixed thing".to_string()],
            &["Wrong thing. Because the caller already checks.".to_string()],
            &[],
            &style(),
        )
        .unwrap();
        assert!(text.starts_with("Two of three were right."), "{text}");
        assert!(!text.contains("claude"), "no agent name: {text}");
        assert!(
            text.contains("Because the caller already checks."),
            "{text}"
        );
    }

    #[test]
    fn an_empty_disposition_comment_is_not_posted() {
        let response = ResponseDoc {
            summary: "s".into(),
            dispositions: vec![],
        };
        assert!(disposition_comment("claude", &response, &[], &[], &[], &style()).is_none());
    }

    /// Both, not either. The link is how an agent that can reach the network
    /// reads the discussion spar does not fetch, and the body is what the one
    /// that cannot works from: codex runs with no network, so a link alone
    /// would leave it building from the title.
    #[test]
    fn the_implementor_is_given_the_link_and_the_body() {
        let prompt = implement_prompt(
            42,
            "Retry a 429",
            "https://github.com/o/r/issues/42",
            "A rate limited response was treated as fatal.",
        );
        assert!(
            prompt.contains("https://github.com/o/r/issues/42"),
            "{prompt}"
        );
        assert!(
            prompt.contains("A rate limited response was treated as fatal."),
            "{prompt}"
        );
        assert!(prompt.contains("#42"), "{prompt}");
        assert!(prompt.contains("Retry a 429"), "{prompt}");
        // Nothing left unsubstituted.
        assert!(!prompt.contains('{'), "{prompt}");
    }

    /// An agent that cannot reach the link is told what it is missing, so it
    /// works from the body rather than assuming the body is everything.
    #[test]
    fn the_prompt_says_the_discussion_is_not_included() {
        let prompt = implement_prompt(1, "t", "u", "b");
        // Flattened, so the assertion does not turn on where the prompt wraps.
        let lower = prompt
            .split_whitespace()
            .collect::<Vec<_>>()
            .join(" ")
            .to_lowercase();
        assert!(
            lower.contains("discussion since is not included"),
            "{prompt}"
        );
        assert!(lower.contains("cannot reach the network"), "{prompt}");
    }

    /// A fully reported implementation, for the body tests.
    fn worked() -> Implementation {
        Implementation {
            summary: "Retry a 429 instead of failing the run.".into(),
            problem: "A rate limited response was treated as fatal, so one throttled call ended \
                      a run that had hours of work left in it."
                .into(),
            changes: vec![
                "`send` retries a 429 with the delay the header asks for".into(),
                "the retry budget is bounded, so a permanent 429 still ends".into(),
            ],
            testing: vec![
                "`cargo test retries_a_429`".into(),
                "point it at a throttled endpoint and watch it finish".into(),
            ],
            ..Implementation::default()
        }
    }

    #[test]
    /// GitHub renders the file count and the plus and minus figures in the
    /// header, immediately above whatever spar writes, so neither is here.
    fn a_pr_body_is_what_it_closes_and_what_changed() {
        let body = pr_body(42, &worked(), &style());
        assert_eq!(
            "Closes #42\n\n\
             Retry a 429 instead of failing the run.\n\n\
             A rate limited response was treated as fatal, so one throttled call \
             ended a run that had hours of work left in it.\n\n\
             ## What changed\n\n\
             - `send` retries a 429 with the delay the header asks for\n\
             - the retry budget is bounded, so a permanent 429 still ends\n\n\
             ## How to test\n\n\
             - `cargo test retries_a_429`\n\
             - point it at a throttled endpoint and watch it finish",
            body
        );
    }

    /// The sections are optional and the lead is not. A one line fix should
    /// read as one, not as a form with most of it left blank.
    #[test]
    fn a_body_with_nothing_to_list_carries_no_empty_headings() {
        let work = Implementation {
            summary: "Retry a 429 instead of failing the run.".into(),
            ..Implementation::default()
        };
        assert_eq!(
            "Closes #42\n\nRetry a 429 instead of failing the run.",
            pr_body(42, &work, &style())
        );
    }

    #[test]
    fn a_pr_body_survives_an_implementor_that_said_nothing() {
        assert_eq!(
            "Closes #7",
            pr_body(7, &Implementation::default(), &style())
        );
    }

    /// Blank entries are the model's, not the reader's problem. A heading whose
    /// only bullet was an empty string used to be possible.
    #[test]
    fn blank_list_entries_do_not_earn_a_heading() {
        let work = Implementation {
            summary: "Did a thing.".into(),
            changes: vec![String::new(), "   ".into()],
            ..Implementation::default()
        };
        let body = pr_body(42, &work, &style());
        assert!(!body.contains("What changed"), "{body}");
    }

    #[test]
    fn notes_appear_only_when_there_is_something_to_note() {
        let mut work = worked();
        assert!(!pr_body(42, &work, &style()).contains("## Notes"));
        work.notes = Some("The retry is not applied to streaming calls.".into());
        let body = pr_body(42, &work, &style());
        assert!(body.contains("## Notes"), "{body}");
        assert!(body.contains("streaming calls"), "{body}");
    }

    /// An issue that produced no commits is told so. Never the summary, which
    /// describes a change that is not in the branch.
    #[test]
    fn declining_posts_the_reason_and_not_the_summary() {
        let work = Implementation {
            not_worth_doing: true,
            reason: "Already fixed in 1.2, and the report predates it.".into(),
            summary: "Nothing to do.".into(),
            ..Implementation::default()
        };
        assert_eq!(
            "Already fixed in 1.2, and the report predates it.",
            no_pr_note(&work, &style())
        );
    }

    #[test]
    fn reporting_work_and_committing_none_says_that_rather_than_the_summary() {
        let work = Implementation {
            summary: "Retry a 429 instead of failing the run.".into(),
            ..Implementation::default()
        };
        let note = no_pr_note(&work, &style());
        assert_eq!(
            "Nothing was committed, so there is nothing to review.",
            note
        );
    }

    #[test]
    fn declining_without_a_reason_still_says_something() {
        let work = Implementation {
            not_worth_doing: true,
            ..Implementation::default()
        };
        assert!(no_pr_note(&work, &style()).contains("no reason given"));
    }

    #[test]
    fn a_skip_comment_is_only_the_reasoning() {
        let item = SkippedItem {
            issue: 3,
            title: "t".into(),
            tracker: false,
            reasons: [
                ("claude".to_string(), "Already fixed in 1.2.".to_string()),
                ("codex".to_string(), "Duplicate of #2.".to_string()),
            ]
            .into_iter()
            .collect(),
        };
        let text = skip_comment(&item, &style());
        assert!(text.contains("Already fixed in 1.2."), "{text}");
        assert!(text.contains("Duplicate of #2."), "{text}");
        assert!(
            !text.contains("claude") && !text.contains("codex"),
            "{text}"
        );
        assert!(!text.to_lowercase().contains("not scheduled"), "{text}");
        assert!(text.lines().count() <= 3, "{text}");
    }

    #[test]
    fn findings_for_a_model_keep_full_detail() {
        let long = "x".repeat(2000);
        let text = findings_for_prompt(&[finding("blocking", "T", &long, "a.rs", true)]);
        assert!(
            text.contains(&long),
            "a model needs the whole finding, only humans need brevity"
        );
    }

    #[test]
    fn findings_for_a_model_are_never_empty() {
        assert_eq!("(none)", findings_for_prompt(&[]));
    }
}

#[cfg(test)]
mod outcome_tests {
    use super::*;
    use crate::model::{Dispute, Severity};

    fn style() -> Style {
        Style::default()
    }

    fn state_with(disputes: Vec<(&str, &str)>, filed: Vec<&str>) -> IssueRun {
        let mut s = IssueRun::new(482, "t");
        s.disputes = disputes
            .into_iter()
            .map(|(title, reasoning)| Dispute {
                title: title.into(),
                reasoning: reasoning.into(),
            })
            .collect();
        s.filed = filed.into_iter().map(String::from).collect();
        s
    }

    fn finding(title: &str, file: &str) -> Finding {
        Finding {
            severity: Severity::Blocking,
            title: title.into(),
            detail: "d".into(),
            file: file.into(),
            in_scope: true,
            ..Default::default()
        }
    }

    /// The absence of objections is the message. A PR that reviewed cleanly and
    /// filed nothing should leave no trace in the thread at all.
    #[test]
    fn a_clean_approval_says_nothing() {
        let state = state_with(vec![], vec![]);
        assert!(outcome_comment(&state, &Ledger::new(), &Ending::Approved, &style()).is_none());
    }

    #[test]
    fn an_approval_that_filed_follow_ups_links_them() {
        let state = state_with(
            vec![],
            vec![
                "https://github.com/you/thing/issues/485",
                "https://github.com/you/thing/issues/486",
            ],
        );
        let text = outcome_comment(&state, &Ledger::new(), &Ending::Approved, &style()).unwrap();
        assert!(text.contains("Filed separately: #485, #486"), "{text}");
    }

    /// The real PR ended with "5 fixed" followed by "no convergence", which
    /// reads as a contradiction. What a maintainer needs is that the fixes went
    /// in and nobody checked them.
    #[test]
    fn running_out_of_rounds_says_what_that_means_for_the_reader() {
        let state = state_with(vec![], vec![]);
        let text = outcome_comment(&state, &Ledger::new(), &Ending::OutOfRounds, &style()).unwrap();
        assert!(text.contains("has not been reviewed"), "{text}");
        assert!(
            !text.to_lowercase().contains("round 3"),
            "no round numbers: {text}"
        );
        assert!(!text.to_lowercase().contains("convergence"), "{text}");
    }

    #[test]
    fn a_deadlock_names_the_point_they_could_not_settle() {
        let state = state_with(vec![], vec![]);
        let points = [finding("Retry loop never terminates", "src/net.rs:88")];
        let text = outcome_comment(
            &state,
            &Ledger::new(),
            &Ending::Deadlocked(&points),
            &style(),
        )
        .unwrap();
        assert!(
            text.contains("Retry loop never terminates (src/net.rs:88)"),
            "{text}"
        );
        assert!(text.contains("could not settle"), "{text}");
    }

    /// The diff records what was fixed. Nothing records what was argued down.
    #[test]
    fn refutations_survive_because_nothing_else_carries_them() {
        let state = state_with(
            vec![(
                "Error is swallowed",
                "the caller already validates the file",
            )],
            vec![],
        );
        let text = outcome_comment(&state, &Ledger::new(), &Ending::Approved, &style()).unwrap();
        assert!(text.contains("Raised and refuted:"), "{text}");
        assert!(
            text.contains("The caller already validates the file"),
            "{text}"
        );
    }

    #[test]
    fn no_agent_names_counts_or_round_numbers_reach_the_thread() {
        let state = state_with(
            vec![("A point", "a reason")],
            vec!["https://github.com/you/thing/issues/485"],
        );
        for ending in [Ending::Approved, Ending::OutOfRounds] {
            let text = outcome_comment(&state, &Ledger::new(), &ending, &style()).unwrap();
            let lower = text.to_lowercase();
            for banned in ["claude", "codex", "blocking,", "nit,", " fixed."] {
                assert!(
                    !lower.contains(banned),
                    "{banned:?} leaked into the thread:\n{text}"
                );
            }
            // "the last round of fixes" is prose. "round 3" is narration.
            for n in 1..9 {
                assert!(
                    !lower.contains(&format!("round {n}")),
                    "a round number leaked into the thread:\n{text}"
                );
            }
        }
    }

    #[test]
    /// A refutation is an argument, and an argument that stops mid clause is
    /// not one. Bounded, but with room to make the case.
    fn a_refutation_is_allowed_to_make_its_case() {
        let reasoning = "The caller validates against the schema first. \
                         The discarded error is therefore unreachable in practice. ";
        let state = state_with(
            vec![("A point", &reasoning.repeat(6))],
            vec!["https://github.com/you/thing/issues/485"],
        );
        let text = outcome_comment(&state, &Ledger::new(), &Ending::OutOfRounds, &style()).unwrap();
        assert!(
            !text.contains("..."),
            "nothing was cut mid thought:\n{text}"
        );
        assert!(text.len() < 4000, "{} chars", text.len());
    }

    #[test]
    fn a_url_that_is_not_an_issue_link_is_left_alone() {
        assert_eq!(
            "#485",
            as_reference("https://github.com/you/thing/issues/485")
        );
        assert_eq!("note: something", as_reference("note: something"));
    }
}

#[cfg(test)]
mod filed_reference_tests {
    use super::*;

    #[test]
    fn an_issue_url_yields_its_number() {
        assert_eq!(
            Some(485),
            filed_issue_number("https://github.com/you/thing/issues/485")
        );
    }

    /// Local mode records a note rather than a URL, and a run with
    /// followups = "local" must not try to absorb it as an issue.
    #[test]
    fn a_local_note_yields_nothing() {
        assert_eq!(None, filed_issue_number("note: Retry is unbounded"));
        assert_eq!(None, filed_issue_number(""));
        assert_eq!(
            None,
            filed_issue_number("https://github.com/you/thing/issues/")
        );
    }
}

#[cfg(test)]
mod followup_restraint_tests {
    use super::*;
    use crate::model::Severity;

    fn cfg_with(followups: Followups, non_blocking: bool, nits: bool, cap: usize) -> Config {
        let mut cfg =
            crate::config::parse("[agents.a]\ncommand = [\"x\"]\n[agents.b]\ncommand = [\"y\"]\n")
                .unwrap();
        cfg.loop_cfg.followups = followups;
        cfg.loop_cfg.file_non_blocking = non_blocking;
        cfg.loop_cfg.file_nits = nits;
        cfg.loop_cfg.max_followups = cap;
        cfg
    }

    fn finding(severity: Severity, title: &str, in_scope: bool) -> Finding {
        Finding {
            severity,
            title: title.into(),
            detail: "d".into(),
            file: "a.rs".into(),
            in_scope,
            ..Default::default()
        }
    }

    /// The defaults are what let one issue spawn ten, which spawned more. A
    /// thorough reviewer always finds improvements; not gating a merge is not
    /// the same as deserving somebody's triage queue.
    #[test]
    fn a_non_blocking_finding_is_not_a_tracker_item_by_default() {
        let cfg = cfg_with(Followups::Issues, false, false, 5);
        assert!(!cfg.loop_cfg.file_non_blocking);
        assert!(!cfg.loop_cfg.file_nits);
    }

    #[test]
    fn follow_ups_stay_off_the_tracker_by_default() {
        let cfg =
            crate::config::parse("[agents.a]\ncommand = [\"x\"]\n[agents.b]\ncommand = [\"y\"]\n")
                .unwrap();
        assert_eq!(
            Followups::Local,
            cfg.loop_cfg.followups,
            "the tracker is somebody's queue; the default must not write to it"
        );
        assert_eq!(5, cfg.loop_cfg.max_followups);
    }

    /// Which severities survive the filter, at the defaults and when opened up.
    #[test]
    fn only_out_of_scope_defects_qualify_at_the_defaults() {
        let cfg = cfg_with(Followups::Issues, false, false, 5);
        let qualifies = |f: &Finding| match f.severity {
            Severity::NonBlocking => cfg.loop_cfg.file_non_blocking && f.in_scope,
            Severity::Nit => cfg.loop_cfg.file_nits && f.in_scope,
            Severity::Blocking => false,
        } || !f.in_scope;

        assert!(qualifies(&finding(
            Severity::Blocking,
            "pre-existing",
            false
        )));
        assert!(!qualifies(&finding(
            Severity::NonBlocking,
            "improvement",
            true
        )));
        assert!(!qualifies(&finding(Severity::Nit, "taste", true)));
        assert!(!qualifies(&finding(
            Severity::Blocking,
            "fix it here",
            true
        )));
    }

    #[test]
    fn opening_it_up_lets_non_blocking_findings_through_again() {
        let cfg = cfg_with(Followups::Issues, true, false, 5);
        assert!(cfg.loop_cfg.file_non_blocking);
    }

    /// A run that will not stop finding things is stopped, and says so.
    #[test]
    fn the_cap_is_a_real_backstop() {
        let cfg = cfg_with(Followups::Issues, false, false, 3);
        let mut state = IssueRun::new(1, "t");
        state.filed = (0..3).map(|n| format!("url{n}")).collect();
        assert!(state.filed.len() >= cfg.loop_cfg.max_followups);
    }

    /// The number that matters. Reviewing one issue produced ten follow-ups on
    /// a real repository, each of which could be run in turn: mean offspring
    /// above one never terminates.
    #[test]
    fn the_cap_bounds_what_one_run_can_spawn() {
        let cfg = cfg_with(Followups::Issues, false, false, 5);
        assert!(
            cfg.loop_cfg.max_followups <= 5,
            "a run that can file ten follow-ups is a branching process"
        );
    }
}

/// What the ledger is told about a point the author moved out of the pull
/// request. Every case here used to record "filed", including the ones where
/// nothing was written anywhere.
#[cfg(test)]
mod followup_outcome_tests {
    use super::*;

    const URL: &str = "https://github.com/you/thing/issues/485";

    fn entry(recorded: Followup) -> Option<(Settled, String)> {
        filed_entry(&recorded, "It predates this branch.")
    }

    /// The bug. A tracker request or a local write that failed left no
    /// follow-up, and the ledger said it had been filed, which is a claim that
    /// survives every later round and every resume.
    #[test]
    fn a_failed_followup_settles_nothing() {
        assert_eq!(None, entry(Followup::Failed));
    }

    #[test]
    fn a_recorded_followup_is_filed_and_says_where() {
        let (outcome, reasoning) = entry(Followup::Recorded(URL.into())).unwrap();
        assert_eq!(Settled::Filed, outcome);
        assert!(
            reasoning.contains("It predates this branch."),
            "{reasoning}"
        );
        assert!(reasoning.contains("#485"), "{reasoning}");
    }

    /// A closed issue already carries the point, so raising it again is waste.
    /// It is still not something to hand anybody as work.
    #[test]
    fn a_closed_issue_covering_the_point_settles_it_without_offering_work() {
        let recorded = Followup::from(Filed::AlreadyClosed(9, URL.into()));
        assert_eq!(Followup::Covered(URL.into()), recorded);
        assert_eq!(
            None,
            recorded.url(),
            "a closed issue is not work to pick up"
        );

        let (outcome, reasoning) = entry(recorded).unwrap();
        assert_eq!(Settled::Filed, outcome);
        assert!(reasoning.contains("#485"), "{reasoning}");
    }

    /// An open issue that already covers the point is worth linking from the
    /// pull request, and worth counting against the cap.
    #[test]
    fn an_open_issue_that_already_covers_the_point_is_still_a_reference() {
        for filed in [
            Filed::Opened(9, URL.into()),
            Filed::AddedTo(9, URL.into()),
            Filed::Covered(9, URL.into()),
        ] {
            assert_eq!(Some(URL), Followup::from(filed).url());
        }
    }

    /// Configuration, not failure: retrying it every round would spend the
    /// budget on a write that is never going to happen. The entry has to be
    /// honest about it, because nothing else holds the point.
    #[test]
    fn a_dropped_followup_is_settled_but_never_reported_as_filed() {
        let (outcome, reasoning) = entry(Followup::Dropped("follow-ups are off")).unwrap();
        assert_eq!(Settled::Dropped, outcome);
        assert!(reasoning.contains("follow-ups are off"), "{reasoning}");
        assert!(reasoning.contains("Not filed"), "{reasoning}");
    }

    fn ledger_of(outcome: Settled, reasoning: &str) -> Ledger {
        let mut ledger = Ledger::new();
        ledger.insert(
            finding_key("A pre-existing leak", "src/x.rs"),
            LedgerEntry {
                title: "A pre-existing leak".into(),
                file: "src/x.rs".into(),
                reasoning: reasoning.into(),
                round: 1,
                reraised: 0,
                outcome,
            },
        );
        ledger
    }

    /// The next reviewer is told to leave settled points alone either way, so
    /// the wording is all that separates them. Saying "filed" of a point
    /// nothing holds is the lie that loses it.
    #[test]
    fn the_settled_block_tells_a_filed_point_from_a_dropped_one() {
        let filed = settled_block(&ledger_of(Settled::Filed, "Tracked in #9."));
        assert!(filed.contains("out of scope here, and filed"), "{filed}");

        let dropped = settled_block(&ledger_of(Settled::Dropped, "Not filed anywhere: off."));
        assert!(
            dropped.contains("out of scope here, and not filed"),
            "{dropped}"
        );
        assert!(dropped.contains("A pre-existing leak"), "{dropped}");
    }

    /// A deadlock goes to a person, and the first thing they do is look for the
    /// issue the comment says exists.
    #[test]
    fn a_deadlocked_point_that_was_never_filed_does_not_claim_to_be() {
        let points = [Finding {
            severity: Severity::Blocking,
            title: "A pre-existing leak".into(),
            detail: "d".into(),
            file: "src/x.rs".into(),
            in_scope: false,
            ..Default::default()
        }];
        let text = outcome_comment(
            &IssueRun::new(1, "t"),
            &ledger_of(Settled::Dropped, "Not filed anywhere: follow-ups are off."),
            &Ending::Deadlocked(&points),
            &Style::default(),
        )
        .unwrap();
        assert!(text.contains("not filed"), "{text}");
        assert!(!text.contains("Filed as out of scope"), "{text}");
    }
}

#[cfg(test)]
mod issue_report_tests {
    use super::*;
    use crate::model::Severity;

    /// Shaped after a bug report written by hand that reads the way one should:
    /// what is wrong, how to see it, what it costs, what it should do instead.
    fn reported() -> Finding {
        Finding {
            severity: Severity::Blocking,
            title: "sendPaymentAsync bypasses drain mode and spending limits".into(),
            detail: "The async path skips every admission check payInvoice applies.".into(),
            file: "src/node.ts:412".into(),
            in_scope: false,
            problem: Some(
                "`BeignetNode.sendPaymentAsync()` submits a payment directly to the Lightning \
                 engine without applying the safeguards used by `payInvoice()`.\n\nThe async path \
                 does not:\n\n- call `_checkDraining()`\n- call `_checkSpendLimit()`"
                    .into(),
            ),
            reproduction: Some(
                "1. Create a `BeignetNode` with `dailySpendLimitSats: 1`.\n2. Enable drain mode.\n\
                 3. Submit a 1,000 sat invoice.\n\nActual result:\n\n- The engine is called.\n\
                 - `spentSats` remains 0."
                    .into(),
            ),
            impact: Some(
                "An authorized client can submit async payments up to the available outbound \
                 liquidity despite the configured limits."
                    .into(),
            ),
            expected: Some(
                "- Reject new payments while draining.\n- Enforce the per-payment limit before \
                 submission.\n- Cover both paths with regression tests.\n\nThis predates the \
                 current branch."
                    .into(),
            ),
        }
    }

    #[test]
    fn a_reported_finding_becomes_a_bug_report() {
        let body = issue_report(&reported());
        for heading in [
            "## Problem",
            "## Reproduction",
            "## Impact",
            "## Expected behavior",
        ] {
            assert!(body.contains(heading), "missing {heading}:\n{body}");
        }
        // In the order somebody reads a bug report.
        let at = |h: &str| body.find(h).unwrap();
        assert!(at("## Problem") < at("## Reproduction"));
        assert!(at("## Reproduction") < at("## Impact"));
        assert!(at("## Impact") < at("## Expected behavior"));
    }

    #[test]
    fn the_substance_survives_the_outbound_gates() {
        let repo_style = Style::default();
        let body = crate::style::issue_body(&issue_report(&reported()), &repo_style);
        for kept in [
            "_checkDraining()",
            "Actual result:",
            "outbound liquidity",
            "regression tests",
            "predates the current branch",
        ] {
            assert!(body.contains(kept), "the gate ate {kept:?}:\n{body}");
        }
        assert!(!body.contains("..."), "something was cut:\n{body}");
    }

    /// A finding that was never going to be filed carries none of this, and
    /// must not gain empty headings for the sake of a format.
    #[test]
    fn an_ordinary_finding_is_still_just_its_detail() {
        let plain = Finding {
            severity: Severity::NonBlocking,
            title: "Name is vague".into(),
            detail: "The variable could say what it holds.".into(),
            file: "a.rs".into(),
            in_scope: true,
            ..Default::default()
        };
        assert_eq!(
            "The variable could say what it holds.",
            issue_report(&plain)
        );
    }

    /// Partial reports are normal: a defect with no useful reproduction should
    /// not sprout an empty Reproduction heading.
    #[test]
    fn only_the_sections_that_were_written_appear() {
        let partial = Finding {
            problem: Some("The guard is inverted.".into()),
            expected: Some("It should reject rather than accept.".into()),
            ..reported()
        };
        let partial = Finding {
            reproduction: None,
            impact: None,
            ..partial
        };
        let body = issue_report(&partial);
        assert!(body.contains("## Problem") && body.contains("## Expected behavior"));
        assert!(!body.contains("## Reproduction"), "{body}");
        assert!(!body.contains("## Impact"), "{body}");
    }

    /// The one line the thread shows is not repeated when a section already
    /// says it.
    #[test]
    fn the_summary_line_is_not_printed_twice() {
        let echoed = Finding {
            detail: "The guard is inverted so it rejects valid input.".into(),
            problem: Some("The guard is inverted so it rejects valid input.".into()),
            reproduction: None,
            impact: None,
            expected: None,
            ..reported()
        };
        let body = issue_report(&echoed);
        assert_eq!(1, body.matches("The guard is inverted").count(), "{body}");
    }
}