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.
//!
//! 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, Followups, PrComments};
use crate::error::{Result, SparError};
use crate::jsonx::finding_key;
use crate::model::{
    Action, Dispute, Finding, Implementation, Issue, IssueRun, Ledger, LedgerEntry, NextAction,
    PersistedState, PlanItem, PrView, ResponseDoc, Review, 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.";

/// 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 mut work: Implementation = implementor.ask_json(
        &prompt,
        &schema::implementation(),
        work_dir,
        cfg.effort_for_round(&implementor.spec, 1).as_deref(),
    )?;

    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();
    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 review: Review = reviewer.review(
            &base,
            &prompt,
            &schema::review(),
            &ctx.work_dir,
            effort.as_deref(),
        )?;

        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,
                &cfg.other(&holder),
            );
            return Ok(());
        }

        if blocking.is_empty() {
            state.status = Status::Approved;
            post_outcome(repo, ctx.pr_number, state, ledger, Ending::Approved);
            persist(
                repo,
                ctx.pr_number,
                state,
                ledger,
                round,
                &cfg.other(&holder),
            );
            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 review.next_action == NextAction::FixMyself {
            log!("{}: {holder} fixing its own findings", ctx.label);
            let prompt = FIX_PROMPT.replace("{findings}", &findings_for_prompt(&blocking));
            reviewer.ask(&prompt, &ctx.work_dir, effort.as_deref())?;
        } 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 response: ResponseDoc = author.ask_json(
                &prompt,
                &schema::response(),
                &ctx.work_dir,
                cfg.effort_for_round(&author.spec, round).as_deref(),
            )?;
            apply_dispositions(
                repo,
                cfg,
                &response,
                &blocking,
                ledger,
                state,
                round,
                ctx.subject,
                ctx.pr_number,
                &author_name,
            );
        }

        repo.rewrite_commits_if_needed(&ctx.work_dir, &base)?;
        repo.push(&ctx.work_dir, &ctx.branch)?;
        holder = cfg.other(&holder);
        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(())
}

/// 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| format!("- {}: refuted because {}", e.title, e.reasoning))
        .collect();
    format!(
        "\nThe following points were already raised and refuted. Treat them as settled. Do not \
         raise them again unless you have new evidence:\n{}",
        lines.join("\n")
    )
}

/// A point refuted and then 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 refuted 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);
                ledger.insert(
                    finding_key(canonical, &file),
                    LedgerEntry {
                        title: title.clone(),
                        file: file.clone(),
                        reasoning: reasoning.clone(),
                        round,
                        reraised: 0,
                    },
                );
                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 {
                    state.filed.push(url.clone());
                    filed.push(url);
                }
            }
            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}");
            }
        }
    }
}

// ---------------------------------------------------------------------------
// 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.
fn file_followup(
    repo: &Repo,
    title: &str,
    body: &str,
    source: i64,
    cfg: &Config,
    state: &IssueRun,
) -> Option<String> {
    if repo.followups == Followups::None {
        return None;
    }
    // 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 None;
    }
    // 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.
    let title = match repo.clean_title(title) {
        Ok(title) => title,
        Err(e) => {
            logdim!("could not clean a follow-up title: {e}");
            return None;
        }
    };
    if title.trim().is_empty() {
        return None;
    }
    // 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);
    }

    // An issue that already covers this, however it was worded. Filing a second
    // one is the complaint; silently dropping the new wording is not much
    // better, because a later run often carries evidence the first did not.
    if let Some(existing) = repo.find_similar_issue(&title, &body) {
        let known = format!("{} {}", existing.title, existing.body);
        if !existing.open {
            logdim!(
                "#{} already covers '{title}' and is closed, leaving it alone",
                existing.number
            );
            return None;
        }
        if crate::textsim::adds_information(&body, &known) {
            match repo.comment_issue(existing.number, &body) {
                Ok(()) => log!("added to #{}: {title}", existing.number),
                Err(e) => logdim!("could not add to #{}: {e}", existing.number),
            }
        } else {
            logdim!("#{} already says this, nothing added", existing.number);
        }
        return Some(existing.url);
    }

    match repo.create_issue(&title, &body) {
        Ok(url) => Some(url),
        Err(e) => {
            logdim!("could not file a follow-up for '{title}': {e}");
            None
        }
    }
}

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);
        if let Some(url) = file_followup(repo, &finding.title, &body, subject, cfg, state) {
            state.filed.push(url);
        }
    }
}

/// 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;
        }
        if let Some(url) = file_followup(repo, &finding.title, &finding.detail, subject, cfg, state)
        {
            state.filed.push(url);
        }
    }
}

// ---------------------------------------------------------------------------
// 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}");
    }
}

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

/// `#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 refuted 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 refutation_of(f, state, ledger) {
                        Some(reason) => format!(
                            "{title}{where_at}. Refuted as: {}",
                            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)))
}

/// 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));
    }

    // -- 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,
            },
        );
        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"));
    }

    // -- 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"
        );
    }
}

#[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}");
    }
}