Skip to main content

magi/
land.rs

1//! Landing the winner: watch the pull request, fix what it complains about,
2//! and merge it.
3//!
4//! Opening the pull request used to be where magi stopped and the operator
5//! started: watch the checks, read what the review bots found, push a fix,
6//! wait again, merge. That loop is mechanical, it takes an hour of wall-clock
7//! time per pull request, and doing it by hand six times in one session is how
8//! a queue that drains unattended stops being unattended. So it lives here.
9//!
10//! # Shape
11//!
12//! [`PrState`] is one observation of a pull request and [`decide`] is the whole
13//! policy as a *pure* function of it. Nothing in [`decide`] talks to `gh`,
14//! which is what makes "green with an unresolved comment is a fix, not a merge"
15//! an assertion in a test rather than a claim in a comment. [`land`] is the
16//! only part that performs I/O: observe, decide, act, repeat.
17//!
18//! # What it refuses to do
19//!
20//! Merging is the one irreversible thing magi can do to a repository, so the
21//! loop is built to stop rather than to guess:
22//!
23//! * A red pull request is never merged. When the budget runs out the pull
24//!   request is left open with a comment naming what is still failing, because
25//!   a magi that force-merges a red pull request is worse than one that stops.
26//! * A pull request whose checks cannot be read at all (`gh` reported no
27//!   rollup) is not merged either. Landing is for repositories with CI; with no
28//!   signal there is nothing to be green.
29//! * A pull request a human merged or closed underneath us is
30//!   [`Step::Done`] - the person won, and their decision is not an error.
31//!
32//! # Why `--subject` is not optional
33//!
34//! A candidate branch holds one commit whose subject is
35//! `magi: candidate A (uncommitted work)`, and `gh pr merge --squash` prefers a
36//! single commit's message over the pull request title. Merging without
37//! [`merge_argv`]'s explicit `--subject` therefore writes a `main` history that
38//! says nothing about what landed. `AGENTS.md` records the trap; this module is
39//! where it is prevented.
40
41use std::collections::{BTreeMap, BTreeSet};
42use std::fmt::Write as _;
43use std::path::{Path, PathBuf};
44use std::sync::Arc;
45use std::time::Duration;
46
47use anyhow::{Context as _, Result, bail};
48use serde::Deserialize;
49
50use crate::agent::{self, Invocation, SeatState};
51use crate::ask;
52use crate::config::{AgentSpec, MergeMode};
53use crate::git;
54use crate::proc::Quiet as _;
55use crate::prompt;
56use crate::run::{MergeOutcome, RunState, RunStatus, tail};
57
58/// How often the pull request is re-read while its checks are still running.
59///
60/// Thirty seconds: a CI matrix takes minutes, so anything shorter is spent
61/// entirely on `gh` invocations, and anything much longer adds latency to every
62/// single round of a loop that already waits for agents.
63pub const POLL: Duration = Duration::from_secs(30);
64
65/// How long one wait may last before landing gives up on the checks finishing.
66///
67/// A workflow that has not settled in forty-five minutes is stuck on a runner
68/// queue, a missing approval, or a hung job - none of which more polling fixes,
69/// and all of which a person needs to see.
70pub const WAIT_CEILING: Duration = Duration::from_secs(45 * 60);
71
72/// How long the checks may stay unreadable before landing gives up on them.
73///
74/// GitHub registers a workflow run some seconds after the branch is pushed, so
75/// immediately after a pull request is opened "no checks" and "no CI in this
76/// repository" look identical. Measured on run 01c2: magi opened pull request
77/// 22, read `unknown` four seconds later, refused to merge on a guess and
78/// marked the run blocked - and every check on that pull request was green
79/// minutes afterwards, with the whole competition then re-run from scratch for
80/// a task that was already finished. Three minutes is well past the observed
81/// registration delay and still bounded, so a repository that genuinely has no
82/// checks costs one three-minute wait and then says so.
83pub const CHECKS_GRACE: Duration = Duration::from_secs(3 * 60);
84
85/// Bytes of failing log kept per check. The fixer needs the assertion and the
86/// frame around it, not the forty thousand lines of `cargo` output above it.
87const LOG_TAIL: usize = 4_000;
88
89/// Failing checks whose logs are fetched. Beyond a handful the failures share a
90/// cause, and fetching each one costs a `gh` round trip.
91const MAX_LOGS: usize = 3;
92
93/// Marker carried by every comment magi posts on a pull request.
94///
95/// Without it magi's own "still failing" comment is indistinguishable from a
96/// reviewer's, and the next observation would hand magi's own prose to the
97/// fixer as a finding.
98pub const MARKER: &str = "<!-- magi:land -->";
99
100/// Markers a bot puts in a comment to say that the comment is not a review.
101///
102/// CodeRabbit labels its own machinery in HTML comments - the trigger notice,
103/// the walkthrough summary, the "thanks for using" footer - and its actual
104/// findings arrive as *inline* review comments with a path and a line. Taking
105/// the bot at its word is more honest than guessing from prose, and it is the
106/// difference between a fix round that has something to fix and one that asks
107/// an agent to act on a quota notice.
108const NOT_A_REVIEW: [&str; 3] = [
109    "skip review by coderabbit.ai",
110    "summarize by coderabbit.ai",
111    "<!-- tips_start -->",
112];
113
114/// Where a pull request is in its life.
115#[derive(Debug, Clone, Copy, PartialEq, Eq)]
116pub enum PrLifecycle {
117    /// Still ours to land.
118    Open,
119    /// Already merged, by us or by a person.
120    Merged,
121    /// Closed without merging.
122    Closed,
123}
124
125/// The aggregate verdict of a pull request's checks.
126#[derive(Debug, Clone, Copy, PartialEq, Eq)]
127pub enum Checks {
128    /// At least one check has not finished.
129    Pending,
130    /// Every check passed (a skipped check counts as passed: the review
131    /// workflow skips release and bot pull requests by design).
132    Green,
133    /// At least one check finished without passing.
134    Red,
135    /// Nothing readable - no rollup at all, or a status magi does not know.
136    Unknown,
137}
138
139impl PrLifecycle {
140    /// Stable lower-case name, as the API and the reports spell it.
141    pub fn as_str(self) -> &'static str {
142        match self {
143            Self::Open => "open",
144            Self::Merged => "merged",
145            Self::Closed => "closed",
146        }
147    }
148}
149
150impl Checks {
151    /// Stable lower-case name, as the API and the reports spell it.
152    pub fn as_str(self) -> &'static str {
153        match self {
154            Self::Pending => "pending",
155            Self::Green => "green",
156            Self::Red => "red",
157            Self::Unknown => "unknown",
158        }
159    }
160}
161
162/// One outstanding review comment, human or bot.
163#[derive(Debug, Clone, PartialEq, Eq)]
164pub struct ReviewComment {
165    /// Login of whoever wrote it.
166    pub author: String,
167    /// File it was left on, for inline review comments.
168    pub path: Option<String>,
169    /// Line it was left on, when the comment is inline and still anchored.
170    pub line: Option<u64>,
171    /// The comment itself, as written.
172    pub body: String,
173}
174
175/// One observation of a pull request.
176#[derive(Debug, Clone, PartialEq, Eq)]
177pub struct PrState {
178    /// Pull request url, as `gh` reports it.
179    pub url: String,
180    /// Pull request number.
181    pub number: u64,
182    /// Open, merged, or closed.
183    pub state: PrLifecycle,
184    /// Aggregate check verdict.
185    pub checks: Checks,
186    /// Names of the checks that finished without passing.
187    pub failing: Vec<String>,
188    /// Comments that still want an answer, human and bot.
189    pub review_comments: Vec<ReviewComment>,
190    /// Whether the forge itself considers the failures blocking.
191    pub blocking: Blocking,
192}
193
194/// Whether a failing check actually stands between the pull request and
195/// `main`, according to the forge.
196///
197/// The rollup lists every check equally, so `coverage` going red on a
198/// repository that deliberately does not require it looked exactly like a
199/// broken build - and magi answered by spending a fix round on a change that
200/// was fine. Pull request 37 had to be merged by hand for that reason: the
201/// only red check was `editorconfig`, which was failing because the *action*
202/// could not fetch its own binary, and which the repository does not require.
203///
204/// `mergeStateStatus` is where GitHub applies the required-check set, so it
205/// is the one field that can tell the difference.
206#[derive(Debug, Clone, Copy, PartialEq, Eq)]
207pub enum Blocking {
208    /// Required checks are satisfied and the branch merges cleanly.
209    No,
210    /// Something required is failing or missing.
211    Yes,
212    /// The branch no longer merges: the base moved under it.
213    Conflict,
214    /// The forge did not say - an older `gh`, or a token without the scope.
215    /// Treated as `Yes`, because refusing to guess is the rule everywhere
216    /// else in this module.
217    Unsaid,
218}
219
220impl Blocking {
221    /// Read `mergeStateStatus`, which is upper-case in `gh`'s output.
222    fn of(raw: &str) -> Self {
223        match raw.to_ascii_uppercase().as_str() {
224            // Mergeable. `UNSTABLE` is the interesting one: mergeable, with a
225            // non-required check failing or still running.
226            "CLEAN" | "UNSTABLE" | "HAS_HOOKS" => Self::No,
227            "DIRTY" => Self::Conflict,
228            "" | "UNKNOWN" => Self::Unsaid,
229            // BLOCKED, BEHIND, DRAFT: something has to change first.
230            _ => Self::Yes,
231        }
232    }
233
234    /// Does this stand between the pull request and the base branch?
235    #[must_use]
236    pub fn stops_a_merge(self) -> bool {
237        !matches!(self, Self::No)
238    }
239}
240
241/// What the loop decided to do next. Pure, so the policy is testable.
242#[derive(Debug, Clone, PartialEq, Eq)]
243pub enum Step {
244    /// Checks are still running; re-read the pull request after [`POLL`].
245    Wait,
246    /// The base moved and the branch no longer merges: rebase it.
247    ///
248    /// Not a fix round. Nothing is wrong with the change - a competition
249    /// that runs for two hours against a repository merging pull requests
250    /// all day conflicts on the way in, and that is arithmetic rather than a
251    /// defect. Pull requests 35 and 37 were both rebased by hand for exactly
252    /// this.
253    Rebase,
254    /// Red checks or unresolved comments; run a fix round.
255    Fix {
256        /// What is unhappy, in one line, for the run log and the fix prompt.
257        reason: String,
258    },
259    /// Green and nothing outstanding; merge it.
260    Merge,
261    /// The pull request left our hands.
262    Done {
263        /// Did it land, or was it closed?
264        merged: bool,
265    },
266    /// Stop and leave the pull request to a person.
267    GiveUp {
268        /// Why magi stopped, in one line.
269        reason: String,
270    },
271}
272
273/// The outcome to record when `gh pr merge` exits non-zero, given what the
274/// pull request looked like immediately afterwards.
275///
276/// `gh pr merge` merges server-side first and only then does local work -
277/// deleting the branch, switching back to a base branch - so a non-zero exit
278/// does not mean the merge did not happen. In a jj-colocated repository it
279/// reliably does not mean that: git HEAD is detached, and `--delete-branch`
280/// ends with "could not determine current branch: not on any branch" *after*
281/// the merge has landed. Run ec12 merged pull request 28 into `main` and
282/// recorded `ok: false`, and its task was held waiting for a merge that was
283/// already done.
284///
285/// So the forge is asked, and its answer wins - the same authority [`decide`]
286/// gives the pull request's own state over everything else. The recorded
287/// detail carries both facts, because "the command failed and the merge
288/// happened anyway" is exactly what someone reading the run later needs to
289/// know.
290///
291/// `None` means the merge really did not happen, including when the pull
292/// request could not be read at all: an unreadable answer is not evidence of
293/// success.
294pub(crate) fn merged_after_all(
295    argv: &[String],
296    stderr: &str,
297    after: Option<PrLifecycle>,
298) -> Option<MergeOutcome> {
299    if after? != PrLifecycle::Merged {
300        return None;
301    }
302    Some(MergeOutcome {
303        mode: MergeMode::Pr,
304        ok: true,
305        detail: format!(
306            "gh {} (the command reported `{}`, but the pull request is merged)",
307            argv.join(" "),
308            stderr.trim()
309        ),
310    })
311}
312
313/// Decide the next step. No I/O.
314///
315/// `round` counts the fix rounds already spent, so `round == budget` means the
316/// budget is gone. A wait never spends a round: waiting is free, and a slow CI
317/// must not consume the allowance meant for actual fixes.
318///
319/// The order of the tests is the policy:
320///
321/// 1. **The pull request's own state wins.** A merge or a close that happened
322///    underneath us is the end of the story regardless of what the checks say.
323/// 2. **Pending beats red.** A check that is still running may yet fail, and one
324///    fix round that addresses every failure is cheaper than two that each
325///    address half - the fix pushes and restarts the whole suite anyway.
326/// 3. **Comments outrank green.** An unresolved comment holds the merge even
327///    when CI is happy; that is what a review is for.
328/// 4. **Unreadable is not absent.** Checks that cannot be read yet are waited
329///    on for [`CHECKS_GRACE`], because a pull request opened a moment ago has
330///    not been given its workflow runs yet. Past the grace they are treated as
331///    genuinely missing and magi stops rather than merge on a guess.
332pub fn decide(pr: &PrState, round: usize, budget: usize, waited: Duration) -> Step {
333    match pr.state {
334        PrLifecycle::Merged => return Step::Done { merged: true },
335        PrLifecycle::Closed => return Step::Done { merged: false },
336        PrLifecycle::Open => {}
337    }
338
339    // Before the checks: every check on a branch that cannot land is an
340    // answer about a state that cannot land.
341    if pr.blocking == Blocking::Conflict {
342        return Step::Rebase;
343    }
344
345    let spent = round >= budget;
346    match pr.checks {
347        Checks::Pending => Step::Wait,
348        Checks::Unknown if waited < CHECKS_GRACE => Step::Wait,
349        Checks::Unknown => Step::GiveUp {
350            reason: format!(
351                "no check status is readable on the pull request after {} minute(s); \
352                 refusing to merge on a guess",
353                CHECKS_GRACE.as_secs() / 60
354            ),
355        },
356        // Red, but the forge says it does not stand in the way: the failing
357        // checks are ones this repository chose not to require. Spending a fix
358        // round on them asks an agent to repair something nobody is gating on
359        // - and pull request 37's only red check was an *action* that could
360        // not fetch its own binary. Merge, and name them so the record is
361        // honest about what was red when it landed.
362        Checks::Red if !pr.blocking.stops_a_merge() && pr.review_comments.is_empty() => Step::Merge,
363        Checks::Red => {
364            let what = format!(
365                "{} check(s) failing: {}",
366                pr.failing.len(),
367                pr.failing.join(", ")
368            );
369            if spent {
370                Step::GiveUp {
371                    reason: format!("{what} — still red after {budget} fix round(s)"),
372                }
373            } else {
374                Step::Fix { reason: what }
375            }
376        }
377        Checks::Green if pr.review_comments.is_empty() => Step::Merge,
378        Checks::Green => {
379            let what = format!(
380                "checks are green but {} review comment(s) are unresolved: {}",
381                pr.review_comments.len(),
382                authors(&pr.review_comments)
383            );
384            if spent {
385                Step::GiveUp {
386                    reason: format!("{what} — still unresolved after {budget} fix round(s)"),
387                }
388            } else {
389                Step::Fix { reason: what }
390            }
391        }
392    }
393}
394
395/// Distinct comment authors, in the order they first appear.
396fn authors(comments: &[ReviewComment]) -> String {
397    let mut seen: Vec<&str> = Vec::new();
398    for c in comments {
399        if !seen.contains(&c.author.as_str()) {
400            seen.push(&c.author);
401        }
402    }
403    seen.join(", ")
404}
405
406/// The argv magi merges with, minus the program name.
407///
408/// `--subject` is the point of this function existing: see the module docs.
409pub fn merge_argv(number: u64, subject: &str) -> Vec<String> {
410    vec![
411        "pr".to_owned(),
412        "merge".to_owned(),
413        number.to_string(),
414        "--squash".to_owned(),
415        "--delete-branch".to_owned(),
416        "--subject".to_owned(),
417        subject.to_owned(),
418    ]
419}
420
421/// The squash subject to merge under.
422///
423/// The pull request title, unless it is empty or is a candidate branch's commit
424/// subject that leaked into the title - in which case the task's own first line
425/// is used, because `magi: candidate A (uncommitted work)` in `main` tells a
426/// reader nothing about what landed.
427pub fn merge_subject(pr_title: &str, instruction: &str) -> String {
428    let title = pr_title.trim();
429    if !title.is_empty() && !title.starts_with("magi: candidate") {
430        return title.to_owned();
431    }
432    let first = instruction
433        .lines()
434        .map(str::trim)
435        .find(|l| !l.is_empty())
436        .unwrap_or("magi: land the winning candidate");
437    first.trim_start_matches(['#', ' ']).to_owned()
438}
439
440/// The choice that lets the merge happen, verbatim as the owner taps it.
441pub const APPROVE: &str = "merge";
442
443/// The choice that leaves the pull request open.
444pub const HOLD: &str = "hold";
445
446/// Graph node recorded on the approval question.
447///
448/// The phone keys its high-stakes card off this rather than off the choice
449/// strings, so renaming a button cannot silently downgrade the card that
450/// guards the one irreversible action magi takes.
451pub const APPROVAL_NODE: &str = "land-approval";
452
453/// Unified diff lines carried in the panel before it is truncated.
454///
455/// Four hundred: the panel is read on a 390px phone, where a diff line often
456/// wraps to two rows, so this is already a few thousand rows of scrolling -
457/// past that nobody is reading, and the bytes still count against the panel's
458/// 8 MiB cap. A larger diff is not hidden: the note says how many lines were
459/// cut and which worktree holds the whole patch.
460pub const DIFF_MAX_LINES: usize = 400;
461
462/// What the owner's answer to the approval question means.
463#[derive(Debug, Clone, Copy, PartialEq, Eq)]
464pub enum Approval {
465    /// The owner said [`APPROVE`]. Merge.
466    Merge,
467    /// Anything else, including silence. Leave the pull request open.
468    Hold,
469}
470
471/// Read the owner's answer, where `None` is an unanswered question.
472///
473/// Silence is a hold. A timed-out question means the owner never saw it or
474/// never decided, and defaulting an irreversible merge to "yes" would make this
475/// gate worse than no gate at all: it would merge unattended while claiming to
476/// have asked. Only the exact [`APPROVE`] choice merges, so an answer this
477/// function does not recognise holds too.
478pub fn approval(answer: Option<&str>) -> Approval {
479    match answer {
480        Some(a) if a.trim().eq_ignore_ascii_case(APPROVE) => Approval::Merge,
481        _ => Approval::Hold,
482    }
483}
484
485/// What [`approval_gate`] found on one check of the owner's merge decision.
486#[derive(Debug, Clone, Copy, PartialEq, Eq)]
487enum ApprovalGate {
488    /// The owner said [`APPROVE`]. Merge.
489    Approved,
490    /// The owner said anything else, the question timed out, or it was
491    /// closed with no decision recorded.
492    Held,
493    /// Filed and still waiting - the caller parks rather than blocking on it.
494    Pending,
495}
496
497/// Escape text for HTML, including both quote characters.
498///
499/// Every string in the panel is agent-influenced: a branch name, a file path, a
500/// commit subject, a review comment. The sandboxed frame stops such text from
501/// *running*, but it does not stop a `<` from ending the document early or a
502/// `"` from ending an attribute and inventing a new one - the panel would then
503/// render a lie, or not render at all. Both quotes are escaped because the same
504/// function is used inside attributes, where remembering which quote style the
505/// caller used is one mistake away from an injected attribute.
506fn esc(s: &str) -> String {
507    let mut out = String::with_capacity(s.len());
508    for c in s.chars() {
509        match c {
510            '&' => out.push_str("&amp;"),
511            '<' => out.push_str("&lt;"),
512            '>' => out.push_str("&gt;"),
513            '"' => out.push_str("&quot;"),
514            '\'' => out.push_str("&#39;"),
515            _ => out.push(c),
516        }
517    }
518    out
519}
520
521/// One row of the diffstat table.
522#[derive(Debug, Clone, PartialEq, Eq)]
523struct StatRow {
524    path: String,
525    /// `None` for a binary file, which `git` reports as `-`.
526    added: Option<u64>,
527    removed: Option<u64>,
528}
529
530impl StatRow {
531    /// Lines touched, for sorting. A binary file counts as zero rather than as
532    /// unknown, which puts it at the bottom where it needs no attention.
533    fn churn(&self) -> u64 {
534        self.added.unwrap_or(0) + self.removed.unwrap_or(0)
535    }
536}
537
538/// Parse `git diff --numstat` into rows, biggest churn first.
539///
540/// `--numstat` and not `--stat`: the `+++---` bar in `--stat` is *scaled* to the
541/// terminal width, so counting its characters would print fabricated numbers in
542/// the one table an operator approves an irreversible action from.
543fn parse_numstat(numstat: &str) -> Vec<StatRow> {
544    let mut rows: Vec<StatRow> = numstat
545        .lines()
546        .filter_map(|line| {
547            let mut parts = line.splitn(3, '\t');
548            let added = parts.next()?.trim();
549            let removed = parts.next()?.trim();
550            let path = parts.next()?.trim();
551            if path.is_empty() {
552                return None;
553            }
554            Some(StatRow {
555                path: path.to_owned(),
556                added: added.parse().ok(),
557                removed: removed.parse().ok(),
558            })
559        })
560        .collect();
561    // Path breaks the tie so the same change always renders the same table; an
562    // operator comparing two panels should not see rows shuffle.
563    rows.sort_by(|a, b| b.churn().cmp(&a.churn()).then_with(|| a.path.cmp(&b.path)));
564    rows
565}
566
567/// How one diff line is shown: a gutter character, a style, and the body to
568/// print - which is the line minus its marker, so the marker appears exactly
569/// once, in the gutter.
570///
571/// The gutter is why this exists at all. The operator may be colour blind, or
572/// reading in sunlight with the screen dimmed, so an added line is never
573/// distinguished by its background alone: `+` and `-` sit in a fixed column,
574/// the same mark they already read in a terminal.
575fn diff_row(line: &str) -> (&'static str, &'static str, &str) {
576    if line.starts_with("+++") || line.starts_with("---") {
577        (" ", "color:#57606a;font-weight:600", line)
578    } else if let Some(body) = line.strip_prefix('+') {
579        ("+", "background:#e6ffec;color:#0a3622", body)
580    } else if let Some(body) = line.strip_prefix('-') {
581        ("-", "background:#ffebe9;color:#5c1a17", body)
582    } else if line.starts_with("@@") {
583        ("~", "background:#eef2ff;color:#3730a3", line)
584    } else if let Some(body) = line.strip_prefix(' ') {
585        (" ", "", body)
586    } else {
587        (" ", "color:#57606a;font-weight:600", line)
588    }
589}
590
591/// The handful of words the approval panel says in its own voice.
592///
593/// magi's own text, not an agent's, so `[graph] language` has to reach it too:
594/// the operator asked why the merge question spoke English on a repository
595/// configured for Japanese, and "because that string is a literal in Rust" is
596/// not an answer. Only the languages magi can actually check are translated;
597/// anything else falls back to English rather than shipping a guess, and that
598/// fallback is deliberate.
599struct Words {
600    html_lang: &'static str,
601    task: &'static str,
602    what_changed: &'static str,
603    review_verdict: &'static str,
604    reviewer: &'static str,
605    reviewer_no_answer: &'static str,
606    checks: &'static str,
607    nothing_failing: &'static str,
608    files_changed: &'static str,
609    commits: &'static str,
610    no_commits: &'static str,
611    comments: &'static str,
612    no_comments: &'static str,
613    diff: &'static str,
614    truncated: &'static str,
615    lands_as: &'static str,
616}
617
618const EN: Words = Words {
619    html_lang: "en",
620    task: "Task",
621    what_changed: "What changed",
622    review_verdict: "Review verdict",
623    reviewer: "Reviewer",
624    reviewer_no_answer: "produced no answer",
625    checks: "Checks",
626    nothing_failing: "Nothing failing.",
627    files_changed: "file(s) changed",
628    commits: "Commits being squashed",
629    no_commits: "No commit subjects could be read from the branch.",
630    comments: "Review comments",
631    no_comments: "Nothing outstanding at this observation.",
632    diff: "Diff",
633    truncated: "Truncated",
634    lands_as: "They land as one commit titled",
635};
636
637const JA: Words = Words {
638    html_lang: "ja",
639    task: "タスク",
640    what_changed: "変更内容",
641    review_verdict: "レビューの結論",
642    reviewer: "レビュアー",
643    reviewer_no_answer: "回答なし",
644    checks: "チェック",
645    nothing_failing: "失敗しているものはありません。",
646    files_changed: "ファイル変更",
647    commits: "squash されるコミット",
648    no_commits: "ブランチからコミット件名を読めませんでした。",
649    comments: "レビューコメント",
650    no_comments: "この時点で未対応のものはありません。",
651    diff: "差分",
652    truncated: "省略",
653    lands_as: "これらは次の件名の1コミットとして入ります:",
654};
655
656impl Words {
657    /// The clause after the merge subject. Split out because word order moves:
658    /// Japanese puts the subject before the verb, so a shared template with a
659    /// hole in the middle would read as machine translation.
660    fn lands_as_tail(&self) -> &'static str {
661        if self.html_lang == "ja" {
662            "。この件名も承認の対象です。"
663        } else {
664            ", which you are approving too."
665        }
666    }
667
668    /// The question's own one-line summary, which is what a phone shows first.
669    fn approval_summary(&self, number: u64, subject: &str) -> String {
670        if self.html_lang == "ja" {
671            format!("プルリクエスト #{number} をマージ: {subject}")
672        } else {
673            format!("merge pull request #{number}: {subject}")
674        }
675    }
676
677    /// The body under the summary, above the panel.
678    fn approval_detail(&self, url: &str, base: &str, subject: &str) -> String {
679        if self.html_lang == "ja" {
680            format!(
681                "{url} はチェックが緑で、`{base}` へ `{subject}` として squash \
682                 できる状態です。差分の要約・パッチ・squash されるコミットは\
683                 下のパネルにあります。"
684            )
685        } else {
686            format!(
687                "{url} is green and ready to squash into `{base}` as `{subject}`. \
688                 The panel holds the diffstat, the patch and the commits being squashed."
689            )
690        }
691    }
692
693    /// The truncation note, written whole in each language for the same reason.
694    fn truncated_note(
695        &self,
696        omitted: usize,
697        total: usize,
698        shown: usize,
699        where_: &str,
700        base: &str,
701        head: &str,
702    ) -> String {
703        if self.html_lang == "ja" {
704            format!(
705                "先頭 {shown} 行のあと、差分 {total} 行のうち {omitted} 行を省略しました。\
706                 全体は <code>{where_}</code>(<code>git diff {base}...{head}</code>)と\
707                 プルリクエストにあります。"
708            )
709        } else {
710            format!(
711                "{omitted} of {total} diff lines omitted after the first {shown}. \
712                 The whole patch is in <code>{where_}</code> \
713                 (<code>git diff {base}...{head}</code>) and on the pull request."
714            )
715        }
716    }
717}
718
719/// Pick the panel's language. Codes and names both, because `[graph] language`
720/// has always accepted either.
721fn words(language: &str) -> &'static Words {
722    let l = language.trim();
723    if l.eq_ignore_ascii_case("ja")
724        || l.eq_ignore_ascii_case("jp")
725        || l.eq_ignore_ascii_case("japanese")
726        || l.eq_ignore_ascii_case("日本語")
727    {
728        &JA
729    } else {
730        &EN
731    }
732}
733
734/// The approval panel's html: what is about to land, and the evidence for it.
735///
736/// Pure, so the whole document is asserted in tests without `gh`, without a
737/// network and without a repository. The caller gathers `diffstat`
738/// (`git diff --numstat`), `diff` (the unified patch), `commits` (the subjects
739/// being squashed) and `subject` (what the squash will be called) from the
740/// winner's worktree.
741///
742/// It emits no `<script>`, no `<form>` and no remote url, because the frame's
743/// content security policy blocks all three: anything of the sort here would be
744/// dead markup that misleads the next reader into thinking it works.
745pub fn approval_panel(
746    state: &RunState,
747    pr: &PrState,
748    diffstat: &str,
749    diff: &str,
750    commits: &[String],
751    subject: &str,
752) -> String {
753    let rows = parse_numstat(diffstat);
754    let w = words(&state.config.graph.language);
755    let mut h = String::with_capacity(4_096 + diff.len().min(200_000));
756
757    let _ = writeln!(
758        h,
759        "<!doctype html>\n<html lang=\"{}\">\n<head>\n<meta charset=\"utf-8\">\n\
760         <meta name=\"viewport\" content=\"width=device-width, initial-scale=1\">",
761        w.html_lang
762    );
763    let _ = writeln!(
764        h,
765        "<title>merge #{} — {}</title>\n</head>",
766        pr.number,
767        esc(subject)
768    );
769    h.push_str(
770        "<body style=\"margin:0;padding:12px;font:15px/1.5 -apple-system,\
771         'Segoe UI',system-ui,sans-serif;color:#1f2328;background:#fff;\
772         word-break:break-word\">\n",
773    );
774
775    // The decision, in the words the operator is approving.
776    let _ = writeln!(
777        h,
778        "<h1 style=\"margin:0 0 4px;font-size:19px\">Merge #{} into \
779         <code style=\"background:#f6f8fa;padding:1px 4px;border-radius:4px\">{}</code></h1>\n\
780         <p style=\"margin:0 0 4px;font-size:17px;font-weight:600\">{}</p>\n\
781         <p style=\"margin:0 0 12px;font-size:13px;color:#57606a\">squash merge · run {} · \
782         <a href=\"{}\" style=\"color:#0969da\">{}</a></p>",
783        pr.number,
784        esc(&state.base_branch),
785        esc(subject),
786        esc(&state.id),
787        esc(&pr.url),
788        esc(&pr.url),
789    );
790
791    // The task, verbatim: the operator's own words for what was asked, so the
792    // panel does not make them reconstruct the request from a diffstat.
793    let _ = writeln!(
794        h,
795        "<h2 style=\"margin:16px 0 6px;font-size:15px\">{}</h2>\n\
796         <p style=\"margin:0;font-size:13px;white-space:pre-wrap\">{}</p>",
797        w.task,
798        esc(&state.instruction)
799    );
800
801    // The winner's own account of what it did and why, when there is one.
802    if let Some(summary) = state
803        .winner()
804        .map(|c| c.summary.as_str())
805        .filter(|s| !s.is_empty())
806    {
807        let _ = writeln!(
808            h,
809            "<h2 style=\"margin:16px 0 6px;font-size:15px\">{}</h2>\n\
810             <p style=\"margin:0;font-size:13px;white-space:pre-wrap\">{}</p>",
811            w.what_changed,
812            esc(summary)
813        );
814    }
815
816    // The verdict from the round that actually cleared this for merge - the
817    // last one, since only that round's word is still standing.
818    if let Some(round) = state.reviews.last() {
819        let _ = writeln!(
820            h,
821            "<h2 style=\"margin:16px 0 6px;font-size:15px\">{}</h2>",
822            w.review_verdict
823        );
824        for r in &round.reviews {
825            // A seat the review loop counted as answered has real prose in
826            // `summary`; one it counted against `incomplete` (see
827            // `graph::Runner::review_loop`) never produced any and left it
828            // empty - which must not be read back as a blank verdict, since
829            // an empty box here looks like "nothing to say" rather than
830            // "never answered".
831            let body = match &r.failed {
832                Some(reason) => format!("{}: {}", w.reviewer_no_answer, esc(reason)),
833                None => esc(&r.summary),
834            };
835            let _ = writeln!(
836                h,
837                "<div style=\"margin:0 0 8px;padding:8px;background:#f6f8fa;\
838                 border-radius:6px\">\
839                 <div style=\"font-size:12px;color:#57606a\">{} {} · {}</div>\
840                 <div style=\"white-space:pre-wrap;font-size:13px\">{}</div></div>",
841                w.reviewer,
842                r.reviewer,
843                esc(&r.agent),
844                body,
845            );
846        }
847    }
848
849    let _ = writeln!(
850        h,
851        "<h2 style=\"margin:16px 0 6px;font-size:15px\">{}: {}</h2>",
852        w.checks,
853        esc(pr.checks.as_str())
854    );
855    if pr.failing.is_empty() {
856        let _ = writeln!(
857            h,
858            "<p style=\"margin:0;font-size:13px;color:#57606a\">{}</p>",
859            w.nothing_failing
860        );
861    } else {
862        h.push_str("<ul style=\"margin:0;padding-left:20px;font-size:13px\">\n");
863        for f in &pr.failing {
864            let _ = writeln!(h, "<li>{}</li>", esc(f));
865        }
866        h.push_str("</ul>\n");
867    }
868
869    // Diffstat as a real table, so a phone reads what moved without scrolling
870    // sideways through a terminal bar chart.
871    let _ = writeln!(
872        h,
873        "<h2 style=\"margin:16px 0 6px;font-size:15px\">{} {}</h2>",
874        rows.len(),
875        w.files_changed
876    );
877    h.push_str(
878        "<table style=\"width:100%;border-collapse:collapse;font-size:13px\">\n\
879         <thead><tr>\
880         <th style=\"text-align:left;border-bottom:1px solid #d0d7de;padding:4px 2px\">file</th>\
881         <th style=\"text-align:right;border-bottom:1px solid #d0d7de;padding:4px 2px\">added</th>\
882         <th style=\"text-align:right;border-bottom:1px solid #d0d7de;padding:4px 2px\">removed\
883         </th></tr></thead>\n<tbody>\n",
884    );
885    let mut total_added = 0u64;
886    let mut total_removed = 0u64;
887    for r in &rows {
888        total_added += r.added.unwrap_or(0);
889        total_removed += r.removed.unwrap_or(0);
890        let cell = |n: Option<u64>| match n {
891            Some(n) => n.to_string(),
892            None => "bin".to_owned(),
893        };
894        let _ = writeln!(
895            h,
896            "<tr>\
897             <td style=\"padding:4px 2px;border-bottom:1px solid #eaeef2;\
898             font-family:ui-monospace,monospace\">{}</td>\
899             <td style=\"padding:4px 2px;border-bottom:1px solid #eaeef2;text-align:right;\
900             color:#0a3622\">{}</td>\
901             <td style=\"padding:4px 2px;border-bottom:1px solid #eaeef2;text-align:right;\
902             color:#5c1a17\">{}</td></tr>",
903            esc(&r.path),
904            cell(r.added),
905            cell(r.removed),
906        );
907    }
908    let _ = writeln!(
909        h,
910        "</tbody>\n<tfoot><tr style=\"font-weight:600\">\
911         <td style=\"padding:4px 2px\">total</td>\
912         <td style=\"padding:4px 2px;text-align:right\">{total_added}</td>\
913         <td style=\"padding:4px 2px;text-align:right\">{total_removed}</td>\
914         </tr></tfoot>\n</table>"
915    );
916
917    // The commits being squashed, and the subject that replaces them.
918    let _ = writeln!(
919        h,
920        "<h2 style=\"margin:16px 0 6px;font-size:15px\">{}</h2>",
921        w.commits
922    );
923    if commits.is_empty() {
924        h.push_str(&format!(
925            "<p style=\"margin:0;font-size:13px;color:#57606a\">{}</p>\n",
926            w.no_commits
927        ));
928    } else {
929        h.push_str("<ol style=\"margin:0;padding-left:20px;font-size:13px\">\n");
930        for c in commits {
931            let _ = writeln!(h, "<li>{}</li>", esc(c));
932        }
933        h.push_str("</ol>\n");
934    }
935    let _ = writeln!(
936        h,
937        "<p style=\"margin:8px 0 0;font-size:13px\">{} <strong>{}</strong>{}</p>",
938        w.lands_as,
939        esc(subject),
940        w.lands_as_tail()
941    );
942
943    // The review comments that shaped this branch, and who asked for them.
944    let _ = writeln!(
945        h,
946        "<h2 style=\"margin:16px 0 6px;font-size:15px\">{}</h2>",
947        w.comments
948    );
949    if pr.review_comments.is_empty() {
950        h.push_str(&format!(
951            "<p style=\"margin:0;font-size:13px;color:#57606a\">{}</p>\n",
952            w.no_comments
953        ));
954    } else {
955        for c in &pr.review_comments {
956            let anchor = match (&c.path, c.line) {
957                (Some(p), Some(l)) => format!("{p}:{l}"),
958                (Some(p), None) => p.clone(),
959                _ => "pull request thread".to_owned(),
960            };
961            let _ = writeln!(
962                h,
963                "<div style=\"margin:0 0 8px;padding:8px;background:#f6f8fa;border-radius:6px\">\
964                 <div style=\"font-size:12px;color:#57606a\">{} · {}</div>\
965                 <div style=\"white-space:pre-wrap;font-size:13px\">{}</div></div>",
966                esc(&c.author),
967                esc(&anchor),
968                esc(&tail(&c.body, 800)),
969            );
970        }
971    }
972
973    // The patch itself.
974    let total = diff.lines().count();
975    let shown = total.min(DIFF_MAX_LINES);
976    let _ = writeln!(
977        h,
978        "<h2 style=\"margin:16px 0 6px;font-size:15px\">{}</h2>",
979        w.diff
980    );
981    h.push_str(
982        "<div style=\"font:12px/1.45 ui-monospace,SFMono-Regular,Menlo,monospace;\
983         border:1px solid #d0d7de;border-radius:6px;overflow-x:auto\">\n",
984    );
985    for line in diff.lines().take(shown) {
986        let (gutter, style, body) = diff_row(line);
987        let _ = writeln!(
988            h,
989            "<div style=\"display:flex;{style}\">\
990             <span style=\"flex:0 0 1.4em;text-align:center;user-select:none;\
991             border-right:1px solid #d0d7de\">{gutter}</span>\
992             <span style=\"white-space:pre;padding-left:6px\">{}</span></div>",
993            esc(body),
994        );
995    }
996    h.push_str("</div>\n");
997    if total > shown {
998        let omitted = total - shown;
999        let head = state.winner().map_or("HEAD", |w| w.branch.as_str());
1000        let where_ = state.winner().map_or_else(
1001            || state.repo.display().to_string(),
1002            |w| w.worktree.display().to_string(),
1003        );
1004        let _ = writeln!(
1005            h,
1006            "<p style=\"margin:8px 0 0;padding:8px;background:#fff8c5;border-radius:6px;\
1007             font-size:13px\">{}: {}</p>",
1008            w.truncated,
1009            w.truncated_note(
1010                omitted,
1011                total,
1012                shown,
1013                &esc(&where_),
1014                &esc(&state.base_branch),
1015                &esc(head),
1016            ),
1017        );
1018    }
1019
1020    h.push_str("</body>\n</html>\n");
1021    h
1022}
1023
1024/// Ask the owner before merging, with the whole case attached as a panel.
1025///
1026/// The evidence is gathered from the winner's own worktree with the `git` CLI,
1027/// never from the network, so a phone on a slow link gets the diff magi is
1028/// looking at rather than a link it has to go and open.
1029///
1030/// Never blocks. `land` used to sit inside [`ask::ask_and_wait`]'s poll loop
1031/// for up to a day right here, which held the whole run's task claim - and
1032/// the daemon's one slot with it - for exactly as long as the owner took to
1033/// notice their phone. [`ApprovalGate::Pending`] is the answer that lets the
1034/// caller park the run and hand the slot back instead: the question is on
1035/// disk either way, so nothing about the wait itself changes, only who is
1036/// blocked on it.
1037///
1038/// Idempotent across resumes: called again for a run already waiting on its
1039/// own question, this finds that question by [`crate::ask::Questions::list`]
1040/// rather than filing a second one - asking twice would double the
1041/// notification for one decision, and leave the first question's panel an
1042/// orphan nobody's answer ever reaches.
1043async fn approval_gate(state: &mut RunState, pr: &PrState, subject: &str) -> Result<ApprovalGate> {
1044    let store = ask::Questions::open();
1045    let existing = store
1046        .list()
1047        .into_iter()
1048        .filter(|q| q.run == state.id && q.node == APPROVAL_NODE)
1049        .max_by(|a, b| a.id.cmp(&b.id));
1050
1051    let q = match existing {
1052        Some(q) => q,
1053        None => {
1054            let (worktree, head) = match state.winner() {
1055                Some(w) => (w.worktree.clone(), w.branch.clone()),
1056                None => (state.repo.clone(), "HEAD".to_owned()),
1057            };
1058            let base = state.base_branch.clone();
1059            let range = format!("{base}...{head}");
1060            // A failed `git` must not decide the merge: the panel degrades to
1061            // less evidence and the owner still chooses. Merging because the
1062            // diff could not be read would be the worst of both.
1063            let numstat = git::git_raw(&worktree, &["diff", "--numstat", "-M", &range])
1064                .await
1065                .map(|o| o.stdout)
1066                .unwrap_or_default();
1067            let diff = git::diff(&worktree, &base, &head).await.unwrap_or_default();
1068            let commits: Vec<String> = git::git_raw(
1069                &worktree,
1070                &[
1071                    "log",
1072                    "--reverse",
1073                    "--format=%s",
1074                    &format!("{base}..{head}"),
1075                ],
1076            )
1077            .await
1078            .map(|o| o.stdout)
1079            .unwrap_or_default()
1080            .lines()
1081            .filter(|l| !l.trim().is_empty())
1082            .map(str::to_owned)
1083            .collect();
1084
1085            let w = words(&state.config.graph.language);
1086            let html = approval_panel(state, pr, &numstat, &diff, &commits, subject);
1087            let mut fresh = ask::Question::new(
1088                state.id.clone(),
1089                APPROVAL_NODE.to_owned(),
1090                "land".to_owned(),
1091                w.approval_summary(pr.number, subject),
1092                w.approval_detail(&pr.url, &base, subject),
1093                vec![APPROVE.to_owned(), HOLD.to_owned()],
1094            );
1095            store
1096                .put_panel(&mut fresh, &html, &[])
1097                .context("write the merge approval panel")?;
1098            store
1099                .put(&mut fresh)
1100                .context("file the merge approval question")?;
1101            state.event(
1102                "land",
1103                format!("asking for merge approval ({})", fresh.short()),
1104            );
1105            state.save()?;
1106            if let Err(e) = ask::notify(&state.config.notify, &fresh).await {
1107                // A broken webhook is not a reason to lose the merge: the
1108                // question is already on disk and the web UI already shows
1109                // it, so the operator still has a way in.
1110                tracing::warn!(
1111                    "could not notify about merge approval question {}: {e:#} - \
1112                     the web UI is the only surface for it now",
1113                    fresh.short()
1114                );
1115            }
1116            fresh
1117        }
1118    };
1119
1120    Ok(match q.status {
1121        ask::QuestionStatus::Open => ApprovalGate::Pending,
1122        // Nobody answered before `state.config.graph.answer_timeout` passed,
1123        // or the question was closed with no decision recorded underneath
1124        // this run - either way there is nothing left to wait on.
1125        ask::QuestionStatus::Abandoned => ApprovalGate::Held,
1126        // The merge gate does not speak `--thread`: an owner who talked back
1127        // instead of choosing never reaches `Answered`, so this arm only
1128        // ever sees an actual decision.
1129        ask::QuestionStatus::Answered => match approval(q.resolution().as_deref()) {
1130            Approval::Merge => ApprovalGate::Approved,
1131            Approval::Hold => ApprovalGate::Held,
1132        },
1133    })
1134}
1135
1136/// Parse `gh pr view --json url,number,state,statusCheckRollup,reviews,comments`
1137/// output into a [`PrState`]. No I/O.
1138pub fn parse_pr(json: &str) -> Result<PrState> {
1139    let raw: GhPr = serde_json::from_str(json).context("parse `gh pr view --json ...` output")?;
1140    let state = match raw.state.to_ascii_uppercase().as_str() {
1141        "OPEN" => PrLifecycle::Open,
1142        "MERGED" => PrLifecycle::Merged,
1143        "CLOSED" => PrLifecycle::Closed,
1144        other => bail!("unknown pull request state `{other}`"),
1145    };
1146
1147    let mut failing = Vec::new();
1148    let mut pending = false;
1149    let mut unknown = false;
1150    for check in &raw.status_check_rollup {
1151        match check.verdict() {
1152            Verdict::Pass => {}
1153            Verdict::Pending => pending = true,
1154            Verdict::Fail => failing.push(check.label()),
1155            Verdict::Unknown => unknown = true,
1156        }
1157    }
1158    let checks = if raw.status_check_rollup.is_empty() {
1159        Checks::Unknown
1160    } else if pending {
1161        Checks::Pending
1162    } else if !failing.is_empty() {
1163        Checks::Red
1164    } else if unknown {
1165        Checks::Unknown
1166    } else {
1167        Checks::Green
1168    };
1169
1170    let mut review_comments = Vec::new();
1171    for r in raw.reviews {
1172        push_if_outstanding(
1173            &mut review_comments,
1174            ReviewComment {
1175                author: r.author.login,
1176                path: None,
1177                line: None,
1178                body: r.body,
1179            },
1180        );
1181    }
1182    for c in raw.comments {
1183        push_if_outstanding(
1184            &mut review_comments,
1185            ReviewComment {
1186                author: c.author.login,
1187                path: None,
1188                line: None,
1189                body: c.body,
1190            },
1191        );
1192    }
1193
1194    Ok(PrState {
1195        url: raw.url,
1196        number: raw.number,
1197        state,
1198        checks,
1199        failing,
1200        review_comments,
1201        blocking: Blocking::of(&raw.merge_state_status),
1202    })
1203}
1204
1205/// Parse `gh api repos/{owner}/{repo}/pulls/<n>/comments` into inline review
1206/// comments. No I/O.
1207///
1208/// `gh pr view` does not surface inline comments, and inline is exactly where
1209/// both review bots put their findings - a landing loop that read only the
1210/// top-level thread would never see the thing it is supposed to fix.
1211pub fn parse_inline_comments(json: &str) -> Result<Vec<ReviewComment>> {
1212    let raw: Vec<GhInline> =
1213        serde_json::from_str(json).context("parse `gh api .../pulls/<n>/comments` output")?;
1214    let mut out = Vec::new();
1215    for c in raw {
1216        push_if_outstanding(
1217            &mut out,
1218            ReviewComment {
1219                author: c.user.login,
1220                path: c.path,
1221                line: c.line,
1222                body: c.body,
1223            },
1224        );
1225    }
1226    Ok(out)
1227}
1228
1229/// Keep a comment only when it asks for something.
1230///
1231/// An inline comment always does: it names a file and a line. A top-level
1232/// comment is dropped when it is empty, when it is magi's own, or when it is
1233/// [noise](is_noise).
1234fn push_if_outstanding(out: &mut Vec<ReviewComment>, comment: ReviewComment) {
1235    if comment.body.trim().is_empty() || comment.body.contains(MARKER) {
1236        return;
1237    }
1238    if comment.path.is_none() && is_noise(&comment.body) {
1239        return;
1240    }
1241    out.push(comment);
1242}
1243
1244/// Is this comment body machinery rather than a finding?
1245///
1246/// Two tests, both structural, because guessing from prose is how a "looks
1247/// good to me" turns into a fix round:
1248///
1249/// 1. The bot said so - the body carries one of the [`NOT_A_REVIEW`] markers
1250///    with which CodeRabbit labels its trigger notice, its walkthrough, and its
1251///    footer.
1252/// 2. It asks for nothing - once HTML comments, `<details>` blocks, headings,
1253///    horizontal rules, and the bot's own status banner are removed, every
1254///    remaining line is a task-list item. That is exactly the shape of the
1255///    comment the Claude review job posts while it is still working.
1256///
1257/// Anything else is input, including bot prose. A bot that writes a paragraph
1258/// has said something, and the fix prompt tells the fixer it may decline a
1259/// comment with an argument - a wasted sentence in a prompt is cheaper than a
1260/// missed finding.
1261pub fn is_noise(body: &str) -> bool {
1262    if NOT_A_REVIEW.iter().any(|m| body.contains(m)) {
1263        return true;
1264    }
1265    let mut content = false;
1266    for line in strip_blocks(body).lines() {
1267        let line = unquote(line);
1268        if line.is_empty() || is_checklist(line) || is_decoration(line) || is_banner(line) {
1269            continue;
1270        }
1271        content = true;
1272        break;
1273    }
1274    !content
1275}
1276
1277/// Remove HTML comments and collapsed `<details>` blocks.
1278fn strip_blocks(body: &str) -> String {
1279    let mut out = String::with_capacity(body.len());
1280    let mut rest = body;
1281    loop {
1282        let open = ["<!--", "<details>"]
1283            .iter()
1284            .filter_map(|tag| rest.find(tag).map(|i| (i, *tag)))
1285            .min_by_key(|(i, _)| *i);
1286        let Some((at, tag)) = open else {
1287            out.push_str(rest);
1288            return out;
1289        };
1290        out.push_str(&rest[..at]);
1291        let after = &rest[at + tag.len()..];
1292        let close = if tag == "<!--" { "-->" } else { "</details>" };
1293        match after.find(close) {
1294            Some(end) => rest = &after[end + close.len()..],
1295            // Unterminated: the rest of the body is inside the block.
1296            None => return out,
1297        }
1298    }
1299}
1300
1301/// Strip blockquote markers, which both bots wrap their callouts in.
1302fn unquote(line: &str) -> &str {
1303    let mut s = line.trim();
1304    while let Some(rest) = s.strip_prefix('>') {
1305        s = rest.trim_start();
1306    }
1307    s.trim()
1308}
1309
1310/// `- [ ]` / `- [x]`, in any of the bullet styles GitHub renders.
1311fn is_checklist(line: &str) -> bool {
1312    let rest = line
1313        .strip_prefix("- ")
1314        .or_else(|| line.strip_prefix("* "))
1315        .unwrap_or("");
1316    let rest = rest.trim_start();
1317    matches!(
1318        rest.get(..3),
1319        Some("[ ]") | Some("[x]") | Some("[X]") | Some("[*]")
1320    )
1321}
1322
1323/// A heading, a horizontal rule, or a callout tag - shape, never content.
1324fn is_decoration(line: &str) -> bool {
1325    line.starts_with('#')
1326        || line.starts_with("[!")
1327        || (line.len() >= 3 && line.chars().all(|c| matches!(c, '-' | '=' | '*' | '_')))
1328}
1329
1330/// A line that is nothing but emphasis and links.
1331///
1332/// Both review jobs open with a status banner
1333/// (`**Claude finished ... in 4m 14s** —— [View job](url)`). It reads as prose
1334/// to a line-based test and asks for nothing, so it is measured the same way a
1335/// heading is: strip the markup, and if no word survives, it was decoration.
1336fn is_banner(line: &str) -> bool {
1337    let plain = drop_spans(line, "**", "**");
1338    let plain = if plain.contains("](") {
1339        drop_spans(&plain, "[", ")")
1340    } else {
1341        plain
1342    };
1343    !plain.chars().any(char::is_alphanumeric)
1344}
1345
1346/// Remove every `open` .. `close` span, including the delimiters. An
1347/// unterminated span swallows the rest of the input, which is what a reader
1348/// sees too.
1349fn drop_spans(s: &str, open: &str, close: &str) -> String {
1350    let mut out = String::with_capacity(s.len());
1351    let mut rest = s;
1352    while let Some(at) = rest.find(open) {
1353        out.push_str(&rest[..at]);
1354        let after = &rest[at + open.len()..];
1355        match after.find(close) {
1356            Some(end) => rest = &after[end + close.len()..],
1357            None => return out,
1358        }
1359    }
1360    out.push_str(rest);
1361    out
1362}
1363
1364/// The lock that keeps at most one run per repository actually moving the
1365/// base branch at a time: a rebase push, or `gh pr merge`.
1366///
1367/// Deliberately narrow. Everything else in [`land`]'s loop - watching CI,
1368/// running a fix round in the winner's own worktree, waiting on the owner's
1369/// approval - touches nothing a *different* run in the same repository could
1370/// collide with, and holding a lock across any of that would serialise one
1371/// run's CI wait (up to [`WAIT_CEILING`]) against another run's land-approval
1372/// resume, which is precisely the "must not wait on another task" property
1373/// the daemon's slot-freeing exists to give a resume. Only the two moments
1374/// that actually write to the shared base branch need mutual exclusion, and
1375/// both are brief.
1376///
1377/// One entry per repository, each its own `tokio::sync::Mutex`, so two
1378/// different repositories' runs never wait on each other. The outer
1379/// `std::sync::Mutex` guards only the map itself, held long enough to find or
1380/// insert an entry and clone its `Arc`, never across an `.await`.
1381fn repo_merge_lock(repo: &Path) -> Arc<tokio::sync::Mutex<()>> {
1382    static LOCKS: std::sync::LazyLock<
1383        std::sync::Mutex<BTreeMap<PathBuf, Arc<tokio::sync::Mutex<()>>>>,
1384    > = std::sync::LazyLock::new(|| std::sync::Mutex::new(BTreeMap::new()));
1385    LOCKS
1386        .lock()
1387        .unwrap_or_else(std::sync::PoisonError::into_inner)
1388        .entry(repo.to_path_buf())
1389        .or_insert_with(|| Arc::new(tokio::sync::Mutex::new(())))
1390        .clone()
1391}
1392
1393/// Run the loop against a real pull request until it merges or the budget runs
1394/// out.
1395///
1396/// The caller decides whether landing happens at all: this is only reached when
1397/// `graph.land` is on. Returns the last observation, so the caller can report
1398/// what magi was looking at when it stopped.
1399pub async fn land(state: &mut RunState, pr_url: &str) -> Result<PrState> {
1400    let repo = state.repo.clone();
1401    let budget = state.config.graph.land_rounds;
1402    let mut round = 0usize;
1403    // Counted apart from `round`: a rebase is not a fix, and a base that
1404    // moved is not the change's fault.
1405    let mut rebases = 0usize;
1406    let mut waited = Duration::ZERO;
1407    // Comment bodies the fixer has already been shown. A comment is
1408    // outstanding until it has been handed over once; after that it is a
1409    // recorded decision, not an open question, and re-feeding it would loop the
1410    // budget away on a comment the fixer already declined with an argument.
1411    let mut shown: BTreeSet<String> = BTreeSet::new();
1412
1413    // Marks the run resumable through exactly this function, not through a
1414    // fresh competition: `RunStatus::resumable` excludes only `Merged`,
1415    // `Ready` and `Failed`, and `merge`'s own re-entry guard looks for this
1416    // status specifically to know a resumed run belongs back in `land`
1417    // rather than at a second `gh pr create`. Set on every entry - fresh or
1418    // resumed - because a resume that parked here again must keep reading
1419    // `Landing`, not whatever a first pass through `merge` left behind.
1420    state.status = RunStatus::Landing;
1421    state.event("land", format!("watching {pr_url}"));
1422    state.save()?;
1423
1424    loop {
1425        let seen = observe(&repo, pr_url).await?;
1426        let mut pr = seen.pr;
1427        pr.review_comments.retain(|c| !shown.contains(&c.body));
1428        state.pr = Some(crate::run::PrRecord {
1429            url: pr.url.clone(),
1430            number: pr.number,
1431            state: pr.state.as_str().to_owned(),
1432            checks: pr.checks.as_str().to_owned(),
1433            round,
1434            rounds: budget,
1435        });
1436        state.save()?;
1437
1438        match decide(&pr, round, budget, waited) {
1439            Step::Wait => {
1440                if waited >= WAIT_CEILING {
1441                    let why = format!(
1442                        "checks were still running after {} minutes",
1443                        WAIT_CEILING.as_secs() / 60
1444                    );
1445                    stop(state, &repo, &pr, &why).await?;
1446                    return Ok(pr);
1447                }
1448                waited += POLL;
1449                tokio::time::sleep(POLL).await;
1450            }
1451            Step::Done { merged } => {
1452                state.status = if merged {
1453                    RunStatus::Merged
1454                } else {
1455                    RunStatus::Ready
1456                };
1457                let detail = if merged {
1458                    format!("{} was merged", pr.url)
1459                } else {
1460                    format!("{} was closed without merging", pr.url)
1461                };
1462                state.merge = Some(MergeOutcome {
1463                    mode: MergeMode::Pr,
1464                    ok: merged,
1465                    detail: detail.clone(),
1466                });
1467                state.event("land", detail);
1468                state.save()?;
1469                return Ok(pr);
1470            }
1471            Step::Merge => {
1472                let subject = merge_subject(&seen.title, &state.instruction);
1473                // The owner sees the panel before the one irreversible step,
1474                // and an unanswered question is a hold: silence never merges.
1475                if state.config.graph.land_approval {
1476                    match approval_gate(state, &pr, &subject).await? {
1477                        ApprovalGate::Approved => {}
1478                        ApprovalGate::Held => {
1479                            stop(
1480                                state,
1481                                &repo,
1482                                &pr,
1483                                "the owner did not approve the merge (held or unanswered)",
1484                            )
1485                            .await?;
1486                            return Ok(pr);
1487                        }
1488                        // Filed (or still standing from an earlier visit) and
1489                        // not yet answered. Park here rather than wait: the
1490                        // question survives on disk, the daemon hands this
1491                        // run's slot to something else, and a later resume
1492                        // re-enters `land`, finds the same question, and
1493                        // either merges or stops depending on what it says
1494                        // by then.
1495                        ApprovalGate::Pending => {
1496                            state.parked = true;
1497                            state.event(
1498                                "land",
1499                                "parked awaiting merge approval - resumes once answered",
1500                            );
1501                            state.save()?;
1502                            return Ok(pr);
1503                        }
1504                    }
1505                }
1506                let argv = merge_argv(pr.number, &subject);
1507                let out = {
1508                    let merge_lock = repo_merge_lock(&repo);
1509                    let _merge_slot = merge_lock.lock().await;
1510                    gh(&repo, &argv).await?
1511                };
1512                if out.0 {
1513                    state.status = RunStatus::Merged;
1514                    state.merge = Some(MergeOutcome {
1515                        mode: MergeMode::Pr,
1516                        ok: true,
1517                        detail: format!("gh {}", argv.join(" ")),
1518                    });
1519                    state.event("land", format!("merged {} as `{subject}`", pr.url));
1520                    state.save()?;
1521                    pr.state = PrLifecycle::Merged;
1522                    return Ok(pr);
1523                }
1524                let after = observe(&repo, pr_url).await.ok().map(|s| s.pr.state);
1525                if let Some(outcome) = merged_after_all(&argv, &out.1, after) {
1526                    state.status = RunStatus::Merged;
1527                    state.merge = Some(outcome);
1528                    state.event("land", format!("merged {} as `{subject}`", pr.url));
1529                    state.save()?;
1530                    pr.state = PrLifecycle::Merged;
1531                    return Ok(pr);
1532                }
1533                stop(
1534                    state,
1535                    &repo,
1536                    &pr,
1537                    &format!("`gh pr merge` failed: {}", out.1),
1538                )
1539                .await?;
1540                return Ok(pr);
1541            }
1542            Step::Rebase => {
1543                // Bounded by the same budget as a fix, because a rebase that
1544                // keeps being needed means the base moves faster than this
1545                // run can land and a person should decide what to do. It
1546                // spends none of that budget: the change is not what is
1547                // wrong.
1548                if rebases >= budget {
1549                    let why = format!(
1550                        "the base moved under this branch {budget} time(s) and it still does \
1551                         not merge; rebasing again would only race it"
1552                    );
1553                    stop(state, &repo, &pr, &why).await?;
1554                    return Ok(pr);
1555                }
1556                rebases += 1;
1557                let Some(branch) = state.winner().map(|w| w.branch.clone()) else {
1558                    stop(
1559                        state,
1560                        &repo,
1561                        &pr,
1562                        "the pull request conflicts and this run has no winning branch to rebase",
1563                    )
1564                    .await?;
1565                    return Ok(pr);
1566                };
1567                let base = state.base_branch.clone();
1568                state.event(
1569                    "land",
1570                    format!("{} no longer merges; rebasing onto {base}", pr.url),
1571                );
1572                state.save()?;
1573
1574                // Onto the base as the *remote* has it: the local ref may be
1575                // behind, and rebasing onto a stale base produces a branch
1576                // that conflicts all over again.
1577                git::fetch(&repo, "origin", &base).await.ok();
1578                let scratch = state.dir().join("rebase");
1579                let onto = format!("origin/{base}");
1580                match git::rebase_branch_in_temp(&repo, &scratch, &branch, &onto).await {
1581                    Ok(None) => {
1582                        let pushed = {
1583                            let merge_lock = repo_merge_lock(&repo);
1584                            let _merge_slot = merge_lock.lock().await;
1585                            git::push_rewritten(&repo, "origin", &branch).await?
1586                        };
1587                        if !pushed.ok() {
1588                            let why = format!(
1589                                "rebased {branch} but could not push it: {}",
1590                                pushed.stderr.trim()
1591                            );
1592                            stop(state, &repo, &pr, &why).await?;
1593                            return Ok(pr);
1594                        }
1595                        state.event("land", format!("rebased {branch} onto {base}"));
1596                        state.save()?;
1597                        // The forge has to re-run its checks against the
1598                        // rebased head before anything else can be decided.
1599                        waited = Duration::ZERO;
1600                        tokio::time::sleep(POLL).await;
1601                    }
1602                    // A conflict is a decision, not a chore.
1603                    Ok(Some(conflict)) => {
1604                        let why = format!(
1605                            "{} conflicts with {base} and the rebase did not apply: {}",
1606                            pr.url,
1607                            conflict.chars().take(600).collect::<String>()
1608                        );
1609                        stop(state, &repo, &pr, &why).await?;
1610                        return Ok(pr);
1611                    }
1612                    Err(e) => {
1613                        let why = format!("could not rebase {branch} onto {base}: {e:#}");
1614                        stop(state, &repo, &pr, &why).await?;
1615                        return Ok(pr);
1616                    }
1617                }
1618            }
1619            Step::GiveUp { reason } => {
1620                stop(state, &repo, &pr, &reason).await?;
1621                return Ok(pr);
1622            }
1623            Step::Fix { reason } => {
1624                round += 1;
1625                waited = Duration::ZERO;
1626                for c in &pr.review_comments {
1627                    shown.insert(c.body.clone());
1628                }
1629                state.event("land", format!("round {round}: {reason}"));
1630                state.save()?;
1631
1632                let logs = failing_logs(&repo, &seen.failing_urls).await;
1633                let was_red = pr.checks == Checks::Red;
1634                match fix_round(state, &pr, round, budget, &reason, &logs).await? {
1635                    Fixed::Committed => {}
1636                    Fixed::Declined if was_red => {
1637                        let why = format!(
1638                            "the fixer produced no commit while {} check(s) were failing \
1639                             ({}); stopping instead of looping on an unchanged tree",
1640                            pr.failing.len(),
1641                            pr.failing.join(", ")
1642                        );
1643                        stop(state, &repo, &pr, &why).await?;
1644                        return Ok(pr);
1645                    }
1646                    // Comment-driven round with no commit: the fixer read the
1647                    // comments and changed nothing, which is a decision it is
1648                    // allowed to make. The comments are recorded as shown, so
1649                    // the next observation sees a clean pull request.
1650                    Fixed::Declined => state.event(
1651                        "land",
1652                        format!("round {round}: fixer declined the comments, nothing committed"),
1653                    ),
1654                    Fixed::Failed(why) => {
1655                        stop(state, &repo, &pr, &format!("the fix round failed: {why}")).await?;
1656                        return Ok(pr);
1657                    }
1658                }
1659                state.save()?;
1660            }
1661        }
1662    }
1663}
1664
1665/// One observation, plus the two things [`PrState`] deliberately does not carry:
1666/// the title (needed for the squash subject) and where the failing checks'
1667/// logs live.
1668struct Seen {
1669    pr: PrState,
1670    title: String,
1671    failing_urls: Vec<(String, String)>,
1672}
1673
1674/// Read the pull request: `gh pr view` for the rollup and the top-level thread,
1675/// `gh api` for the inline review comments `gh pr view` does not report.
1676async fn observe(repo: &Path, pr_url: &str) -> Result<Seen> {
1677    let view = gh(
1678        repo,
1679        &[
1680            "pr".to_owned(),
1681            "view".to_owned(),
1682            pr_url.to_owned(),
1683            "--json".to_owned(),
1684            "url,number,state,title,statusCheckRollup,reviews,comments,mergeStateStatus".to_owned(),
1685        ],
1686    )
1687    .await?;
1688    if !view.0 {
1689        bail!("gh pr view {pr_url}: {}", view.1);
1690    }
1691    let mut pr = parse_pr(&view.1)?;
1692    let raw: GhPr = serde_json::from_str(&view.1).context("re-read pull request json")?;
1693
1694    let inline = gh(
1695        repo,
1696        &[
1697            "api".to_owned(),
1698            format!("repos/{{owner}}/{{repo}}/pulls/{}/comments", pr.number),
1699        ],
1700    )
1701    .await?;
1702    if inline.0 {
1703        match parse_inline_comments(&inline.1) {
1704            Ok(mut comments) => pr.review_comments.append(&mut comments),
1705            // An unreadable inline thread must not end a landing: the rollup
1706            // and the top-level thread are still real signal.
1707            Err(e) => tracing::warn!("inline review comments unreadable: {e}"),
1708        }
1709    } else {
1710        tracing::warn!("gh api pulls/{}/comments: {}", pr.number, inline.1);
1711    }
1712
1713    let failing_urls = raw
1714        .status_check_rollup
1715        .iter()
1716        .filter(|c| c.verdict() == Verdict::Fail)
1717        .filter_map(|c| c.url().map(|u| (c.label(), u.to_owned())))
1718        .collect();
1719
1720    Ok(Seen {
1721        pr,
1722        title: raw.title,
1723        failing_urls,
1724    })
1725}
1726
1727/// What a fix round did.
1728enum Fixed {
1729    /// The fixer committed something.
1730    Committed,
1731    /// The fixer ran and chose to change nothing.
1732    Declined,
1733    /// The fixer could not run, or said nothing usable.
1734    Failed(String),
1735}
1736
1737/// Hand the failures and the comments to the fixer, then commit and push.
1738///
1739/// The fixer works in the winner's own worktree so its commits land on the
1740/// branch the pull request is built from, and it runs with `allow_write` for
1741/// the same reason.
1742async fn fix_round(
1743    state: &mut RunState,
1744    pr: &PrState,
1745    round: usize,
1746    budget: usize,
1747    reason: &str,
1748    logs: &str,
1749) -> Result<Fixed> {
1750    let winner = state
1751        .winner()
1752        .cloned()
1753        .context("landing needs a winning candidate; none is recorded on this run")?;
1754    let roles = state
1755        .config
1756        .resolve_roles()
1757        .context("resolve the roster for the fix round")?;
1758    // Same rule as the review loop: an explicitly configured fixer, otherwise
1759    // the winner's own author continuing its own conversation - the competition
1760    // is over, so its context is pure benefit.
1761    let (spec, seat_key): (AgentSpec, String) = match &roles.fixer {
1762        Some(f) if f.id != winner.agent => (f.clone(), "fix".to_owned()),
1763        _ => (
1764            state
1765                .config
1766                .agent(&winner.agent)
1767                .cloned()
1768                .unwrap_or_else(|_| roles.implementers[winner.index].clone()),
1769            format!("impl-{}", winner.label),
1770        ),
1771    };
1772
1773    let prompt = fix_prompt(state, pr, round, budget, reason, logs);
1774    let mut seat = seat_of(state, &seat_key, &spec.id);
1775    let artifacts = agent::artifacts_dir(&state.dir());
1776    let prompt = if state.config.cache_dir().is_some() {
1777        format!("{prompt}\n\n{}", prompt::build_cache_note())
1778    } else {
1779        prompt
1780    };
1781    let out = agent::invoke(
1782        &spec,
1783        &mut seat,
1784        &Invocation {
1785            cwd: &winner.worktree,
1786            prompt: &prompt,
1787            timeout: Duration::from_secs(state.config.graph.timeout_fix),
1788            allow_write: true,
1789            sessions: state.config.graph.sessions,
1790            artifacts: &artifacts,
1791            stem: &format!("land-{round}"),
1792            run: &state.id,
1793            node: "land",
1794            cache_dir: state.config.cache_dir().as_deref(),
1795        },
1796    )
1797    .await;
1798    state.seats.insert(seat.key.clone(), seat);
1799
1800    match out {
1801        Ok(o) if o.quota_exhausted() => {
1802            return Ok(Fixed::Failed(
1803                "rate limited (quota); the fixer could not run".to_owned(),
1804            ));
1805        }
1806        Ok(o) if !o.usable() => {
1807            return Ok(Fixed::Failed(format!(
1808                "the fixer produced nothing usable (exit {:?}, timed out: {})",
1809                o.exit_code, o.timed_out
1810            )));
1811        }
1812        Ok(_) => {}
1813        Err(e) => return Ok(Fixed::Failed(format!("{e:#}"))),
1814    }
1815
1816    let before = git::rev_parse(&winner.worktree, "HEAD").await?;
1817    // An agent that edited files but never committed would otherwise push
1818    // nothing and look like a refusal.
1819    git::commit_all(
1820        &winner.worktree,
1821        &format!("magi: land round {round} fixes (uncommitted work)"),
1822    )
1823    .await
1824    .ok();
1825    let after = git::rev_parse(&winner.worktree, "HEAD").await?;
1826    if after == before {
1827        return Ok(Fixed::Declined);
1828    }
1829
1830    let remote = state.config.merge.remote.clone();
1831    let push = git::push(&winner.worktree, &remote, &winner.branch).await?;
1832    if !push.ok() {
1833        return Ok(Fixed::Failed(format!(
1834            "pushing {} to {remote} failed: {}",
1835            winner.branch, push.stderr
1836        )));
1837    }
1838    state.event(
1839        "land",
1840        format!("round {round}: pushed a fix to {}", winner.branch),
1841    );
1842    Ok(Fixed::Committed)
1843}
1844
1845/// Fetch or create a seat, keeping its conversation across nodes.
1846fn seat_of(state: &mut RunState, key: &str, agent: &str) -> SeatState {
1847    if let Some(existing) = state.seats.get(key)
1848        && existing.agent == agent
1849    {
1850        return existing.clone();
1851    }
1852    let fresh = SeatState::new(key, agent, state.seed);
1853    state.seats.insert(key.to_owned(), fresh.clone());
1854    fresh
1855}
1856
1857/// What the fixer is told.
1858fn fix_prompt(
1859    state: &RunState,
1860    pr: &PrState,
1861    round: usize,
1862    budget: usize,
1863    reason: &str,
1864    logs: &str,
1865) -> String {
1866    let mut s = format!(
1867        "Your patch is open as a pull request and it is not landing. Land round \
1868         {round} of {budget}.\n\n\
1869         Pull request: {}\n\n\
1870         What is holding it: {reason}\n\n\
1871         # The task\n\n{}\n",
1872        pr.url, state.instruction
1873    );
1874
1875    if pr.failing.is_empty() {
1876        s.push_str("\n# Failing checks\n\n(none)\n");
1877    } else {
1878        let _ = write!(s, "\n# Failing checks\n\n- {}\n", pr.failing.join("\n- "));
1879        if logs.trim().is_empty() {
1880            s.push_str("\nNo log could be read; reproduce the failure locally.\n");
1881        } else {
1882            let _ = write!(s, "\n## Failing log tails\n\n{logs}\n");
1883        }
1884    }
1885
1886    if pr.review_comments.is_empty() {
1887        s.push_str("\n# Review comments\n\n(none)\n");
1888    } else {
1889        s.push_str("\n# Review comments\n");
1890        for c in &pr.review_comments {
1891            let where_ = match (&c.path, c.line) {
1892                (Some(p), Some(l)) => format!(" ({p}:{l})"),
1893                (Some(p), None) => format!(" ({p})"),
1894                _ => String::new(),
1895            };
1896            let _ = write!(s, "\n## {}{where_}\n\n{}\n", c.author, c.body.trim());
1897        }
1898    }
1899
1900    s.push_str(
1901        "\n# Rules\n\n\
1902         1. Fix the cause, never the symptom. Do not delete, skip, or weaken a \
1903            failing test; do not silence a lint with an allow attribute; do not \
1904            stretch a timeout to hide a race. If the check is right, the code is \
1905            wrong.\n\
1906         2. Change nothing the checks and the comments did not raise. A \
1907            drive-by refactor turns a one-line fix into a pull request that \
1908            needs reviewing again.\n\
1909         3. If a comment is wrong, say so with a checkable argument and change \
1910            nothing for it. A declined comment with a reason is a correct \
1911            outcome; a change made to appease a reviewer is not.\n\
1912         4. Commit in this worktree. magi pushes to the pull request's branch \
1913            for you; do not push, merge, or close anything yourself.\n\
1914         5. Never name yourself, your vendor, or your model, anywhere.\n\n\
1915         # Output\n\n\
1916         Say what you changed and why, and what you declined and why.",
1917    );
1918
1919    let language = &state.config.graph.language;
1920    if !(language.trim().is_empty() || language.eq_ignore_ascii_case("en")) {
1921        let _ = write!(s, "\n\nWrite all prose in {language}.");
1922    }
1923    if let Some(overlay) = state.config.prompts.overlay("fix") {
1924        let _ = write!(s, "\n\n{overlay}");
1925    }
1926    s
1927}
1928
1929/// Failing log tails, the way the operator collects them by hand:
1930/// `gh run view --log-failed`.
1931async fn failing_logs(repo: &Path, failing: &[(String, String)]) -> String {
1932    let mut out = String::new();
1933    for (name, url) in failing.iter().take(MAX_LOGS) {
1934        let args = match (job_of(url), run_of(url)) {
1935            (Some(job), _) => vec![
1936                "run".to_owned(),
1937                "view".to_owned(),
1938                "--log-failed".to_owned(),
1939                "--job".to_owned(),
1940                job,
1941            ],
1942            (None, Some(run)) => vec![
1943                "run".to_owned(),
1944                "view".to_owned(),
1945                run,
1946                "--log-failed".to_owned(),
1947            ],
1948            // Not a GitHub Actions check - an external status has no log here.
1949            (None, None) => continue,
1950        };
1951        let (ok, body) = match gh(repo, &args).await {
1952            Ok(v) => v,
1953            Err(e) => (false, format!("{e:#}")),
1954        };
1955        if !ok && body.trim().is_empty() {
1956            continue;
1957        }
1958        let _ = write!(out, "### {name}\n\n```\n{}\n```\n\n", tail(&body, LOG_TAIL));
1959    }
1960    out
1961}
1962
1963/// Job id out of a check's `detailsUrl`
1964/// (`https://github.com/o/r/actions/runs/<run>/job/<job>`).
1965fn job_of(details_url: &str) -> Option<String> {
1966    let after = details_url.split("/job/").nth(1)?;
1967    let id: String = after.chars().take_while(char::is_ascii_digit).collect();
1968    (!id.is_empty()).then_some(id)
1969}
1970
1971/// Workflow run id out of a check's `detailsUrl`.
1972fn run_of(details_url: &str) -> Option<String> {
1973    let after = details_url.split("/actions/runs/").nth(1)?;
1974    let id: String = after.chars().take_while(char::is_ascii_digit).collect();
1975    (!id.is_empty()).then_some(id)
1976}
1977
1978/// Leave the pull request open, say why on it, and mark the run blocked.
1979///
1980/// The comment is what makes an unattended stop actionable: the operator wakes
1981/// up to a pull request that explains itself rather than to a silent queue.
1982async fn stop(state: &mut RunState, repo: &Path, pr: &PrState, why: &str) -> Result<()> {
1983    let body = format!(
1984        "{MARKER}\nmagi stopped landing this pull request: {why}\n\n\
1985         The branch is untouched and the run is `{}`. Nothing was merged.",
1986        state.id
1987    );
1988    let posted = gh(
1989        repo,
1990        &[
1991            "pr".to_owned(),
1992            "comment".to_owned(),
1993            pr.number.to_string(),
1994            "--body".to_owned(),
1995            body,
1996        ],
1997    )
1998    .await;
1999    match posted {
2000        Ok((true, _)) => {}
2001        Ok((false, out)) => tracing::warn!("could not comment on {}: {out}", pr.url),
2002        Err(e) => tracing::warn!("could not comment on {}: {e:#}", pr.url),
2003    }
2004    state.status = RunStatus::Blocked;
2005    state.merge = Some(MergeOutcome {
2006        mode: MergeMode::Pr,
2007        ok: false,
2008        detail: why.to_owned(),
2009    });
2010    state.event("land", format!("stopped: {why}"));
2011    state.save()?;
2012    Ok(())
2013}
2014
2015/// Run `gh` in `repo`, returning success and the combined output.
2016///
2017/// Combined because `gh` reports a refused merge on stderr and the pull request
2018/// json on stdout, and both are evidence.
2019async fn gh(cwd: &Path, args: &[String]) -> Result<(bool, String)> {
2020    let out = tokio::process::Command::new("gh")
2021        .args(args)
2022        .current_dir(cwd)
2023        .quiet()
2024        .stdin(std::process::Stdio::null())
2025        .output()
2026        .await
2027        .with_context(|| format!("spawn gh {}", args.join(" ")))?;
2028    let mut body = String::from_utf8_lossy(&out.stdout).into_owned();
2029    let err = String::from_utf8_lossy(&out.stderr);
2030    if body.trim().is_empty() {
2031        body = err.into_owned();
2032    } else if !err.trim().is_empty() {
2033        body.push_str(&err);
2034    }
2035    Ok((out.status.success(), body.trim().to_owned()))
2036}
2037
2038/// Verdict of one entry in the status rollup.
2039#[derive(Debug, Clone, Copy, PartialEq, Eq)]
2040enum Verdict {
2041    Pass,
2042    Fail,
2043    Pending,
2044    Unknown,
2045}
2046
2047#[derive(Debug, Deserialize)]
2048#[serde(rename_all = "camelCase")]
2049struct GhPr {
2050    #[serde(default)]
2051    url: String,
2052    #[serde(default)]
2053    number: u64,
2054    #[serde(default)]
2055    state: String,
2056    #[serde(default)]
2057    title: String,
2058    #[serde(default)]
2059    status_check_rollup: Vec<GhCheck>,
2060    /// GitHub's own verdict on whether the pull request can be merged.
2061    ///
2062    /// Worth asking for because it is the only place the *required* check set
2063    /// is applied: the rollup lists every check equally, so a repository that
2064    /// deliberately does not require `coverage` still looks red here. See
2065    /// [`Blocking`].
2066    #[serde(default)]
2067    merge_state_status: String,
2068    #[serde(default)]
2069    reviews: Vec<GhReview>,
2070    #[serde(default)]
2071    comments: Vec<GhComment>,
2072}
2073
2074/// One rollup entry. `gh` mixes two GraphQL types in this array: a `CheckRun`
2075/// has `name`/`status`/`conclusion`, while a `StatusContext` - the old commit
2076/// status API, which is how CodeRabbit reports - has `context`/`state` and no
2077/// conclusion at all.
2078#[derive(Debug, Deserialize)]
2079#[serde(rename_all = "camelCase")]
2080struct GhCheck {
2081    #[serde(default)]
2082    name: Option<String>,
2083    #[serde(default)]
2084    context: Option<String>,
2085    #[serde(default)]
2086    status: Option<String>,
2087    #[serde(default)]
2088    conclusion: Option<String>,
2089    #[serde(default)]
2090    state: Option<String>,
2091    #[serde(default)]
2092    details_url: Option<String>,
2093    #[serde(default)]
2094    target_url: Option<String>,
2095}
2096
2097impl GhCheck {
2098    /// Name to show a human and hand to the fixer.
2099    fn label(&self) -> String {
2100        self.name
2101            .clone()
2102            .or_else(|| self.context.clone())
2103            .unwrap_or_else(|| "(unnamed check)".to_owned())
2104    }
2105
2106    /// Where this check's logs live, when it has any.
2107    fn url(&self) -> Option<&str> {
2108        self.details_url
2109            .as_deref()
2110            .or(self.target_url.as_deref())
2111            .filter(|u| !u.is_empty())
2112    }
2113
2114    /// Did it pass?
2115    ///
2116    /// `SKIPPED` and `NEUTRAL` count as passed: the Claude review workflow
2117    /// skips release and bot pull requests by design, and a skip that blocked
2118    /// landing would block exactly the pull requests that need no review.
2119    /// `CANCELLED` counts as failed - a cancelled check did not pass, and
2120    /// merging over one is merging over a check that never ran.
2121    fn verdict(&self) -> Verdict {
2122        if let Some(status) = self.status.as_deref() {
2123            if !status.eq_ignore_ascii_case("COMPLETED") {
2124                return Verdict::Pending;
2125            }
2126        }
2127        let outcome = self
2128            .conclusion
2129            .as_deref()
2130            .or(self.state.as_deref())
2131            .unwrap_or("");
2132        match outcome.to_ascii_uppercase().as_str() {
2133            "SUCCESS" | "SKIPPED" | "NEUTRAL" => Verdict::Pass,
2134            "FAILURE" | "ERROR" | "TIMED_OUT" | "CANCELLED" | "STARTUP_FAILURE"
2135            | "ACTION_REQUIRED" => Verdict::Fail,
2136            "PENDING" | "EXPECTED" | "QUEUED" | "IN_PROGRESS" | "WAITING" | "REQUESTED" => {
2137                Verdict::Pending
2138            }
2139            _ => Verdict::Unknown,
2140        }
2141    }
2142}
2143
2144#[derive(Debug, Deserialize)]
2145struct GhAuthor {
2146    #[serde(default)]
2147    login: String,
2148}
2149
2150#[derive(Debug, Deserialize)]
2151struct GhReview {
2152    #[serde(default)]
2153    author: GhAuthor,
2154    #[serde(default)]
2155    body: String,
2156}
2157
2158#[derive(Debug, Deserialize)]
2159struct GhComment {
2160    #[serde(default)]
2161    author: GhAuthor,
2162    #[serde(default)]
2163    body: String,
2164}
2165
2166#[derive(Debug, Deserialize)]
2167struct GhUser {
2168    #[serde(default)]
2169    login: String,
2170}
2171
2172#[derive(Debug, Deserialize)]
2173struct GhInline {
2174    #[serde(default)]
2175    user: GhUser,
2176    #[serde(default)]
2177    path: Option<String>,
2178    #[serde(default)]
2179    line: Option<u64>,
2180    #[serde(default)]
2181    body: String,
2182}
2183
2184impl Default for GhAuthor {
2185    fn default() -> Self {
2186        Self {
2187            login: "(unknown)".to_owned(),
2188        }
2189    }
2190}
2191
2192impl Default for GhUser {
2193    fn default() -> Self {
2194        Self {
2195            login: "(unknown)".to_owned(),
2196        }
2197    }
2198}
2199
2200#[cfg(test)]
2201mod tests {
2202    use super::*;
2203    use crate::run::{Candidate, ReviewRecord, ReviewRound, Tally};
2204
2205    /// Real `gh pr view` output for the open pull request #10 (Renovate's apm bump), trimmed to four checks and its one comment. Every check passed or was skipped by the review workflow, and the only comment is CodeRabbit's trigger notice.
2206    const GREEN_OPEN: &str = r####"{
2207  "url": "https://github.com/yukimemi/magi/pull/10",
2208  "number": 10,
2209  "state": "OPEN",
2210  "mergeStateStatus": "CLEAN",
2211  "statusCheckRollup": [
2212    {
2213      "__typename": "CheckRun",
2214      "conclusion": "SKIPPED",
2215      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33356278334/job/99378963755",
2216      "name": "review",
2217      "status": "COMPLETED",
2218      "workflowName": "claude-review"
2219    },
2220    {
2221      "__typename": "CheckRun",
2222      "conclusion": "SUCCESS",
2223      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33356278338/job/99378963144",
2224      "name": "check (ubuntu-latest)",
2225      "status": "COMPLETED",
2226      "workflowName": "CI"
2227    },
2228    {
2229      "__typename": "CheckRun",
2230      "conclusion": "SUCCESS",
2231      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33356278338/job/99378963095",
2232      "name": "rustfmt",
2233      "status": "COMPLETED",
2234      "workflowName": "CI"
2235    },
2236    {
2237      "__typename": "StatusContext",
2238      "context": "CodeRabbit",
2239      "state": "SUCCESS",
2240      "targetUrl": ""
2241    }
2242  ],
2243  "reviews": [],
2244  "comments": [
2245    {
2246      "author": {
2247        "login": "coderabbitai"
2248      },
2249      "authorAssociation": "NONE",
2250      "body": "<!-- This is an auto-generated comment: summarize by coderabbit.ai -->\n<!-- This is an auto-generated comment: skip review by coderabbit.ai -->\n\n> [!IMPORTANT]\n> - [ ] <!-- {\"checkboxId\":\"e9bb8d72-00e8-4f67-9cb2-caf3b22574fe\"} --> 🔍 Trigger review\n> \n> This repository does not receive automatic reviews because it has fewer than 10 stars.\n> \n> <details>\n> <summary>⚙️ Run configuration</summary>\n> \n> **Configuration used**: defaults\n> \n> **Review profile**: CHILL\n> \n> **Plan**: Pro Plus\n> \n> **Run ID**: `78e70bf3-c5a0-4269-a96c-2afb2dba7eff`\n> \n> </details>\n\n<!-- end of auto-generated comment: skip review by coderabbit.ai -->\n\n<!-- tips_start -->\n\n---\n\nThanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=yukimemi/magi&utm_content=10)! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.\n\n<details>\n<summary>❤️ Share</summary>\n\n- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%2"
2251    }
2252  ]
2253}"####;
2254
2255    /// Real output for the open pull request #9 (the daily kata-apply), whose `editorconfig` check failed while everything else passed.
2256    const RED_OPEN: &str = r####"{
2257  "url": "https://github.com/yukimemi/magi/pull/9",
2258  "number": 9,
2259  "state": "OPEN",
2260  "mergeStateStatus": "UNSTABLE",
2261  "statusCheckRollup": [
2262    {
2263      "__typename": "CheckRun",
2264      "conclusion": "SUCCESS",
2265      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323744",
2266      "name": "check (ubuntu-latest)",
2267      "status": "COMPLETED",
2268      "workflowName": "CI"
2269    },
2270    {
2271      "__typename": "CheckRun",
2272      "conclusion": "SUCCESS",
2273      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323811",
2274      "name": "rustfmt",
2275      "status": "COMPLETED",
2276      "workflowName": "CI"
2277    },
2278    {
2279      "__typename": "CheckRun",
2280      "conclusion": "FAILURE",
2281      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323572",
2282      "name": "editorconfig",
2283      "status": "COMPLETED",
2284      "workflowName": "CI"
2285    },
2286    {
2287      "__typename": "StatusContext",
2288      "context": "CodeRabbit",
2289      "state": "SUCCESS",
2290      "targetUrl": ""
2291    }
2292  ],
2293  "reviews": [],
2294  "comments": [
2295    {
2296      "author": {
2297        "login": "coderabbitai"
2298      },
2299      "authorAssociation": "NONE",
2300      "body": "<!-- This is an auto-generated comment: summarize by coderabbit.ai -->\n<!-- This is an auto-generated comment: skip review by coderabbit.ai -->\n\n> [!IMPORTANT]\n> - [ ] <!-- {\"checkboxId\":\"e9bb8d72-00e8-4f67-9cb2-caf3b22574fe\"} --> 🔍 Trigger review\n> \n> This repository does not receive automatic reviews because it has fewer than 10 stars.\n> \n> <details>\n> <summary>⚙️ Run configuration</summary>\n> \n> **Configuration used**: defaults\n> \n> **Review profile**: CHILL\n> \n> **Plan**: Team\n> \n> **Run ID**: `91e0dc24-6040-4c3d-92c6-f7d2b542523d`\n> \n> </details>\n\n<!-- end of auto-generated comment: skip review by coderabbit.ai -->\n\n<!-- tips_start -->\n\n---\n\nThanks for using [CodeRabbit](https://coderab"
2301    }
2302  ]
2303}"####;
2304
2305    /// Pull request #9's real payload with its `editorconfig` check rewound to the `IN_PROGRESS` / `conclusion: null` pair `gh` reports while a job is still in flight.
2306    const PENDING_OPEN: &str = r####"{
2307  "url": "https://github.com/yukimemi/magi/pull/9",
2308  "number": 9,
2309  "state": "OPEN",
2310  "statusCheckRollup": [
2311    {
2312      "__typename": "CheckRun",
2313      "conclusion": "SUCCESS",
2314      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323744",
2315      "name": "check (ubuntu-latest)",
2316      "status": "COMPLETED",
2317      "workflowName": "CI"
2318    },
2319    {
2320      "__typename": "CheckRun",
2321      "conclusion": "SUCCESS",
2322      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323811",
2323      "name": "rustfmt",
2324      "status": "COMPLETED",
2325      "workflowName": "CI"
2326    },
2327    {
2328      "__typename": "CheckRun",
2329      "conclusion": null,
2330      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323572",
2331      "name": "editorconfig",
2332      "status": "IN_PROGRESS",
2333      "workflowName": "CI"
2334    },
2335    {
2336      "__typename": "StatusContext",
2337      "context": "CodeRabbit",
2338      "state": "SUCCESS",
2339      "targetUrl": ""
2340    }
2341  ],
2342  "reviews": [],
2343  "comments": []
2344}"####;
2345
2346    /// Real output for pull request #16 after it was merged - the shape landing sees when a person merged underneath it.
2347    const MERGED: &str = r####"{
2348  "url": "https://github.com/yukimemi/magi/pull/16",
2349  "number": 16,
2350  "state": "MERGED",
2351  "statusCheckRollup": [
2352    {
2353      "__typename": "CheckRun",
2354      "conclusion": "SUCCESS",
2355      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33636587933/job/100268878095",
2356      "name": "check (ubuntu-latest)",
2357      "status": "COMPLETED",
2358      "workflowName": "CI"
2359    },
2360    {
2361      "__typename": "CheckRun",
2362      "conclusion": "SUCCESS",
2363      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33636587918/job/100268876427",
2364      "name": "review",
2365      "status": "COMPLETED",
2366      "workflowName": "claude-review"
2367    }
2368  ],
2369  "reviews": [],
2370  "comments": []
2371}"####;
2372
2373    /// Pull request #12's real payload - a green pull request carrying CodeRabbit's walkthrough and a Claude review that found a real bug - rewound to the `OPEN` state it was in when that review was posted.
2374    const REVIEWED_OPEN: &str = r####"{
2375  "url": "https://github.com/yukimemi/magi/pull/12",
2376  "number": 12,
2377  "state": "OPEN",
2378  "statusCheckRollup": [
2379    {
2380      "__typename": "CheckRun",
2381      "conclusion": "SUCCESS",
2382      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33571212506/job/100065355258",
2383      "name": "check (ubuntu-latest)",
2384      "status": "COMPLETED",
2385      "workflowName": "CI"
2386    },
2387    {
2388      "__typename": "CheckRun",
2389      "conclusion": "SUCCESS",
2390      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33571212566/job/100065355810",
2391      "name": "review",
2392      "status": "COMPLETED",
2393      "workflowName": "claude-review"
2394    }
2395  ],
2396  "reviews": [
2397    {
2398      "author": {
2399        "login": "claude"
2400      },
2401      "state": "COMMENTED",
2402      "body": ""
2403    }
2404  ],
2405  "comments": [
2406    {
2407      "author": {
2408        "login": "coderabbitai"
2409      },
2410      "authorAssociation": "NONE",
2411      "body": "<!-- This is an auto-generated comment: summarize by coderabbit.ai -->\n<!-- This is an auto-generated comment: skip review by coderabbit.ai -->\n\n> [!IMPORTANT]\n> - [ ] <!-- {\"checkboxId\":\"e9bb8d72-00e8-4f67-9cb2-caf3b22574fe\"} --> 🔍 Trigger review\n> \n> This repository does not receive automatic reviews because it has fewer than 10 stars.\n> \n> <details>\n> <summary>⚙️ Run configuration</summary>\n> \n> **Configuration used**: defaults\n> \n> **Review profile**: CHILL\n> \n> **Plan**: Team\n> \n> **Run ID**: `72058bf3-b7df-41d9-8e4d-a06a31be4a26`\n> \n> </details>\n\n<!-- end of auto-generated comment: skip review by coderabbit.ai -->\n\n<!-- tips_start -->\n\n---\n\nThanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=yukimemi/magi&utm_content=12)! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.\n\n<details>\n<summa"
2412    },
2413    {
2414      "author": {
2415        "login": "claude"
2416      },
2417      "authorAssociation": "NONE",
2418      "body": "**Claude finished @yukimemi's task in 3m 52s** —— [View job](https://github.com/yukimemi/magi/actions/runs/33571212566)\n\n---\n### Review: `magi review <branch>` — cheap-half-only graph\n\nRead through `src/graph.rs`, `src/main.rs`, `src/prompt.rs`, and the new/edited tests, and traced the claimed degeneration (`prep` short-circuits on a non-empty candidate list, `implement` skips because `commits != 0`, `judge`/`vote` skip on `viable().len() == 1`, `tally` skips because it's pre-set, `fold_losers` has no losers) against the actual code — it holds up. CI (`cargo make check`) is green on this PR.\n\n**Correctness**\n\n- One real bug, flagged inline on `src/graph.rs:1255`: the fixer-agent fallback (`self.roles.implementers[winner.index].clone()`) is unreachable in the normal graph (a real candidate's `winner.agent` always resolves via `config.agent(...)`), but a review-only run's `winner.agent` is always the `\"(existing branch)\"` sentinel, so this fallback now runs on *every* review-only fix that has no dedicated `[roles] fixer`. `graph.candidates` has no lower-bound validation, so a `magi.toml` tuned for review-only use (`candidates = 0`, plausible given this PR's own cost rationale) would panic with an out-of-bounds index the first time a"
2419    }
2420  ]
2421}"####;
2422
2423    /// Real `gh api repos/{owner}/{repo}/pulls/12/comments` output: one inline finding with its file and line.
2424    const INLINE: &str = r####"[
2425  {
2426    "user": {
2427      "login": "claude[bot]"
2428    },
2429    "path": "src/graph.rs",
2430    "line": 231,
2431    "body": "Minor edge case: unlike `implement()` (which sets `c.empty = commits == 0 || patch.trim().is_empty()`, `src/graph.rs:472`), the seeded review-only candidate always sets `empty: false` once `commits > 0` is confirmed, without checking whether the diff itself is actually empty (e.g. a commit immediately followed by a revert nets zero file changes). Such a branch would pass `Runner::review`'s validation and proceed into a review round with an empty patch, where `implement()`'s equivalent path would"
2432  }
2433]"####;
2434
2435    /// CodeRabbit's real trigger notice: a checkbox, a `<details>` block, and its own "skip review" marker.
2436    const CODERABBIT_TRIGGER: &str = r####"<!-- This is an auto-generated comment: summarize by coderabbit.ai -->
2437<!-- This is an auto-generated comment: skip review by coderabbit.ai -->
2438
2439> [!IMPORTANT]
2440> - [ ] <!-- {"checkboxId":"e9bb8d72-00e8-4f67-9cb2-caf3b22574fe"} --> 🔍 Trigger review
2441> 
2442> This repository does not receive automatic reviews because it has fewer than 10 stars.
2443> 
2444> <details>
2445> <summary>⚙️ Run configuration</summary>
2446> 
2447> **Configuration used**: defaults
2448> 
2449> **Review profile**: CHILL
2450> 
2451> **Plan**: Team
2452> 
2453> **Run ID**: `c1e2a68f-87fc-4b35-9ec4-e75c7854966a`
2454> 
2455> </details>
2456
2457<!-- end of auto-generated comment: skip review by coderabbit.ai -->
2458
2459<!-- tips_start -->
2460
2461---
2462
2463Thanks for using [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=yukimemi/magi&utm_content=16)! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
2464
2465<details>
2466<summary>❤️ Share</summary>
2467
2468- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20off"####;
2469
2470    /// The Claude review job's real comment while it is still working: a heading and a task list, and nothing that asks for a change.
2471    const CLAUDE_CHECKLIST: &str = r####"**Claude finished @yukimemi's task in 4m 14s** —— [View job](https://github.com/yukimemi/magi/actions/runs/33636587918)
2472
2473---
2474### Reviewing PR #16
2475
2476- [x] Read AGENTS.md conventions
2477- [x] Review `src/daemon.rs` changes
2478- [x] Review `src/main.rs` changes (new `doctor` reporting)
2479- [x] Review `src/web.rs` changes (reuse of unreadable-run count)
2480- [x] Check test coverage for new behavior
2481- [x] Run verification commands (blocked — see note)
2482- [x] Post findings"####;
2483
2484    /// The same job's real comment on pull request #12 once it had something to say.
2485    const CLAUDE_FINDING: &str = r####"**Claude finished @yukimemi's task in 3m 52s** —— [View job](https://github.com/yukimemi/magi/actions/runs/33571212566)
2486
2487---
2488### Review: `magi review <branch>` — cheap-half-only graph
2489
2490Read through `src/graph.rs`, `src/main.rs`, `src/prompt.rs`, and the new/edited tests, and traced the claimed degeneration (`prep` short-circuits on a non-empty candidate list, `implement` skips because `commits != 0`, `judge`/`vote` skip on `viable().len() == 1`, `tally` skips because it's pre-set, `fold_losers` has no losers) against the actual code — it holds up. CI (`cargo make check`) is green on this PR.
2491
2492**Correctness**
2493
2494- One real bug, flagged inline on `src/graph.rs:1255`: the fixer-agent fallback (`self.roles.implementers[winner.index].clone()`) is unreachable in the normal graph (a real candidate's `winner.agent` always resolves via `config.agent(...)`), but a review-only run's `winner.agent` is always the `"(existing branch)"` sentinel, so this fallback now runs on *every* review-only fix that has no dedicated `[roles] fixer`. `graph.candidates` has no lower-bound validation, so a `magi.toml` tuned for review-only use (`candidates = 0`, plausible given this PR's own cost rationale) would panic with an out-of-bounds index the first time a"####;
2495
2496    fn pr(checks: Checks, failing: &[&str], comments: usize) -> PrState {
2497        PrState {
2498            url: "https://github.com/yukimemi/magi/pull/16".to_owned(),
2499            number: 16,
2500            state: PrLifecycle::Open,
2501            checks,
2502            // These tests are about red-means-fix, so a red here is one the
2503            // forge gates on. Without saying so they would assert the new
2504            // "merge past a check nobody requires" path by accident.
2505            blocking: if matches!(checks, Checks::Red) {
2506                Blocking::Yes
2507            } else {
2508                Blocking::No
2509            },
2510            failing: failing.iter().map(|s| (*s).to_owned()).collect(),
2511            review_comments: (0..comments)
2512                .map(|i| ReviewComment {
2513                    author: "coderabbitai".to_owned(),
2514                    path: Some("src/graph.rs".to_owned()),
2515                    line: Some(231),
2516                    body: format!("finding {i}"),
2517                })
2518                .collect(),
2519        }
2520    }
2521
2522    #[test]
2523    fn a_green_pull_request_with_nothing_outstanding_parses_as_ready_to_merge() {
2524        let state = parse_pr(GREEN_OPEN).expect("green fixture parses");
2525        assert_eq!(state.number, 10);
2526        assert_eq!(state.state, PrLifecycle::Open);
2527        assert_eq!(state.checks, Checks::Green);
2528        assert!(state.failing.is_empty());
2529        assert!(
2530            state.review_comments.is_empty(),
2531            "the only comment is CodeRabbit's trigger notice: {:?}",
2532            state.review_comments
2533        );
2534        assert_eq!(decide(&state, 0, 4, Duration::ZERO), Step::Merge);
2535    }
2536
2537    #[test]
2538    fn a_failing_check_parses_as_red_and_is_named() {
2539        let state = parse_pr(RED_OPEN).expect("red fixture parses");
2540        assert_eq!(state.checks, Checks::Red);
2541        assert_eq!(state.failing, vec!["editorconfig".to_owned()]);
2542        // The captured payload says `UNSTABLE` - mergeable, with a check
2543        // nobody requires red - which is exactly the shape that had to be
2544        // merged by hand. Asserted separately, in
2545        // `a_red_check_nobody_requires_does_not_buy_a_fix_round`. What this
2546        // test is about is that a red check is *named*, so the reason a fixer
2547        // is handed says which one; so it asks the blocking question here.
2548        let mut blocking = state.clone();
2549        blocking.blocking = Blocking::Yes;
2550        match decide(&blocking, 0, 4, Duration::ZERO) {
2551            Step::Fix { reason } => {
2552                assert!(reason.contains("editorconfig"), "reason: {reason}");
2553                assert!(reason.contains("failing"), "reason: {reason}");
2554            }
2555            other => panic!("expected a fix round, got {other:?}"),
2556        }
2557    }
2558
2559    #[test]
2560    fn a_check_still_running_parses_as_pending_and_is_waited_for() {
2561        let state = parse_pr(PENDING_OPEN).expect("pending fixture parses");
2562        assert_eq!(state.checks, Checks::Pending);
2563        assert_eq!(decide(&state, 0, 4, Duration::ZERO), Step::Wait);
2564    }
2565
2566    #[test]
2567    fn a_pull_request_merged_underneath_us_is_done_rather_than_a_failure() {
2568        let state = parse_pr(MERGED).expect("merged fixture parses");
2569        assert_eq!(state.state, PrLifecycle::Merged);
2570        assert_eq!(
2571            decide(&state, 0, 4, Duration::ZERO),
2572            Step::Done { merged: true }
2573        );
2574    }
2575
2576    #[test]
2577    fn a_review_that_found_something_is_outstanding_and_holds_the_merge() {
2578        let state = parse_pr(REVIEWED_OPEN).expect("reviewed fixture parses");
2579        assert_eq!(state.checks, Checks::Green);
2580        let authors: Vec<&str> = state
2581            .review_comments
2582            .iter()
2583            .map(|c| c.author.as_str())
2584            .collect();
2585        assert_eq!(
2586            authors,
2587            vec!["claude"],
2588            "CodeRabbit's walkthrough is machinery; Claude's review is a finding"
2589        );
2590        match decide(&state, 0, 4, Duration::ZERO) {
2591            Step::Fix { reason } => assert!(reason.contains("unresolved"), "reason: {reason}"),
2592            other => panic!("expected a fix round, got {other:?}"),
2593        }
2594    }
2595
2596    #[test]
2597    fn inline_review_comments_keep_their_file_and_line() {
2598        let comments = parse_inline_comments(INLINE).expect("inline fixture parses");
2599        assert_eq!(comments.len(), 1);
2600        assert_eq!(comments[0].author, "claude[bot]");
2601        assert_eq!(comments[0].path.as_deref(), Some("src/graph.rs"));
2602        assert_eq!(comments[0].line, Some(231));
2603        assert!(comments[0].body.contains("empty"), "{}", comments[0].body);
2604    }
2605
2606    #[test]
2607    fn a_status_only_bot_comment_does_not_trigger_a_fix_round() {
2608        assert!(
2609            is_noise(CODERABBIT_TRIGGER),
2610            "CodeRabbit's trigger notice declares itself not a review"
2611        );
2612        assert!(
2613            is_noise(CLAUDE_CHECKLIST),
2614            "a progress checklist asks for nothing"
2615        );
2616        assert!(
2617            !is_noise(CLAUDE_FINDING),
2618            "a review that names a bug is input, not noise"
2619        );
2620
2621        let mut clean = pr(Checks::Green, &[], 0);
2622        clean.review_comments.push(ReviewComment {
2623            author: "coderabbitai".to_owned(),
2624            path: None,
2625            line: None,
2626            body: CODERABBIT_TRIGGER.to_owned(),
2627        });
2628        clean.review_comments.retain(|c| !is_noise(&c.body));
2629        assert_eq!(decide(&clean, 0, 4, Duration::ZERO), Step::Merge);
2630
2631        let mut found = pr(Checks::Green, &[], 0);
2632        found.review_comments.push(ReviewComment {
2633            author: "claude".to_owned(),
2634            path: None,
2635            line: None,
2636            body: CLAUDE_FINDING.to_owned(),
2637        });
2638        found.review_comments.retain(|c| !is_noise(&c.body));
2639        assert!(matches!(
2640            decide(&found, 0, 4, Duration::ZERO),
2641            Step::Fix { .. }
2642        ));
2643    }
2644
2645    #[test]
2646    fn the_policy_table_holds_for_every_combination_that_matters() {
2647        let cases: Vec<(&str, PrState, usize, usize, Duration, Step)> = vec![
2648            (
2649                "pending checks are waited for, even on the last round",
2650                pr(Checks::Pending, &[], 0),
2651                4,
2652                4,
2653                Duration::ZERO,
2654                Step::Wait,
2655            ),
2656            (
2657                "red checks are fixed",
2658                pr(Checks::Red, &["editorconfig"], 0),
2659                0,
2660                4,
2661                Duration::ZERO,
2662                Step::Fix {
2663                    reason: "1 check(s) failing: editorconfig".to_owned(),
2664                },
2665            ),
2666            (
2667                "green with comments is fixed, not merged",
2668                pr(Checks::Green, &[], 2),
2669                1,
2670                4,
2671                Duration::ZERO,
2672                Step::Fix {
2673                    reason: "checks are green but 2 review comment(s) are unresolved: coderabbitai"
2674                        .to_owned(),
2675                },
2676            ),
2677            (
2678                "green and clean merges",
2679                pr(Checks::Green, &[], 0),
2680                3,
2681                4,
2682                Duration::ZERO,
2683                Step::Merge,
2684            ),
2685            (
2686                "an unreadable rollup is waited on while the grace lasts",
2687                pr(Checks::Unknown, &[], 0),
2688                0,
2689                4,
2690                Duration::ZERO,
2691                Step::Wait,
2692            ),
2693            (
2694                "an unreadable rollup is never merged once the grace is spent",
2695                pr(Checks::Unknown, &[], 0),
2696                0,
2697                4,
2698                CHECKS_GRACE,
2699                Step::GiveUp {
2700                    reason: "no check status is readable on the pull request after 3 minute(s); \
2701                             refusing to merge on a guess"
2702                        .to_owned(),
2703                },
2704            ),
2705        ];
2706        for (what, state, round, budget, waited, want) in cases {
2707            assert_eq!(decide(&state, round, budget, waited), want, "{what}");
2708        }
2709    }
2710
2711    #[test]
2712    fn the_forge_verdict_survives_the_round_trip_from_gh() {
2713        // Read off `gh pr view --json ...,mergeStateStatus`, because a field
2714        // requested but never parsed is the kind of thing that looks wired up
2715        // and answers `Unsaid` forever.
2716        let green = parse_pr(GREEN_OPEN).expect("parse");
2717        assert_eq!(green.blocking, Blocking::No);
2718        let red = parse_pr(RED_OPEN).expect("parse");
2719        assert_eq!(
2720            red.blocking,
2721            Blocking::No,
2722            "`UNSTABLE` is mergeable: the red check is one nobody requires"
2723        );
2724        assert_eq!(red.checks, Checks::Red, "and it is still reported as red");
2725        // A payload from an older `gh` has no such field at all.
2726        let quiet =
2727            parse_pr(&GREEN_OPEN.replace("\"mergeStateStatus\": \"CLEAN\",", "")).expect("parse");
2728        assert_eq!(quiet.blocking, Blocking::Unsaid);
2729    }
2730
2731    #[test]
2732    fn a_red_check_nobody_requires_does_not_buy_a_fix_round() {
2733        // Pull request 37's only red check was `editorconfig`, failing
2734        // because the action could not fetch its own binary after
2735        // editorconfig-checker v4 renamed its release assets. The repository
2736        // does not require it. magi answered by asking a fixer to repair a
2737        // change that was fine, and the pull request had to be merged by hand.
2738        let mut nonblocking = pr(Checks::Red, &["editorconfig", "coverage"], 0);
2739        nonblocking.blocking = Blocking::No;
2740        assert_eq!(
2741            decide(&nonblocking, 0, 4, Duration::ZERO),
2742            Step::Merge,
2743            "the forge says nothing is in the way, so nothing is"
2744        );
2745
2746        // The same red, gated on: that is a fix round, as before.
2747        let mut blocking = pr(Checks::Red, &["test (ubuntu-latest)"], 0);
2748        blocking.blocking = Blocking::Yes;
2749        assert!(matches!(
2750            decide(&blocking, 0, 4, Duration::ZERO),
2751            Step::Fix { .. }
2752        ));
2753
2754        // A review comment still outranks green-enough: a non-required red
2755        // must not become a way to merge past an unanswered reviewer.
2756        let mut commented = pr(Checks::Red, &["coverage"], 1);
2757        commented.blocking = Blocking::No;
2758        assert!(matches!(
2759            decide(&commented, 0, 4, Duration::ZERO),
2760            Step::Fix { .. }
2761        ));
2762
2763        // And silence from the forge is not consent.
2764        let mut unsaid = pr(Checks::Red, &["coverage"], 0);
2765        unsaid.blocking = Blocking::Unsaid;
2766        assert!(matches!(
2767            decide(&unsaid, 0, 4, Duration::ZERO),
2768            Step::Fix { .. }
2769        ));
2770    }
2771
2772    #[test]
2773    fn a_branch_the_base_moved_under_is_rebased_not_fixed() {
2774        // Pull requests 35 and 37 were both rebased by hand: a competition
2775        // that runs for two hours against a repository merging pull requests
2776        // all day conflicts on the way in, and that is arithmetic rather
2777        // than a defect in the change.
2778        let mut conflicted = pr(Checks::Green, &[], 0);
2779        conflicted.blocking = Blocking::Conflict;
2780        assert_eq!(decide(&conflicted, 0, 4, Duration::ZERO), Step::Rebase);
2781
2782        // Decided before the checks, and even with the rounds spent: every
2783        // check on a branch that cannot land is an answer about a state that
2784        // cannot land, and a conflict is not the change's fault.
2785        let mut red = pr(Checks::Red, &["test (ubuntu-latest)"], 2);
2786        red.blocking = Blocking::Conflict;
2787        assert_eq!(decide(&red, 4, 4, Duration::ZERO), Step::Rebase);
2788
2789        // The lifecycle still wins over everything, conflict included.
2790        let mut merged = pr(Checks::Red, &[], 0);
2791        merged.blocking = Blocking::Conflict;
2792        merged.state = PrLifecycle::Merged;
2793        assert_eq!(
2794            decide(&merged, 0, 4, Duration::ZERO),
2795            Step::Done { merged: true }
2796        );
2797    }
2798
2799    #[test]
2800    fn the_forge_verdict_is_read_off_merge_state_status() {
2801        // The spellings that mean "mergeable". `UNSTABLE` is the one that
2802        // matters: mergeable, with a non-required check red or still running.
2803        for ok in ["CLEAN", "UNSTABLE", "unstable", "HAS_HOOKS"] {
2804            assert_eq!(Blocking::of(ok), Blocking::No, "{ok}");
2805            assert!(!Blocking::of(ok).stops_a_merge(), "{ok}");
2806        }
2807        assert_eq!(Blocking::of("DIRTY"), Blocking::Conflict);
2808        assert_eq!(Blocking::of("BLOCKED"), Blocking::Yes);
2809        assert_eq!(Blocking::of("BEHIND"), Blocking::Yes);
2810        // An older `gh`, or a token without the scope, says nothing - and
2811        // refusing to guess is the rule everywhere else in this module.
2812        for quiet in ["", "UNKNOWN"] {
2813            assert_eq!(Blocking::of(quiet), Blocking::Unsaid);
2814            assert!(Blocking::of(quiet).stops_a_merge());
2815        }
2816    }
2817
2818    #[test]
2819    fn a_merge_command_that_failed_after_merging_is_still_a_merge() {
2820        let argv = merge_argv(28, "Merge magi run ec12 (candidate B)");
2821        // The exact stderr from run ec12, in a jj-colocated repository.
2822        let jj = "could not determine current branch: failed to run git: not on any branch";
2823
2824        let landed = merged_after_all(&argv, jj, Some(PrLifecycle::Merged))
2825            .expect("the forge says merged, so it merged");
2826        assert!(landed.ok);
2827        assert!(
2828            landed.detail.contains("but the pull request is merged"),
2829            "the record must not read as a clean success: {}",
2830            landed.detail
2831        );
2832        assert!(
2833            landed.detail.contains("not on any branch"),
2834            "and it must keep what the command actually said: {}",
2835            landed.detail
2836        );
2837
2838        // A pull request still open means the merge really failed.
2839        assert!(merged_after_all(&argv, jj, Some(PrLifecycle::Open)).is_none());
2840        assert!(merged_after_all(&argv, jj, Some(PrLifecycle::Closed)).is_none());
2841        // And an unreadable answer is not evidence of success.
2842        assert!(merged_after_all(&argv, jj, None).is_none());
2843    }
2844
2845    #[test]
2846    fn a_pull_request_closed_underneath_us_is_done_and_not_merged() {
2847        let mut state = pr(Checks::Red, &["editorconfig"], 3);
2848        state.state = PrLifecycle::Closed;
2849        assert_eq!(
2850            decide(&state, 0, 4, Duration::ZERO),
2851            Step::Done { merged: false },
2852            "a human closing the pull request ends the loop, whatever CI says"
2853        );
2854    }
2855
2856    #[test]
2857    fn the_last_round_gives_up_with_a_reason_naming_what_is_still_failing() {
2858        let red = decide(
2859            &pr(Checks::Red, &["editorconfig", "test (macos)"], 0),
2860            4,
2861            4,
2862            Duration::ZERO,
2863        );
2864        match red {
2865            Step::GiveUp { reason } => {
2866                assert!(reason.contains("editorconfig"), "reason: {reason}");
2867                assert!(reason.contains("test (macos)"), "reason: {reason}");
2868                assert!(reason.contains("4 fix round(s)"), "reason: {reason}");
2869            }
2870            other => panic!("expected a give-up, got {other:?}"),
2871        }
2872
2873        let commented = decide(&pr(Checks::Green, &[], 1), 2, 2, Duration::ZERO);
2874        match commented {
2875            Step::GiveUp { reason } => {
2876                assert!(reason.contains("unresolved"), "reason: {reason}");
2877                assert!(reason.contains("2 fix round(s)"), "reason: {reason}");
2878            }
2879            other => panic!("expected a give-up, got {other:?}"),
2880        }
2881    }
2882
2883    #[test]
2884    fn the_merge_command_squashes_deletes_the_branch_and_sets_its_own_subject() {
2885        let candidate_commit = "magi: candidate A (uncommitted work)";
2886        let subject = merge_subject(candidate_commit, "add retries to the uploader");
2887        let argv = merge_argv(16, &subject);
2888
2889        assert!(argv.contains(&"--squash".to_owned()));
2890        assert!(argv.contains(&"--delete-branch".to_owned()));
2891        assert!(argv.contains(&"--subject".to_owned()));
2892        assert_eq!(
2893            argv.last().map(String::as_str),
2894            Some("add retries to the uploader"),
2895            "the subject must not be the candidate commit message"
2896        );
2897        assert_ne!(subject, candidate_commit);
2898    }
2899
2900    #[test]
2901    fn a_real_pull_request_title_is_used_as_the_squash_subject_verbatim() {
2902        assert_eq!(
2903            merge_subject("feat: a queue, an unattended loop, and a phone UI", "task"),
2904            "feat: a queue, an unattended loop, and a phone UI"
2905        );
2906        assert_eq!(
2907            merge_subject("", "# port the retry logic\n\ndetails"),
2908            "port the retry logic",
2909            "an empty title falls back to the task's first line, heading marks stripped"
2910        );
2911    }
2912
2913    #[test]
2914    fn a_failing_checks_details_url_yields_the_job_to_read_logs_from() {
2915        let url = "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323572";
2916        assert_eq!(job_of(url).as_deref(), Some("100114323572"));
2917        assert_eq!(run_of(url).as_deref(), Some("33587406996"));
2918        assert_eq!(job_of("https://coderabbit.ai/status"), None);
2919        assert_eq!(run_of(""), None);
2920    }
2921
2922    #[test]
2923    fn magis_own_stop_comment_is_never_read_back_as_a_finding() {
2924        let mut out = Vec::new();
2925        push_if_outstanding(
2926            &mut out,
2927            ReviewComment {
2928                author: "yukimemi".to_owned(),
2929                path: None,
2930                line: None,
2931                body: format!("{MARKER}\nmagi stopped landing this pull request: 1 check failing"),
2932            },
2933        );
2934        assert!(out.is_empty());
2935    }
2936
2937    /// A run with no tally, so [`RunState::winner`] is `None` and the panel
2938    /// falls back to the repository - which keeps these tests free of a
2939    /// worktree, a `git` invocation and a network.
2940    fn run_state() -> RunState {
2941        RunState::new(
2942            std::path::PathBuf::from("/repo/magi"),
2943            "main".to_owned(),
2944            "abcdef1234".to_owned(),
2945            "add retries to the uploader".to_owned(),
2946            crate::config::Config::default(),
2947        )
2948    }
2949
2950    fn green_pr() -> PrState {
2951        PrState {
2952            url: "https://github.com/yukimemi/magi/pull/42".to_owned(),
2953            number: 42,
2954            state: PrLifecycle::Open,
2955            checks: Checks::Green,
2956            // The forge sees nothing in the way unless a test says otherwise.
2957            blocking: Blocking::No,
2958            failing: Vec::new(),
2959            review_comments: vec![ReviewComment {
2960                author: "coderabbitai".to_owned(),
2961                path: Some("src/land.rs".to_owned()),
2962                line: Some(212),
2963                body: "this branch never checks the exit code".to_owned(),
2964            }],
2965        }
2966    }
2967
2968    const NUMSTAT: &str = "12\t3\tsrc/land.rs\n40\t1\tsrc/web.rs\n-\t-\tassets/logo.png";
2969
2970    fn panel() -> String {
2971        approval_panel(
2972            &run_state(),
2973            &green_pr(),
2974            NUMSTAT,
2975            "diff --git a/src/land.rs b/src/land.rs\n@@ -1,2 +1,2 @@\n-old line\n+new line\n context",
2976            &[
2977                "land: ask before merging".to_owned(),
2978                "land: colour the diff".to_owned(),
2979            ],
2980            "feat: merge approval from the phone",
2981        )
2982    }
2983
2984    #[test]
2985    fn the_approval_panel_carries_the_whole_case_for_the_merge() {
2986        let html = panel();
2987        for needle in [
2988            "42",
2989            "main",
2990            "src/land.rs",
2991            "src/web.rs",
2992            "assets/logo.png",
2993            "feat: merge approval from the phone",
2994            "land: ask before merging",
2995            "land: colour the diff",
2996            "coderabbitai",
2997            "this branch never checks the exit code",
2998            "green",
2999        ] {
3000            assert!(html.contains(needle), "the panel must state `{needle}`");
3001        }
3002    }
3003
3004    /// A candidate whose label is `A` and has won, so [`RunState::winner`]
3005    /// resolves to it.
3006    fn winning_candidate(summary: &str) -> Candidate {
3007        Candidate {
3008            index: 0,
3009            label: 'A',
3010            agent: "opus".to_owned(),
3011            branch: "magi/x/A".to_owned(),
3012            worktree: PathBuf::from("/wt/A"),
3013            summary: summary.to_owned(),
3014            stat: String::new(),
3015            files: 1,
3016            commits: 1,
3017            empty: false,
3018            failed: None,
3019            duration_ms: 0,
3020            folded: false,
3021        }
3022    }
3023
3024    fn uncontested_tally() -> Tally {
3025        Tally {
3026            first_choice: BTreeMap::from([('A', 1)]),
3027            borda: BTreeMap::new(),
3028            winner: 'A',
3029            rankings: 1,
3030            unanimous_initial: true,
3031            deliberated: false,
3032            changed_votes: 0,
3033            unanimous_final: true,
3034            tie_break: None,
3035            judges: 1,
3036            present: 1,
3037            quorum: 1,
3038            met_quorum: true,
3039            uncontested: None,
3040        }
3041    }
3042
3043    fn review_record(reviewer: usize, agent: &str, summary: &str) -> ReviewRecord {
3044        ReviewRecord {
3045            reviewer,
3046            agent: agent.to_owned(),
3047            summary: summary.to_owned(),
3048            findings: Vec::new(),
3049            vote: None,
3050            failed: None,
3051            duration_ms: 0,
3052        }
3053    }
3054
3055    fn review_round(round: usize, reviews: Vec<ReviewRecord>) -> ReviewRound {
3056        let answered = reviews.len();
3057        ReviewRound {
3058            round,
3059            head: "abc1234".to_owned(),
3060            reviews,
3061            e2e: Vec::new(),
3062            verify_retried: false,
3063            fix: None,
3064            blocking: 0,
3065            answered,
3066            expected: answered,
3067            clean: true,
3068            progressed: false,
3069            vote_split: false,
3070            reconsideration: Vec::new(),
3071            verdict: None,
3072        }
3073    }
3074
3075    #[test]
3076    fn the_approval_panel_states_the_task_verbatim_in_either_language() {
3077        let en = panel();
3078        assert!(en.contains("Task"), "{en}");
3079        assert!(en.contains("add retries to the uploader"), "{en}");
3080
3081        let mut state = run_state();
3082        state.config.graph.language = "ja".to_owned();
3083        let ja = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3084        assert!(ja.contains("タスク"), "{ja}");
3085        assert!(
3086            ja.contains("add retries to the uploader"),
3087            "the task itself is not translated: {ja}"
3088        );
3089    }
3090
3091    #[test]
3092    fn the_approval_panel_omits_what_changed_and_review_verdict_with_no_data() {
3093        // `run_state()` has no candidates, no tally and no reviews - exactly
3094        // the shape a run has before anything has judged or reviewed it, and
3095        // the panel must not print an empty box for either.
3096        let html = panel();
3097        assert!(!html.contains("What changed"), "{html}");
3098        assert!(!html.contains("Review verdict"), "{html}");
3099    }
3100
3101    #[test]
3102    fn the_approval_panel_omits_what_changed_when_the_winners_summary_is_empty() {
3103        let mut state = run_state();
3104        state.candidates = vec![winning_candidate("")];
3105        state.tally = Some(uncontested_tally());
3106        let html = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3107        assert!(
3108            !html.contains("What changed"),
3109            "an empty summary must not render an empty box: {html}"
3110        );
3111    }
3112
3113    #[test]
3114    fn the_approval_panel_shows_the_winners_own_account_in_either_language() {
3115        let mut state = run_state();
3116        state.candidates = vec![winning_candidate(
3117            "Added a retry loop around the uploader PUT call.",
3118        )];
3119        state.tally = Some(uncontested_tally());
3120        let en = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3121        assert!(en.contains("What changed"), "{en}");
3122        assert!(
3123            en.contains("Added a retry loop around the uploader PUT call."),
3124            "{en}"
3125        );
3126
3127        state.config.graph.language = "ja".to_owned();
3128        let ja = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3129        assert!(ja.contains("変更内容"), "{ja}");
3130        assert!(
3131            ja.contains("Added a retry loop around the uploader PUT call."),
3132            "{ja}"
3133        );
3134    }
3135
3136    #[test]
3137    fn the_approval_panel_shows_only_the_last_review_rounds_verdict() {
3138        let mut state = run_state();
3139        state.reviews = vec![
3140            review_round(
3141                1,
3142                vec![review_record(1, "alpha", "found a race, sent back")],
3143            ),
3144            review_round(2, vec![review_record(1, "alpha", "race is fixed, clean")]),
3145        ];
3146        let en = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3147        assert!(en.contains("Review verdict"), "{en}");
3148        assert!(en.contains("race is fixed, clean"), "{en}");
3149        assert!(
3150            !en.contains("found a race, sent back"),
3151            "only the round that actually cleared the merge should show: {en}"
3152        );
3153
3154        state.config.graph.language = "ja".to_owned();
3155        let ja = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3156        assert!(ja.contains("レビューの結論"), "{ja}");
3157        assert!(ja.contains("レビュアー"), "{ja}");
3158        assert!(ja.contains("race is fixed, clean"), "{ja}");
3159    }
3160
3161    /// The `incomplete_review = "warn"` policy (see
3162    /// `graph::Runner::review_loop`) can push a `clean` round to
3163    /// `state.reviews` while one seat's own record still has `failed: Some`
3164    /// and an empty `summary` - a seat that never answered, not one that
3165    /// answered with nothing to say.
3166    fn unanswered_review_record(reviewer: usize, agent: &str, reason: &str) -> ReviewRecord {
3167        ReviewRecord {
3168            reviewer,
3169            agent: agent.to_owned(),
3170            summary: String::new(),
3171            findings: Vec::new(),
3172            vote: None,
3173            failed: Some(reason.to_owned()),
3174            duration_ms: 0,
3175        }
3176    }
3177
3178    #[test]
3179    fn the_approval_panel_never_shows_an_unanswered_seat_as_a_blank_verdict() {
3180        let mut state = run_state();
3181        state.reviews = vec![review_round(
3182            1,
3183            vec![
3184                review_record(1, "alpha", "clean, nothing to add"),
3185                unanswered_review_record(2, "beta", "timed out"),
3186            ],
3187        )];
3188        let en = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3189        assert!(en.contains("clean, nothing to add"), "{en}");
3190        assert!(
3191            en.contains("produced no answer: timed out"),
3192            "a seat that never answered must say so, not render a blank box: {en}"
3193        );
3194        assert!(
3195            !en.contains("<div style=\"white-space:pre-wrap;font-size:13px\"></div>"),
3196            "no reviewer box may be left empty: {en}"
3197        );
3198
3199        state.config.graph.language = "ja".to_owned();
3200        let ja = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3201        assert!(ja.contains("回答なし: timed out"), "{ja}");
3202    }
3203
3204    #[test]
3205    fn the_approval_panel_contains_nothing_the_frames_policy_would_block() {
3206        let html = panel();
3207        assert!(!html.contains("<script"), "no script survives the csp");
3208        assert!(!html.contains("<form"), "form-action is 'none'");
3209        let pr = green_pr();
3210        assert_eq!(
3211            html.matches("http").count(),
3212            html.matches(pr.url.as_str()).count(),
3213            "the only http url in the panel is the pull request's own link"
3214        );
3215    }
3216
3217    #[test]
3218    fn added_and_removed_diff_lines_are_distinguishable_without_colour() {
3219        let html = panel();
3220        assert!(
3221            html.contains(">+</span>"),
3222            "an added line carries a `+` in the gutter, not only a background"
3223        );
3224        assert!(
3225            html.contains(">-</span>"),
3226            "a removed line carries a `-` in the gutter, not only a background"
3227        );
3228        assert!(
3229            html.contains(">new line</span>"),
3230            "the marker is moved to the gutter, so the body is printed once without it"
3231        );
3232    }
3233
3234    #[test]
3235    fn a_diff_past_the_threshold_is_cut_with_an_honest_count() {
3236        let total = DIFF_MAX_LINES + 100;
3237        let diff: String = (0..total).map(|i| format!("+line {i}\n")).collect();
3238        let html = approval_panel(
3239            &run_state(),
3240            &green_pr(),
3241            NUMSTAT,
3242            &diff,
3243            &[],
3244            "feat: something long",
3245        );
3246        assert!(
3247            html.contains(&format!("100 of {total} diff lines omitted")),
3248            "the note must say exactly how much was cut"
3249        );
3250        assert!(html.contains(&format!("line {}", DIFF_MAX_LINES - 1)));
3251        assert!(
3252            !html.contains(&format!("line {DIFF_MAX_LINES}")),
3253            "nothing past the threshold is rendered"
3254        );
3255        assert!(
3256            html.contains("/repo/magi"),
3257            "the note says where the rest is"
3258        );
3259    }
3260
3261    #[test]
3262    fn a_path_with_html_metacharacters_is_escaped_rather_than_rendered() {
3263        let html = approval_panel(
3264            &run_state(),
3265            &green_pr(),
3266            "1\t2\tsrc/<b>&\"x\"'.rs",
3267            "",
3268            &[],
3269            "subject",
3270        );
3271        assert!(html.contains("src/&lt;b&gt;&amp;&quot;x&quot;&#39;.rs"));
3272        assert!(
3273            !html.contains("<b>"),
3274            "an agent-influenced path must never become markup"
3275        );
3276    }
3277
3278    #[tokio::test]
3279    async fn the_merge_lock_serialises_one_repository_but_never_a_different_one() {
3280        let a = std::path::PathBuf::from("/repo/a");
3281        let b = std::path::PathBuf::from("/repo/b");
3282
3283        let held = repo_merge_lock(&a).lock_owned().await;
3284
3285        // A second, concurrent land run against the *same* repository must
3286        // wait - `try_lock` fails while `held` is alive.
3287        assert!(
3288            repo_merge_lock(&a).try_lock().is_err(),
3289            "a second merge into the same repository must not proceed concurrently"
3290        );
3291
3292        // A run against a *different* repository must not be blocked by it -
3293        // this is what keeps a slow rebase or `gh pr merge` in one
3294        // repository from also stalling a land-approval resume in another.
3295        assert!(
3296            repo_merge_lock(&b).try_lock().is_ok(),
3297            "a different repository's merge lock must be independent"
3298        );
3299
3300        drop(held);
3301        assert!(
3302            repo_merge_lock(&a).try_lock().is_ok(),
3303            "the lock is released once the holder is done"
3304        );
3305    }
3306
3307    #[test]
3308    fn only_the_merge_choice_merges_and_silence_holds() {
3309        let table = [
3310            (None, Approval::Hold),
3311            (Some("merge"), Approval::Merge),
3312            (Some(" merge\n"), Approval::Merge),
3313            (Some("hold"), Approval::Hold),
3314            (Some(""), Approval::Hold),
3315            (Some("yes"), Approval::Hold),
3316        ];
3317        for (answer, want) in table {
3318            assert_eq!(
3319                approval(answer),
3320                want,
3321                "answer {answer:?} must resolve to {want:?}"
3322            );
3323        }
3324    }
3325
3326    #[tokio::test]
3327    async fn a_first_visit_to_the_merge_gate_files_a_question_and_returns_pending_at_once() {
3328        crate::run::set_home(std::env::temp_dir().join("magi-land-approval-test-home"));
3329        let mut state = run_state();
3330        state.config.graph.land_approval = true;
3331        let pr = green_pr();
3332
3333        let gate = approval_gate(&mut state, &pr, "feat: x").await.unwrap();
3334        assert_eq!(gate, ApprovalGate::Pending, "nobody has answered yet");
3335        assert!(
3336            !state.parked,
3337            "approval_gate itself never sets `parked`; only its caller does"
3338        );
3339
3340        let store = ask::Questions::open();
3341        let filed: Vec<_> = store
3342            .list()
3343            .into_iter()
3344            .filter(|q| q.run == state.id)
3345            .collect();
3346        assert_eq!(filed.len(), 1, "exactly one question is filed");
3347        assert_eq!(filed[0].node, APPROVAL_NODE);
3348        assert_eq!(filed[0].choices, vec![APPROVE.to_owned(), HOLD.to_owned()]);
3349        assert!(filed[0].status.open());
3350
3351        // A second visit - standing in for a resumed run whose slot the
3352        // daemon handed to something else while nobody had answered - must
3353        // find the same question rather than filing a second one.
3354        let again = approval_gate(&mut state, &pr, "feat: x").await.unwrap();
3355        assert_eq!(again, ApprovalGate::Pending);
3356        let still_one = store
3357            .list()
3358            .into_iter()
3359            .filter(|q| q.run == state.id)
3360            .count();
3361        assert_eq!(
3362            still_one, 1,
3363            "asking twice must not double-file the question"
3364        );
3365    }
3366
3367    #[tokio::test]
3368    async fn approving_the_existing_question_is_read_back_as_approved() {
3369        crate::run::set_home(std::env::temp_dir().join("magi-land-approval-test-home"));
3370        let mut state = run_state();
3371        state.config.graph.land_approval = true;
3372        let pr = green_pr();
3373        assert_eq!(
3374            approval_gate(&mut state, &pr, "feat: x").await.unwrap(),
3375            ApprovalGate::Pending
3376        );
3377
3378        let store = ask::Questions::open();
3379        let mut q = store
3380            .list()
3381            .into_iter()
3382            .find(|q| q.run == state.id)
3383            .expect("filed above");
3384        q.answer(ask::Answer::Choice(APPROVE.to_owned())).unwrap();
3385        store.put(&mut q).unwrap();
3386
3387        assert_eq!(
3388            approval_gate(&mut state, &pr, "feat: x").await.unwrap(),
3389            ApprovalGate::Approved
3390        );
3391    }
3392
3393    #[tokio::test]
3394    async fn holding_or_abandoning_the_existing_question_is_read_back_as_held() {
3395        crate::run::set_home(std::env::temp_dir().join("magi-land-approval-test-home"));
3396        let store = ask::Questions::open();
3397
3398        let mut held_state = run_state();
3399        held_state.config.graph.land_approval = true;
3400        let pr = green_pr();
3401        approval_gate(&mut held_state, &pr, "feat: x")
3402            .await
3403            .unwrap();
3404        let mut q = store
3405            .list()
3406            .into_iter()
3407            .find(|q| q.run == held_state.id)
3408            .expect("filed above");
3409        q.answer(ask::Answer::Choice(HOLD.to_owned())).unwrap();
3410        store.put(&mut q).unwrap();
3411        assert_eq!(
3412            approval_gate(&mut held_state, &pr, "feat: x")
3413                .await
3414                .unwrap(),
3415            ApprovalGate::Held
3416        );
3417
3418        let mut abandoned_state = run_state();
3419        abandoned_state.config.graph.land_approval = true;
3420        approval_gate(&mut abandoned_state, &pr, "feat: x")
3421            .await
3422            .unwrap();
3423        let mut q = store
3424            .list()
3425            .into_iter()
3426            .find(|q| q.run == abandoned_state.id)
3427            .expect("filed above");
3428        q.abandon("no answer within the timeout");
3429        store.put(&mut q).unwrap();
3430        assert_eq!(
3431            approval_gate(&mut abandoned_state, &pr, "feat: x")
3432                .await
3433                .unwrap(),
3434            ApprovalGate::Held,
3435            "silence must never merge"
3436        );
3437    }
3438
3439    #[test]
3440    fn the_diffstat_table_is_ordered_by_churn_with_binaries_last() {
3441        let rows = parse_numstat(NUMSTAT);
3442        assert_eq!(
3443            rows.iter().map(|r| r.path.as_str()).collect::<Vec<_>>(),
3444            ["src/web.rs", "src/land.rs", "assets/logo.png"]
3445        );
3446        assert_eq!(rows[2].added, None, "a binary file has no line counts");
3447    }
3448    #[test]
3449    fn the_approval_speaks_the_language_the_repository_is_configured_for() {
3450        // Reported from a real run: the merge question arrived in English on a
3451        // repository with `language = "ja"`. magi's own strings have to follow
3452        // that setting too - "it is a literal in Rust" is not an answer.
3453        let mut state = run_state();
3454        state.config.graph.language = "ja".to_owned();
3455        let pr = green_pr();
3456        let commits = ["c1".to_owned()];
3457
3458        let ja = approval_panel(&state, &pr, "3\t1\tsrc/a.rs", "+ x", &commits, "feat: x");
3459        assert!(ja.contains("lang=\"ja\""), "the document must declare it");
3460        assert!(ja.contains("squash されるコミット"), "{ja}");
3461        assert!(ja.contains("レビューコメント"), "{ja}");
3462        assert!(ja.contains("差分"), "{ja}");
3463        assert!(
3464            !ja.contains("Commits being squashed"),
3465            "no English left over"
3466        );
3467
3468        let w = words("ja");
3469        assert!(w.approval_summary(17, "feat: x").contains("マージ"));
3470        assert!(
3471            w.approval_detail("http://x/1", "main", "feat: x")
3472                .contains("パネル")
3473        );
3474
3475        // The evidence itself is language-neutral and must survive either way.
3476        assert!(ja.contains("src/a.rs"), "the diffstat is not prose");
3477        assert!(ja.contains("feat: x"), "nor is the merge subject");
3478
3479        // English stays the default, and a language magi cannot check falls
3480        // back to it rather than shipping a guess.
3481        state.config.graph.language = "en".to_owned();
3482        let en = approval_panel(&state, &pr, "3\t1\tsrc/a.rs", "+ x", &commits, "feat: x");
3483        assert!(en.contains("Commits being squashed"), "{en}");
3484        assert_eq!(words("Klingon").html_lang, "en");
3485    }
3486}