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("fix"))
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            attachments: &[],
1796        },
1797    )
1798    .await;
1799    state.seats.insert(seat.key.clone(), seat);
1800
1801    match out {
1802        Ok(o) if o.quota_exhausted() => {
1803            return Ok(Fixed::Failed(
1804                "rate limited (quota); the fixer could not run".to_owned(),
1805            ));
1806        }
1807        Ok(o) if !o.usable() => {
1808            return Ok(Fixed::Failed(format!(
1809                "the fixer produced nothing usable (exit {:?}, timed out: {})",
1810                o.exit_code, o.timed_out
1811            )));
1812        }
1813        Ok(_) => {}
1814        Err(e) => return Ok(Fixed::Failed(format!("{e:#}"))),
1815    }
1816
1817    let before = git::rev_parse(&winner.worktree, "HEAD").await?;
1818    // An agent that edited files but never committed would otherwise push
1819    // nothing and look like a refusal.
1820    git::commit_all(
1821        &winner.worktree,
1822        &format!("magi: land round {round} fixes (uncommitted work)"),
1823    )
1824    .await
1825    .ok();
1826    let after = git::rev_parse(&winner.worktree, "HEAD").await?;
1827    if after == before {
1828        return Ok(Fixed::Declined);
1829    }
1830
1831    let remote = state.config.merge.remote.clone();
1832    let push = git::push(&winner.worktree, &remote, &winner.branch).await?;
1833    if !push.ok() {
1834        return Ok(Fixed::Failed(format!(
1835            "pushing {} to {remote} failed: {}",
1836            winner.branch, push.stderr
1837        )));
1838    }
1839    state.event(
1840        "land",
1841        format!("round {round}: pushed a fix to {}", winner.branch),
1842    );
1843    Ok(Fixed::Committed)
1844}
1845
1846/// Fetch or create a seat, keeping its conversation across nodes.
1847fn seat_of(state: &mut RunState, key: &str, agent: &str) -> SeatState {
1848    if let Some(existing) = state.seats.get(key)
1849        && existing.agent == agent
1850    {
1851        return existing.clone();
1852    }
1853    let fresh = SeatState::new(key, agent, state.seed);
1854    state.seats.insert(key.to_owned(), fresh.clone());
1855    fresh
1856}
1857
1858/// What the fixer is told.
1859fn fix_prompt(
1860    state: &RunState,
1861    pr: &PrState,
1862    round: usize,
1863    budget: usize,
1864    reason: &str,
1865    logs: &str,
1866) -> String {
1867    let mut s = format!(
1868        "Your patch is open as a pull request and it is not landing. Land round \
1869         {round} of {budget}.\n\n\
1870         Pull request: {}\n\n\
1871         What is holding it: {reason}\n\n\
1872         # The task\n\n{}\n",
1873        pr.url, state.instruction
1874    );
1875
1876    if pr.failing.is_empty() {
1877        s.push_str("\n# Failing checks\n\n(none)\n");
1878    } else {
1879        let _ = write!(s, "\n# Failing checks\n\n- {}\n", pr.failing.join("\n- "));
1880        if logs.trim().is_empty() {
1881            s.push_str("\nNo log could be read; reproduce the failure locally.\n");
1882        } else {
1883            let _ = write!(s, "\n## Failing log tails\n\n{logs}\n");
1884        }
1885    }
1886
1887    if pr.review_comments.is_empty() {
1888        s.push_str("\n# Review comments\n\n(none)\n");
1889    } else {
1890        s.push_str("\n# Review comments\n");
1891        for c in &pr.review_comments {
1892            let where_ = match (&c.path, c.line) {
1893                (Some(p), Some(l)) => format!(" ({p}:{l})"),
1894                (Some(p), None) => format!(" ({p})"),
1895                _ => String::new(),
1896            };
1897            let _ = write!(s, "\n## {}{where_}\n\n{}\n", c.author, c.body.trim());
1898        }
1899    }
1900
1901    s.push_str(
1902        "\n# Rules\n\n\
1903         1. Fix the cause, never the symptom. Do not delete, skip, or weaken a \
1904            failing test; do not silence a lint with an allow attribute; do not \
1905            stretch a timeout to hide a race. If the check is right, the code is \
1906            wrong.\n\
1907         2. Change nothing the checks and the comments did not raise. A \
1908            drive-by refactor turns a one-line fix into a pull request that \
1909            needs reviewing again.\n\
1910         3. If a comment is wrong, say so with a checkable argument and change \
1911            nothing for it. A declined comment with a reason is a correct \
1912            outcome; a change made to appease a reviewer is not.\n\
1913         4. Commit in this worktree. magi pushes to the pull request's branch \
1914            for you; do not push, merge, or close anything yourself.\n\
1915         5. Never name yourself, your vendor, or your model, anywhere.\n\n\
1916         # Output\n\n\
1917         Say what you changed and why, and what you declined and why.",
1918    );
1919
1920    let language = &state.config.graph.language;
1921    if !(language.trim().is_empty() || language.eq_ignore_ascii_case("en")) {
1922        let _ = write!(s, "\n\nWrite all prose in {language}.");
1923    }
1924    if let Some(overlay) = state.config.prompts.overlay("fix") {
1925        let _ = write!(s, "\n\n{overlay}");
1926    }
1927    s
1928}
1929
1930/// Failing log tails, the way the operator collects them by hand:
1931/// `gh run view --log-failed`.
1932async fn failing_logs(repo: &Path, failing: &[(String, String)]) -> String {
1933    let mut out = String::new();
1934    for (name, url) in failing.iter().take(MAX_LOGS) {
1935        let args = match (job_of(url), run_of(url)) {
1936            (Some(job), _) => vec![
1937                "run".to_owned(),
1938                "view".to_owned(),
1939                "--log-failed".to_owned(),
1940                "--job".to_owned(),
1941                job,
1942            ],
1943            (None, Some(run)) => vec![
1944                "run".to_owned(),
1945                "view".to_owned(),
1946                run,
1947                "--log-failed".to_owned(),
1948            ],
1949            // Not a GitHub Actions check - an external status has no log here.
1950            (None, None) => continue,
1951        };
1952        let (ok, body) = match gh(repo, &args).await {
1953            Ok(v) => v,
1954            Err(e) => (false, format!("{e:#}")),
1955        };
1956        if !ok && body.trim().is_empty() {
1957            continue;
1958        }
1959        let _ = write!(out, "### {name}\n\n```\n{}\n```\n\n", tail(&body, LOG_TAIL));
1960    }
1961    out
1962}
1963
1964/// Job id out of a check's `detailsUrl`
1965/// (`https://github.com/o/r/actions/runs/<run>/job/<job>`).
1966fn job_of(details_url: &str) -> Option<String> {
1967    let after = details_url.split("/job/").nth(1)?;
1968    let id: String = after.chars().take_while(char::is_ascii_digit).collect();
1969    (!id.is_empty()).then_some(id)
1970}
1971
1972/// Workflow run id out of a check's `detailsUrl`.
1973fn run_of(details_url: &str) -> Option<String> {
1974    let after = details_url.split("/actions/runs/").nth(1)?;
1975    let id: String = after.chars().take_while(char::is_ascii_digit).collect();
1976    (!id.is_empty()).then_some(id)
1977}
1978
1979/// Leave the pull request open, say why on it, and mark the run blocked.
1980///
1981/// The comment is what makes an unattended stop actionable: the operator wakes
1982/// up to a pull request that explains itself rather than to a silent queue.
1983async fn stop(state: &mut RunState, repo: &Path, pr: &PrState, why: &str) -> Result<()> {
1984    let body = format!(
1985        "{MARKER}\nmagi stopped landing this pull request: {why}\n\n\
1986         The branch is untouched and the run is `{}`. Nothing was merged.",
1987        state.id
1988    );
1989    let posted = gh(
1990        repo,
1991        &[
1992            "pr".to_owned(),
1993            "comment".to_owned(),
1994            pr.number.to_string(),
1995            "--body".to_owned(),
1996            body,
1997        ],
1998    )
1999    .await;
2000    match posted {
2001        Ok((true, _)) => {}
2002        Ok((false, out)) => tracing::warn!("could not comment on {}: {out}", pr.url),
2003        Err(e) => tracing::warn!("could not comment on {}: {e:#}", pr.url),
2004    }
2005    state.status = RunStatus::Blocked;
2006    state.merge = Some(MergeOutcome {
2007        mode: MergeMode::Pr,
2008        ok: false,
2009        detail: why.to_owned(),
2010    });
2011    state.event("land", format!("stopped: {why}"));
2012    state.save()?;
2013    Ok(())
2014}
2015
2016/// Run `gh` in `repo`, returning success and the combined output.
2017///
2018/// Combined because `gh` reports a refused merge on stderr and the pull request
2019/// json on stdout, and both are evidence.
2020async fn gh(cwd: &Path, args: &[String]) -> Result<(bool, String)> {
2021    let out = tokio::process::Command::new("gh")
2022        .args(args)
2023        .current_dir(cwd)
2024        .quiet()
2025        .stdin(std::process::Stdio::null())
2026        .output()
2027        .await
2028        .with_context(|| format!("spawn gh {}", args.join(" ")))?;
2029    let mut body = String::from_utf8_lossy(&out.stdout).into_owned();
2030    let err = String::from_utf8_lossy(&out.stderr);
2031    if body.trim().is_empty() {
2032        body = err.into_owned();
2033    } else if !err.trim().is_empty() {
2034        body.push_str(&err);
2035    }
2036    Ok((out.status.success(), body.trim().to_owned()))
2037}
2038
2039/// Verdict of one entry in the status rollup.
2040#[derive(Debug, Clone, Copy, PartialEq, Eq)]
2041enum Verdict {
2042    Pass,
2043    Fail,
2044    Pending,
2045    Unknown,
2046}
2047
2048#[derive(Debug, Deserialize)]
2049#[serde(rename_all = "camelCase")]
2050struct GhPr {
2051    #[serde(default)]
2052    url: String,
2053    #[serde(default)]
2054    number: u64,
2055    #[serde(default)]
2056    state: String,
2057    #[serde(default)]
2058    title: String,
2059    #[serde(default)]
2060    status_check_rollup: Vec<GhCheck>,
2061    /// GitHub's own verdict on whether the pull request can be merged.
2062    ///
2063    /// Worth asking for because it is the only place the *required* check set
2064    /// is applied: the rollup lists every check equally, so a repository that
2065    /// deliberately does not require `coverage` still looks red here. See
2066    /// [`Blocking`].
2067    #[serde(default)]
2068    merge_state_status: String,
2069    #[serde(default)]
2070    reviews: Vec<GhReview>,
2071    #[serde(default)]
2072    comments: Vec<GhComment>,
2073}
2074
2075/// One rollup entry. `gh` mixes two GraphQL types in this array: a `CheckRun`
2076/// has `name`/`status`/`conclusion`, while a `StatusContext` - the old commit
2077/// status API, which is how CodeRabbit reports - has `context`/`state` and no
2078/// conclusion at all.
2079#[derive(Debug, Deserialize)]
2080#[serde(rename_all = "camelCase")]
2081struct GhCheck {
2082    #[serde(default)]
2083    name: Option<String>,
2084    #[serde(default)]
2085    context: Option<String>,
2086    #[serde(default)]
2087    status: Option<String>,
2088    #[serde(default)]
2089    conclusion: Option<String>,
2090    #[serde(default)]
2091    state: Option<String>,
2092    #[serde(default)]
2093    details_url: Option<String>,
2094    #[serde(default)]
2095    target_url: Option<String>,
2096}
2097
2098impl GhCheck {
2099    /// Name to show a human and hand to the fixer.
2100    fn label(&self) -> String {
2101        self.name
2102            .clone()
2103            .or_else(|| self.context.clone())
2104            .unwrap_or_else(|| "(unnamed check)".to_owned())
2105    }
2106
2107    /// Where this check's logs live, when it has any.
2108    fn url(&self) -> Option<&str> {
2109        self.details_url
2110            .as_deref()
2111            .or(self.target_url.as_deref())
2112            .filter(|u| !u.is_empty())
2113    }
2114
2115    /// Did it pass?
2116    ///
2117    /// `SKIPPED` and `NEUTRAL` count as passed: the Claude review workflow
2118    /// skips release and bot pull requests by design, and a skip that blocked
2119    /// landing would block exactly the pull requests that need no review.
2120    /// `CANCELLED` counts as failed - a cancelled check did not pass, and
2121    /// merging over one is merging over a check that never ran.
2122    fn verdict(&self) -> Verdict {
2123        if let Some(status) = self.status.as_deref() {
2124            if !status.eq_ignore_ascii_case("COMPLETED") {
2125                return Verdict::Pending;
2126            }
2127        }
2128        let outcome = self
2129            .conclusion
2130            .as_deref()
2131            .or(self.state.as_deref())
2132            .unwrap_or("");
2133        match outcome.to_ascii_uppercase().as_str() {
2134            "SUCCESS" | "SKIPPED" | "NEUTRAL" => Verdict::Pass,
2135            "FAILURE" | "ERROR" | "TIMED_OUT" | "CANCELLED" | "STARTUP_FAILURE"
2136            | "ACTION_REQUIRED" => Verdict::Fail,
2137            "PENDING" | "EXPECTED" | "QUEUED" | "IN_PROGRESS" | "WAITING" | "REQUESTED" => {
2138                Verdict::Pending
2139            }
2140            _ => Verdict::Unknown,
2141        }
2142    }
2143}
2144
2145#[derive(Debug, Deserialize)]
2146struct GhAuthor {
2147    #[serde(default)]
2148    login: String,
2149}
2150
2151#[derive(Debug, Deserialize)]
2152struct GhReview {
2153    #[serde(default)]
2154    author: GhAuthor,
2155    #[serde(default)]
2156    body: String,
2157}
2158
2159#[derive(Debug, Deserialize)]
2160struct GhComment {
2161    #[serde(default)]
2162    author: GhAuthor,
2163    #[serde(default)]
2164    body: String,
2165}
2166
2167#[derive(Debug, Deserialize)]
2168struct GhUser {
2169    #[serde(default)]
2170    login: String,
2171}
2172
2173#[derive(Debug, Deserialize)]
2174struct GhInline {
2175    #[serde(default)]
2176    user: GhUser,
2177    #[serde(default)]
2178    path: Option<String>,
2179    #[serde(default)]
2180    line: Option<u64>,
2181    #[serde(default)]
2182    body: String,
2183}
2184
2185impl Default for GhAuthor {
2186    fn default() -> Self {
2187        Self {
2188            login: "(unknown)".to_owned(),
2189        }
2190    }
2191}
2192
2193impl Default for GhUser {
2194    fn default() -> Self {
2195        Self {
2196            login: "(unknown)".to_owned(),
2197        }
2198    }
2199}
2200
2201#[cfg(test)]
2202mod tests {
2203    use super::*;
2204    use crate::run::{Candidate, ReviewRecord, ReviewRound, Tally};
2205
2206    /// 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.
2207    const GREEN_OPEN: &str = r####"{
2208  "url": "https://github.com/yukimemi/magi/pull/10",
2209  "number": 10,
2210  "state": "OPEN",
2211  "mergeStateStatus": "CLEAN",
2212  "statusCheckRollup": [
2213    {
2214      "__typename": "CheckRun",
2215      "conclusion": "SKIPPED",
2216      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33356278334/job/99378963755",
2217      "name": "review",
2218      "status": "COMPLETED",
2219      "workflowName": "claude-review"
2220    },
2221    {
2222      "__typename": "CheckRun",
2223      "conclusion": "SUCCESS",
2224      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33356278338/job/99378963144",
2225      "name": "check (ubuntu-latest)",
2226      "status": "COMPLETED",
2227      "workflowName": "CI"
2228    },
2229    {
2230      "__typename": "CheckRun",
2231      "conclusion": "SUCCESS",
2232      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33356278338/job/99378963095",
2233      "name": "rustfmt",
2234      "status": "COMPLETED",
2235      "workflowName": "CI"
2236    },
2237    {
2238      "__typename": "StatusContext",
2239      "context": "CodeRabbit",
2240      "state": "SUCCESS",
2241      "targetUrl": ""
2242    }
2243  ],
2244  "reviews": [],
2245  "comments": [
2246    {
2247      "author": {
2248        "login": "coderabbitai"
2249      },
2250      "authorAssociation": "NONE",
2251      "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"
2252    }
2253  ]
2254}"####;
2255
2256    /// Real output for the open pull request #9 (the daily kata-apply), whose `editorconfig` check failed while everything else passed.
2257    const RED_OPEN: &str = r####"{
2258  "url": "https://github.com/yukimemi/magi/pull/9",
2259  "number": 9,
2260  "state": "OPEN",
2261  "mergeStateStatus": "UNSTABLE",
2262  "statusCheckRollup": [
2263    {
2264      "__typename": "CheckRun",
2265      "conclusion": "SUCCESS",
2266      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323744",
2267      "name": "check (ubuntu-latest)",
2268      "status": "COMPLETED",
2269      "workflowName": "CI"
2270    },
2271    {
2272      "__typename": "CheckRun",
2273      "conclusion": "SUCCESS",
2274      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323811",
2275      "name": "rustfmt",
2276      "status": "COMPLETED",
2277      "workflowName": "CI"
2278    },
2279    {
2280      "__typename": "CheckRun",
2281      "conclusion": "FAILURE",
2282      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323572",
2283      "name": "editorconfig",
2284      "status": "COMPLETED",
2285      "workflowName": "CI"
2286    },
2287    {
2288      "__typename": "StatusContext",
2289      "context": "CodeRabbit",
2290      "state": "SUCCESS",
2291      "targetUrl": ""
2292    }
2293  ],
2294  "reviews": [],
2295  "comments": [
2296    {
2297      "author": {
2298        "login": "coderabbitai"
2299      },
2300      "authorAssociation": "NONE",
2301      "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"
2302    }
2303  ]
2304}"####;
2305
2306    /// 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.
2307    const PENDING_OPEN: &str = r####"{
2308  "url": "https://github.com/yukimemi/magi/pull/9",
2309  "number": 9,
2310  "state": "OPEN",
2311  "statusCheckRollup": [
2312    {
2313      "__typename": "CheckRun",
2314      "conclusion": "SUCCESS",
2315      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323744",
2316      "name": "check (ubuntu-latest)",
2317      "status": "COMPLETED",
2318      "workflowName": "CI"
2319    },
2320    {
2321      "__typename": "CheckRun",
2322      "conclusion": "SUCCESS",
2323      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323811",
2324      "name": "rustfmt",
2325      "status": "COMPLETED",
2326      "workflowName": "CI"
2327    },
2328    {
2329      "__typename": "CheckRun",
2330      "conclusion": null,
2331      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323572",
2332      "name": "editorconfig",
2333      "status": "IN_PROGRESS",
2334      "workflowName": "CI"
2335    },
2336    {
2337      "__typename": "StatusContext",
2338      "context": "CodeRabbit",
2339      "state": "SUCCESS",
2340      "targetUrl": ""
2341    }
2342  ],
2343  "reviews": [],
2344  "comments": []
2345}"####;
2346
2347    /// Real output for pull request #16 after it was merged - the shape landing sees when a person merged underneath it.
2348    const MERGED: &str = r####"{
2349  "url": "https://github.com/yukimemi/magi/pull/16",
2350  "number": 16,
2351  "state": "MERGED",
2352  "statusCheckRollup": [
2353    {
2354      "__typename": "CheckRun",
2355      "conclusion": "SUCCESS",
2356      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33636587933/job/100268878095",
2357      "name": "check (ubuntu-latest)",
2358      "status": "COMPLETED",
2359      "workflowName": "CI"
2360    },
2361    {
2362      "__typename": "CheckRun",
2363      "conclusion": "SUCCESS",
2364      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33636587918/job/100268876427",
2365      "name": "review",
2366      "status": "COMPLETED",
2367      "workflowName": "claude-review"
2368    }
2369  ],
2370  "reviews": [],
2371  "comments": []
2372}"####;
2373
2374    /// 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.
2375    const REVIEWED_OPEN: &str = r####"{
2376  "url": "https://github.com/yukimemi/magi/pull/12",
2377  "number": 12,
2378  "state": "OPEN",
2379  "statusCheckRollup": [
2380    {
2381      "__typename": "CheckRun",
2382      "conclusion": "SUCCESS",
2383      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33571212506/job/100065355258",
2384      "name": "check (ubuntu-latest)",
2385      "status": "COMPLETED",
2386      "workflowName": "CI"
2387    },
2388    {
2389      "__typename": "CheckRun",
2390      "conclusion": "SUCCESS",
2391      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33571212566/job/100065355810",
2392      "name": "review",
2393      "status": "COMPLETED",
2394      "workflowName": "claude-review"
2395    }
2396  ],
2397  "reviews": [
2398    {
2399      "author": {
2400        "login": "claude"
2401      },
2402      "state": "COMMENTED",
2403      "body": ""
2404    }
2405  ],
2406  "comments": [
2407    {
2408      "author": {
2409        "login": "coderabbitai"
2410      },
2411      "authorAssociation": "NONE",
2412      "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"
2413    },
2414    {
2415      "author": {
2416        "login": "claude"
2417      },
2418      "authorAssociation": "NONE",
2419      "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"
2420    }
2421  ]
2422}"####;
2423
2424    /// Real `gh api repos/{owner}/{repo}/pulls/12/comments` output: one inline finding with its file and line.
2425    const INLINE: &str = r####"[
2426  {
2427    "user": {
2428      "login": "claude[bot]"
2429    },
2430    "path": "src/graph.rs",
2431    "line": 231,
2432    "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"
2433  }
2434]"####;
2435
2436    /// CodeRabbit's real trigger notice: a checkbox, a `<details>` block, and its own "skip review" marker.
2437    const CODERABBIT_TRIGGER: &str = r####"<!-- This is an auto-generated comment: summarize by coderabbit.ai -->
2438<!-- This is an auto-generated comment: skip review by coderabbit.ai -->
2439
2440> [!IMPORTANT]
2441> - [ ] <!-- {"checkboxId":"e9bb8d72-00e8-4f67-9cb2-caf3b22574fe"} --> 🔍 Trigger review
2442> 
2443> This repository does not receive automatic reviews because it has fewer than 10 stars.
2444> 
2445> <details>
2446> <summary>⚙️ Run configuration</summary>
2447> 
2448> **Configuration used**: defaults
2449> 
2450> **Review profile**: CHILL
2451> 
2452> **Plan**: Team
2453> 
2454> **Run ID**: `c1e2a68f-87fc-4b35-9ec4-e75c7854966a`
2455> 
2456> </details>
2457
2458<!-- end of auto-generated comment: skip review by coderabbit.ai -->
2459
2460<!-- tips_start -->
2461
2462---
2463
2464Thanks 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.
2465
2466<details>
2467<summary>❤️ Share</summary>
2468
2469- [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"####;
2470
2471    /// 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.
2472    const CLAUDE_CHECKLIST: &str = r####"**Claude finished @yukimemi's task in 4m 14s** —— [View job](https://github.com/yukimemi/magi/actions/runs/33636587918)
2473
2474---
2475### Reviewing PR #16
2476
2477- [x] Read AGENTS.md conventions
2478- [x] Review `src/daemon.rs` changes
2479- [x] Review `src/main.rs` changes (new `doctor` reporting)
2480- [x] Review `src/web.rs` changes (reuse of unreadable-run count)
2481- [x] Check test coverage for new behavior
2482- [x] Run verification commands (blocked — see note)
2483- [x] Post findings"####;
2484
2485    /// The same job's real comment on pull request #12 once it had something to say.
2486    const CLAUDE_FINDING: &str = r####"**Claude finished @yukimemi's task in 3m 52s** —— [View job](https://github.com/yukimemi/magi/actions/runs/33571212566)
2487
2488---
2489### Review: `magi review <branch>` — cheap-half-only graph
2490
2491Read 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.
2492
2493**Correctness**
2494
2495- 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"####;
2496
2497    fn pr(checks: Checks, failing: &[&str], comments: usize) -> PrState {
2498        PrState {
2499            url: "https://github.com/yukimemi/magi/pull/16".to_owned(),
2500            number: 16,
2501            state: PrLifecycle::Open,
2502            checks,
2503            // These tests are about red-means-fix, so a red here is one the
2504            // forge gates on. Without saying so they would assert the new
2505            // "merge past a check nobody requires" path by accident.
2506            blocking: if matches!(checks, Checks::Red) {
2507                Blocking::Yes
2508            } else {
2509                Blocking::No
2510            },
2511            failing: failing.iter().map(|s| (*s).to_owned()).collect(),
2512            review_comments: (0..comments)
2513                .map(|i| ReviewComment {
2514                    author: "coderabbitai".to_owned(),
2515                    path: Some("src/graph.rs".to_owned()),
2516                    line: Some(231),
2517                    body: format!("finding {i}"),
2518                })
2519                .collect(),
2520        }
2521    }
2522
2523    #[test]
2524    fn a_green_pull_request_with_nothing_outstanding_parses_as_ready_to_merge() {
2525        let state = parse_pr(GREEN_OPEN).expect("green fixture parses");
2526        assert_eq!(state.number, 10);
2527        assert_eq!(state.state, PrLifecycle::Open);
2528        assert_eq!(state.checks, Checks::Green);
2529        assert!(state.failing.is_empty());
2530        assert!(
2531            state.review_comments.is_empty(),
2532            "the only comment is CodeRabbit's trigger notice: {:?}",
2533            state.review_comments
2534        );
2535        assert_eq!(decide(&state, 0, 4, Duration::ZERO), Step::Merge);
2536    }
2537
2538    #[test]
2539    fn a_failing_check_parses_as_red_and_is_named() {
2540        let state = parse_pr(RED_OPEN).expect("red fixture parses");
2541        assert_eq!(state.checks, Checks::Red);
2542        assert_eq!(state.failing, vec!["editorconfig".to_owned()]);
2543        // The captured payload says `UNSTABLE` - mergeable, with a check
2544        // nobody requires red - which is exactly the shape that had to be
2545        // merged by hand. Asserted separately, in
2546        // `a_red_check_nobody_requires_does_not_buy_a_fix_round`. What this
2547        // test is about is that a red check is *named*, so the reason a fixer
2548        // is handed says which one; so it asks the blocking question here.
2549        let mut blocking = state.clone();
2550        blocking.blocking = Blocking::Yes;
2551        match decide(&blocking, 0, 4, Duration::ZERO) {
2552            Step::Fix { reason } => {
2553                assert!(reason.contains("editorconfig"), "reason: {reason}");
2554                assert!(reason.contains("failing"), "reason: {reason}");
2555            }
2556            other => panic!("expected a fix round, got {other:?}"),
2557        }
2558    }
2559
2560    #[test]
2561    fn a_check_still_running_parses_as_pending_and_is_waited_for() {
2562        let state = parse_pr(PENDING_OPEN).expect("pending fixture parses");
2563        assert_eq!(state.checks, Checks::Pending);
2564        assert_eq!(decide(&state, 0, 4, Duration::ZERO), Step::Wait);
2565    }
2566
2567    #[test]
2568    fn a_pull_request_merged_underneath_us_is_done_rather_than_a_failure() {
2569        let state = parse_pr(MERGED).expect("merged fixture parses");
2570        assert_eq!(state.state, PrLifecycle::Merged);
2571        assert_eq!(
2572            decide(&state, 0, 4, Duration::ZERO),
2573            Step::Done { merged: true }
2574        );
2575    }
2576
2577    #[test]
2578    fn a_review_that_found_something_is_outstanding_and_holds_the_merge() {
2579        let state = parse_pr(REVIEWED_OPEN).expect("reviewed fixture parses");
2580        assert_eq!(state.checks, Checks::Green);
2581        let authors: Vec<&str> = state
2582            .review_comments
2583            .iter()
2584            .map(|c| c.author.as_str())
2585            .collect();
2586        assert_eq!(
2587            authors,
2588            vec!["claude"],
2589            "CodeRabbit's walkthrough is machinery; Claude's review is a finding"
2590        );
2591        match decide(&state, 0, 4, Duration::ZERO) {
2592            Step::Fix { reason } => assert!(reason.contains("unresolved"), "reason: {reason}"),
2593            other => panic!("expected a fix round, got {other:?}"),
2594        }
2595    }
2596
2597    #[test]
2598    fn inline_review_comments_keep_their_file_and_line() {
2599        let comments = parse_inline_comments(INLINE).expect("inline fixture parses");
2600        assert_eq!(comments.len(), 1);
2601        assert_eq!(comments[0].author, "claude[bot]");
2602        assert_eq!(comments[0].path.as_deref(), Some("src/graph.rs"));
2603        assert_eq!(comments[0].line, Some(231));
2604        assert!(comments[0].body.contains("empty"), "{}", comments[0].body);
2605    }
2606
2607    #[test]
2608    fn a_status_only_bot_comment_does_not_trigger_a_fix_round() {
2609        assert!(
2610            is_noise(CODERABBIT_TRIGGER),
2611            "CodeRabbit's trigger notice declares itself not a review"
2612        );
2613        assert!(
2614            is_noise(CLAUDE_CHECKLIST),
2615            "a progress checklist asks for nothing"
2616        );
2617        assert!(
2618            !is_noise(CLAUDE_FINDING),
2619            "a review that names a bug is input, not noise"
2620        );
2621
2622        let mut clean = pr(Checks::Green, &[], 0);
2623        clean.review_comments.push(ReviewComment {
2624            author: "coderabbitai".to_owned(),
2625            path: None,
2626            line: None,
2627            body: CODERABBIT_TRIGGER.to_owned(),
2628        });
2629        clean.review_comments.retain(|c| !is_noise(&c.body));
2630        assert_eq!(decide(&clean, 0, 4, Duration::ZERO), Step::Merge);
2631
2632        let mut found = pr(Checks::Green, &[], 0);
2633        found.review_comments.push(ReviewComment {
2634            author: "claude".to_owned(),
2635            path: None,
2636            line: None,
2637            body: CLAUDE_FINDING.to_owned(),
2638        });
2639        found.review_comments.retain(|c| !is_noise(&c.body));
2640        assert!(matches!(
2641            decide(&found, 0, 4, Duration::ZERO),
2642            Step::Fix { .. }
2643        ));
2644    }
2645
2646    #[test]
2647    fn the_policy_table_holds_for_every_combination_that_matters() {
2648        let cases: Vec<(&str, PrState, usize, usize, Duration, Step)> = vec![
2649            (
2650                "pending checks are waited for, even on the last round",
2651                pr(Checks::Pending, &[], 0),
2652                4,
2653                4,
2654                Duration::ZERO,
2655                Step::Wait,
2656            ),
2657            (
2658                "red checks are fixed",
2659                pr(Checks::Red, &["editorconfig"], 0),
2660                0,
2661                4,
2662                Duration::ZERO,
2663                Step::Fix {
2664                    reason: "1 check(s) failing: editorconfig".to_owned(),
2665                },
2666            ),
2667            (
2668                "green with comments is fixed, not merged",
2669                pr(Checks::Green, &[], 2),
2670                1,
2671                4,
2672                Duration::ZERO,
2673                Step::Fix {
2674                    reason: "checks are green but 2 review comment(s) are unresolved: coderabbitai"
2675                        .to_owned(),
2676                },
2677            ),
2678            (
2679                "green and clean merges",
2680                pr(Checks::Green, &[], 0),
2681                3,
2682                4,
2683                Duration::ZERO,
2684                Step::Merge,
2685            ),
2686            (
2687                "an unreadable rollup is waited on while the grace lasts",
2688                pr(Checks::Unknown, &[], 0),
2689                0,
2690                4,
2691                Duration::ZERO,
2692                Step::Wait,
2693            ),
2694            (
2695                "an unreadable rollup is never merged once the grace is spent",
2696                pr(Checks::Unknown, &[], 0),
2697                0,
2698                4,
2699                CHECKS_GRACE,
2700                Step::GiveUp {
2701                    reason: "no check status is readable on the pull request after 3 minute(s); \
2702                             refusing to merge on a guess"
2703                        .to_owned(),
2704                },
2705            ),
2706        ];
2707        for (what, state, round, budget, waited, want) in cases {
2708            assert_eq!(decide(&state, round, budget, waited), want, "{what}");
2709        }
2710    }
2711
2712    #[test]
2713    fn the_forge_verdict_survives_the_round_trip_from_gh() {
2714        // Read off `gh pr view --json ...,mergeStateStatus`, because a field
2715        // requested but never parsed is the kind of thing that looks wired up
2716        // and answers `Unsaid` forever.
2717        let green = parse_pr(GREEN_OPEN).expect("parse");
2718        assert_eq!(green.blocking, Blocking::No);
2719        let red = parse_pr(RED_OPEN).expect("parse");
2720        assert_eq!(
2721            red.blocking,
2722            Blocking::No,
2723            "`UNSTABLE` is mergeable: the red check is one nobody requires"
2724        );
2725        assert_eq!(red.checks, Checks::Red, "and it is still reported as red");
2726        // A payload from an older `gh` has no such field at all.
2727        let quiet =
2728            parse_pr(&GREEN_OPEN.replace("\"mergeStateStatus\": \"CLEAN\",", "")).expect("parse");
2729        assert_eq!(quiet.blocking, Blocking::Unsaid);
2730    }
2731
2732    #[test]
2733    fn a_red_check_nobody_requires_does_not_buy_a_fix_round() {
2734        // Pull request 37's only red check was `editorconfig`, failing
2735        // because the action could not fetch its own binary after
2736        // editorconfig-checker v4 renamed its release assets. The repository
2737        // does not require it. magi answered by asking a fixer to repair a
2738        // change that was fine, and the pull request had to be merged by hand.
2739        let mut nonblocking = pr(Checks::Red, &["editorconfig", "coverage"], 0);
2740        nonblocking.blocking = Blocking::No;
2741        assert_eq!(
2742            decide(&nonblocking, 0, 4, Duration::ZERO),
2743            Step::Merge,
2744            "the forge says nothing is in the way, so nothing is"
2745        );
2746
2747        // The same red, gated on: that is a fix round, as before.
2748        let mut blocking = pr(Checks::Red, &["test (ubuntu-latest)"], 0);
2749        blocking.blocking = Blocking::Yes;
2750        assert!(matches!(
2751            decide(&blocking, 0, 4, Duration::ZERO),
2752            Step::Fix { .. }
2753        ));
2754
2755        // A review comment still outranks green-enough: a non-required red
2756        // must not become a way to merge past an unanswered reviewer.
2757        let mut commented = pr(Checks::Red, &["coverage"], 1);
2758        commented.blocking = Blocking::No;
2759        assert!(matches!(
2760            decide(&commented, 0, 4, Duration::ZERO),
2761            Step::Fix { .. }
2762        ));
2763
2764        // And silence from the forge is not consent.
2765        let mut unsaid = pr(Checks::Red, &["coverage"], 0);
2766        unsaid.blocking = Blocking::Unsaid;
2767        assert!(matches!(
2768            decide(&unsaid, 0, 4, Duration::ZERO),
2769            Step::Fix { .. }
2770        ));
2771    }
2772
2773    #[test]
2774    fn a_branch_the_base_moved_under_is_rebased_not_fixed() {
2775        // Pull requests 35 and 37 were both rebased by hand: a competition
2776        // that runs for two hours against a repository merging pull requests
2777        // all day conflicts on the way in, and that is arithmetic rather
2778        // than a defect in the change.
2779        let mut conflicted = pr(Checks::Green, &[], 0);
2780        conflicted.blocking = Blocking::Conflict;
2781        assert_eq!(decide(&conflicted, 0, 4, Duration::ZERO), Step::Rebase);
2782
2783        // Decided before the checks, and even with the rounds spent: every
2784        // check on a branch that cannot land is an answer about a state that
2785        // cannot land, and a conflict is not the change's fault.
2786        let mut red = pr(Checks::Red, &["test (ubuntu-latest)"], 2);
2787        red.blocking = Blocking::Conflict;
2788        assert_eq!(decide(&red, 4, 4, Duration::ZERO), Step::Rebase);
2789
2790        // The lifecycle still wins over everything, conflict included.
2791        let mut merged = pr(Checks::Red, &[], 0);
2792        merged.blocking = Blocking::Conflict;
2793        merged.state = PrLifecycle::Merged;
2794        assert_eq!(
2795            decide(&merged, 0, 4, Duration::ZERO),
2796            Step::Done { merged: true }
2797        );
2798    }
2799
2800    #[test]
2801    fn the_forge_verdict_is_read_off_merge_state_status() {
2802        // The spellings that mean "mergeable". `UNSTABLE` is the one that
2803        // matters: mergeable, with a non-required check red or still running.
2804        for ok in ["CLEAN", "UNSTABLE", "unstable", "HAS_HOOKS"] {
2805            assert_eq!(Blocking::of(ok), Blocking::No, "{ok}");
2806            assert!(!Blocking::of(ok).stops_a_merge(), "{ok}");
2807        }
2808        assert_eq!(Blocking::of("DIRTY"), Blocking::Conflict);
2809        assert_eq!(Blocking::of("BLOCKED"), Blocking::Yes);
2810        assert_eq!(Blocking::of("BEHIND"), Blocking::Yes);
2811        // An older `gh`, or a token without the scope, says nothing - and
2812        // refusing to guess is the rule everywhere else in this module.
2813        for quiet in ["", "UNKNOWN"] {
2814            assert_eq!(Blocking::of(quiet), Blocking::Unsaid);
2815            assert!(Blocking::of(quiet).stops_a_merge());
2816        }
2817    }
2818
2819    #[test]
2820    fn a_merge_command_that_failed_after_merging_is_still_a_merge() {
2821        let argv = merge_argv(28, "fix: retry uploads on transient network errors");
2822        // The exact stderr from run ec12, in a jj-colocated repository.
2823        let jj = "could not determine current branch: failed to run git: not on any branch";
2824
2825        let landed = merged_after_all(&argv, jj, Some(PrLifecycle::Merged))
2826            .expect("the forge says merged, so it merged");
2827        assert!(landed.ok);
2828        assert!(
2829            landed.detail.contains("but the pull request is merged"),
2830            "the record must not read as a clean success: {}",
2831            landed.detail
2832        );
2833        assert!(
2834            landed.detail.contains("not on any branch"),
2835            "and it must keep what the command actually said: {}",
2836            landed.detail
2837        );
2838
2839        // A pull request still open means the merge really failed.
2840        assert!(merged_after_all(&argv, jj, Some(PrLifecycle::Open)).is_none());
2841        assert!(merged_after_all(&argv, jj, Some(PrLifecycle::Closed)).is_none());
2842        // And an unreadable answer is not evidence of success.
2843        assert!(merged_after_all(&argv, jj, None).is_none());
2844    }
2845
2846    #[test]
2847    fn a_pull_request_closed_underneath_us_is_done_and_not_merged() {
2848        let mut state = pr(Checks::Red, &["editorconfig"], 3);
2849        state.state = PrLifecycle::Closed;
2850        assert_eq!(
2851            decide(&state, 0, 4, Duration::ZERO),
2852            Step::Done { merged: false },
2853            "a human closing the pull request ends the loop, whatever CI says"
2854        );
2855    }
2856
2857    #[test]
2858    fn the_last_round_gives_up_with_a_reason_naming_what_is_still_failing() {
2859        let red = decide(
2860            &pr(Checks::Red, &["editorconfig", "test (macos)"], 0),
2861            4,
2862            4,
2863            Duration::ZERO,
2864        );
2865        match red {
2866            Step::GiveUp { reason } => {
2867                assert!(reason.contains("editorconfig"), "reason: {reason}");
2868                assert!(reason.contains("test (macos)"), "reason: {reason}");
2869                assert!(reason.contains("4 fix round(s)"), "reason: {reason}");
2870            }
2871            other => panic!("expected a give-up, got {other:?}"),
2872        }
2873
2874        let commented = decide(&pr(Checks::Green, &[], 1), 2, 2, Duration::ZERO);
2875        match commented {
2876            Step::GiveUp { reason } => {
2877                assert!(reason.contains("unresolved"), "reason: {reason}");
2878                assert!(reason.contains("2 fix round(s)"), "reason: {reason}");
2879            }
2880            other => panic!("expected a give-up, got {other:?}"),
2881        }
2882    }
2883
2884    #[test]
2885    fn the_merge_command_squashes_deletes_the_branch_and_sets_its_own_subject() {
2886        let candidate_commit = "magi: candidate A (uncommitted work)";
2887        let subject = merge_subject(candidate_commit, "add retries to the uploader");
2888        let argv = merge_argv(16, &subject);
2889
2890        assert!(argv.contains(&"--squash".to_owned()));
2891        assert!(argv.contains(&"--delete-branch".to_owned()));
2892        assert!(argv.contains(&"--subject".to_owned()));
2893        assert_eq!(
2894            argv.last().map(String::as_str),
2895            Some("add retries to the uploader"),
2896            "the subject must not be the candidate commit message"
2897        );
2898        assert_ne!(subject, candidate_commit);
2899    }
2900
2901    #[test]
2902    fn a_real_pull_request_title_is_used_as_the_squash_subject_verbatim() {
2903        assert_eq!(
2904            merge_subject("feat: a queue, an unattended loop, and a phone UI", "task"),
2905            "feat: a queue, an unattended loop, and a phone UI"
2906        );
2907        assert_eq!(
2908            merge_subject("", "# port the retry logic\n\ndetails"),
2909            "port the retry logic",
2910            "an empty title falls back to the task's first line, heading marks stripped"
2911        );
2912    }
2913
2914    #[test]
2915    fn a_failing_checks_details_url_yields_the_job_to_read_logs_from() {
2916        let url = "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323572";
2917        assert_eq!(job_of(url).as_deref(), Some("100114323572"));
2918        assert_eq!(run_of(url).as_deref(), Some("33587406996"));
2919        assert_eq!(job_of("https://coderabbit.ai/status"), None);
2920        assert_eq!(run_of(""), None);
2921    }
2922
2923    #[test]
2924    fn magis_own_stop_comment_is_never_read_back_as_a_finding() {
2925        let mut out = Vec::new();
2926        push_if_outstanding(
2927            &mut out,
2928            ReviewComment {
2929                author: "yukimemi".to_owned(),
2930                path: None,
2931                line: None,
2932                body: format!("{MARKER}\nmagi stopped landing this pull request: 1 check failing"),
2933            },
2934        );
2935        assert!(out.is_empty());
2936    }
2937
2938    /// A run with no tally, so [`RunState::winner`] is `None` and the panel
2939    /// falls back to the repository - which keeps these tests free of a
2940    /// worktree, a `git` invocation and a network.
2941    fn run_state() -> RunState {
2942        RunState::new(
2943            std::path::PathBuf::from("/repo/magi"),
2944            "main".to_owned(),
2945            "abcdef1234".to_owned(),
2946            "add retries to the uploader".to_owned(),
2947            crate::config::Config::default(),
2948        )
2949    }
2950
2951    fn green_pr() -> PrState {
2952        PrState {
2953            url: "https://github.com/yukimemi/magi/pull/42".to_owned(),
2954            number: 42,
2955            state: PrLifecycle::Open,
2956            checks: Checks::Green,
2957            // The forge sees nothing in the way unless a test says otherwise.
2958            blocking: Blocking::No,
2959            failing: Vec::new(),
2960            review_comments: vec![ReviewComment {
2961                author: "coderabbitai".to_owned(),
2962                path: Some("src/land.rs".to_owned()),
2963                line: Some(212),
2964                body: "this branch never checks the exit code".to_owned(),
2965            }],
2966        }
2967    }
2968
2969    const NUMSTAT: &str = "12\t3\tsrc/land.rs\n40\t1\tsrc/web.rs\n-\t-\tassets/logo.png";
2970
2971    fn panel() -> String {
2972        approval_panel(
2973            &run_state(),
2974            &green_pr(),
2975            NUMSTAT,
2976            "diff --git a/src/land.rs b/src/land.rs\n@@ -1,2 +1,2 @@\n-old line\n+new line\n context",
2977            &[
2978                "land: ask before merging".to_owned(),
2979                "land: colour the diff".to_owned(),
2980            ],
2981            "feat: merge approval from the phone",
2982        )
2983    }
2984
2985    #[test]
2986    fn the_approval_panel_carries_the_whole_case_for_the_merge() {
2987        let html = panel();
2988        for needle in [
2989            "42",
2990            "main",
2991            "src/land.rs",
2992            "src/web.rs",
2993            "assets/logo.png",
2994            "feat: merge approval from the phone",
2995            "land: ask before merging",
2996            "land: colour the diff",
2997            "coderabbitai",
2998            "this branch never checks the exit code",
2999            "green",
3000        ] {
3001            assert!(html.contains(needle), "the panel must state `{needle}`");
3002        }
3003    }
3004
3005    /// A candidate whose label is `A` and has won, so [`RunState::winner`]
3006    /// resolves to it.
3007    fn winning_candidate(summary: &str) -> Candidate {
3008        Candidate {
3009            index: 0,
3010            label: 'A',
3011            agent: "opus".to_owned(),
3012            branch: "magi/x/A".to_owned(),
3013            worktree: PathBuf::from("/wt/A"),
3014            summary: summary.to_owned(),
3015            stat: String::new(),
3016            files: 1,
3017            commits: 1,
3018            empty: false,
3019            failed: None,
3020            duration_ms: 0,
3021            folded: false,
3022        }
3023    }
3024
3025    fn uncontested_tally() -> Tally {
3026        Tally {
3027            first_choice: BTreeMap::from([('A', 1)]),
3028            borda: BTreeMap::new(),
3029            winner: 'A',
3030            rankings: 1,
3031            unanimous_initial: true,
3032            deliberated: false,
3033            changed_votes: 0,
3034            unanimous_final: true,
3035            tie_break: None,
3036            judges: 1,
3037            present: 1,
3038            quorum: 1,
3039            met_quorum: true,
3040            uncontested: None,
3041        }
3042    }
3043
3044    fn review_record(reviewer: usize, agent: &str, summary: &str) -> ReviewRecord {
3045        ReviewRecord {
3046            reviewer,
3047            agent: agent.to_owned(),
3048            summary: summary.to_owned(),
3049            findings: Vec::new(),
3050            vote: None,
3051            failed: None,
3052            duration_ms: 0,
3053        }
3054    }
3055
3056    fn review_round(round: usize, reviews: Vec<ReviewRecord>) -> ReviewRound {
3057        let answered = reviews.len();
3058        ReviewRound {
3059            round,
3060            head: "abc1234".to_owned(),
3061            verified_head: None,
3062            reviews,
3063            e2e: Vec::new(),
3064            verify_retried: false,
3065            e2e_deferred: false,
3066            e2e_defer_reason: None,
3067            fix: None,
3068            blocking: 0,
3069            answered,
3070            expected: answered,
3071            clean: true,
3072            progressed: false,
3073            vote_split: false,
3074            reconsideration: Vec::new(),
3075            verdict: None,
3076        }
3077    }
3078
3079    #[test]
3080    fn the_approval_panel_states_the_task_verbatim_in_either_language() {
3081        let en = panel();
3082        assert!(en.contains("Task"), "{en}");
3083        assert!(en.contains("add retries to the uploader"), "{en}");
3084
3085        let mut state = run_state();
3086        state.config.graph.language = "ja".to_owned();
3087        let ja = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3088        assert!(ja.contains("タスク"), "{ja}");
3089        assert!(
3090            ja.contains("add retries to the uploader"),
3091            "the task itself is not translated: {ja}"
3092        );
3093    }
3094
3095    #[test]
3096    fn the_approval_panel_omits_what_changed_and_review_verdict_with_no_data() {
3097        // `run_state()` has no candidates, no tally and no reviews - exactly
3098        // the shape a run has before anything has judged or reviewed it, and
3099        // the panel must not print an empty box for either.
3100        let html = panel();
3101        assert!(!html.contains("What changed"), "{html}");
3102        assert!(!html.contains("Review verdict"), "{html}");
3103    }
3104
3105    #[test]
3106    fn the_approval_panel_omits_what_changed_when_the_winners_summary_is_empty() {
3107        let mut state = run_state();
3108        state.candidates = vec![winning_candidate("")];
3109        state.tally = Some(uncontested_tally());
3110        let html = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3111        assert!(
3112            !html.contains("What changed"),
3113            "an empty summary must not render an empty box: {html}"
3114        );
3115    }
3116
3117    #[test]
3118    fn the_approval_panel_shows_the_winners_own_account_in_either_language() {
3119        let mut state = run_state();
3120        state.candidates = vec![winning_candidate(
3121            "Added a retry loop around the uploader PUT call.",
3122        )];
3123        state.tally = Some(uncontested_tally());
3124        let en = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3125        assert!(en.contains("What changed"), "{en}");
3126        assert!(
3127            en.contains("Added a retry loop around the uploader PUT call."),
3128            "{en}"
3129        );
3130
3131        state.config.graph.language = "ja".to_owned();
3132        let ja = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3133        assert!(ja.contains("変更内容"), "{ja}");
3134        assert!(
3135            ja.contains("Added a retry loop around the uploader PUT call."),
3136            "{ja}"
3137        );
3138    }
3139
3140    #[test]
3141    fn the_approval_panel_shows_only_the_last_review_rounds_verdict() {
3142        let mut state = run_state();
3143        state.reviews = vec![
3144            review_round(
3145                1,
3146                vec![review_record(1, "alpha", "found a race, sent back")],
3147            ),
3148            review_round(2, vec![review_record(1, "alpha", "race is fixed, clean")]),
3149        ];
3150        let en = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3151        assert!(en.contains("Review verdict"), "{en}");
3152        assert!(en.contains("race is fixed, clean"), "{en}");
3153        assert!(
3154            !en.contains("found a race, sent back"),
3155            "only the round that actually cleared the merge should show: {en}"
3156        );
3157
3158        state.config.graph.language = "ja".to_owned();
3159        let ja = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3160        assert!(ja.contains("レビューの結論"), "{ja}");
3161        assert!(ja.contains("レビュアー"), "{ja}");
3162        assert!(ja.contains("race is fixed, clean"), "{ja}");
3163    }
3164
3165    /// The `incomplete_review = "warn"` policy (see
3166    /// `graph::Runner::review_loop`) can push a `clean` round to
3167    /// `state.reviews` while one seat's own record still has `failed: Some`
3168    /// and an empty `summary` - a seat that never answered, not one that
3169    /// answered with nothing to say.
3170    fn unanswered_review_record(reviewer: usize, agent: &str, reason: &str) -> ReviewRecord {
3171        ReviewRecord {
3172            reviewer,
3173            agent: agent.to_owned(),
3174            summary: String::new(),
3175            findings: Vec::new(),
3176            vote: None,
3177            failed: Some(reason.to_owned()),
3178            duration_ms: 0,
3179        }
3180    }
3181
3182    #[test]
3183    fn the_approval_panel_never_shows_an_unanswered_seat_as_a_blank_verdict() {
3184        let mut state = run_state();
3185        state.reviews = vec![review_round(
3186            1,
3187            vec![
3188                review_record(1, "alpha", "clean, nothing to add"),
3189                unanswered_review_record(2, "beta", "timed out"),
3190            ],
3191        )];
3192        let en = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3193        assert!(en.contains("clean, nothing to add"), "{en}");
3194        assert!(
3195            en.contains("produced no answer: timed out"),
3196            "a seat that never answered must say so, not render a blank box: {en}"
3197        );
3198        assert!(
3199            !en.contains("<div style=\"white-space:pre-wrap;font-size:13px\"></div>"),
3200            "no reviewer box may be left empty: {en}"
3201        );
3202
3203        state.config.graph.language = "ja".to_owned();
3204        let ja = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3205        assert!(ja.contains("回答なし: timed out"), "{ja}");
3206    }
3207
3208    #[test]
3209    fn the_approval_panel_contains_nothing_the_frames_policy_would_block() {
3210        let html = panel();
3211        assert!(!html.contains("<script"), "no script survives the csp");
3212        assert!(!html.contains("<form"), "form-action is 'none'");
3213        let pr = green_pr();
3214        assert_eq!(
3215            html.matches("http").count(),
3216            html.matches(pr.url.as_str()).count(),
3217            "the only http url in the panel is the pull request's own link"
3218        );
3219    }
3220
3221    #[test]
3222    fn added_and_removed_diff_lines_are_distinguishable_without_colour() {
3223        let html = panel();
3224        assert!(
3225            html.contains(">+</span>"),
3226            "an added line carries a `+` in the gutter, not only a background"
3227        );
3228        assert!(
3229            html.contains(">-</span>"),
3230            "a removed line carries a `-` in the gutter, not only a background"
3231        );
3232        assert!(
3233            html.contains(">new line</span>"),
3234            "the marker is moved to the gutter, so the body is printed once without it"
3235        );
3236    }
3237
3238    #[test]
3239    fn a_diff_past_the_threshold_is_cut_with_an_honest_count() {
3240        let total = DIFF_MAX_LINES + 100;
3241        let diff: String = (0..total).map(|i| format!("+line {i}\n")).collect();
3242        let html = approval_panel(
3243            &run_state(),
3244            &green_pr(),
3245            NUMSTAT,
3246            &diff,
3247            &[],
3248            "feat: something long",
3249        );
3250        assert!(
3251            html.contains(&format!("100 of {total} diff lines omitted")),
3252            "the note must say exactly how much was cut"
3253        );
3254        assert!(html.contains(&format!("line {}", DIFF_MAX_LINES - 1)));
3255        assert!(
3256            !html.contains(&format!("line {DIFF_MAX_LINES}")),
3257            "nothing past the threshold is rendered"
3258        );
3259        assert!(
3260            html.contains("/repo/magi"),
3261            "the note says where the rest is"
3262        );
3263    }
3264
3265    #[test]
3266    fn a_path_with_html_metacharacters_is_escaped_rather_than_rendered() {
3267        let html = approval_panel(
3268            &run_state(),
3269            &green_pr(),
3270            "1\t2\tsrc/<b>&\"x\"'.rs",
3271            "",
3272            &[],
3273            "subject",
3274        );
3275        assert!(html.contains("src/&lt;b&gt;&amp;&quot;x&quot;&#39;.rs"));
3276        assert!(
3277            !html.contains("<b>"),
3278            "an agent-influenced path must never become markup"
3279        );
3280    }
3281
3282    #[tokio::test]
3283    async fn the_merge_lock_serialises_one_repository_but_never_a_different_one() {
3284        let a = std::path::PathBuf::from("/repo/a");
3285        let b = std::path::PathBuf::from("/repo/b");
3286
3287        let held = repo_merge_lock(&a).lock_owned().await;
3288
3289        // A second, concurrent land run against the *same* repository must
3290        // wait - `try_lock` fails while `held` is alive.
3291        assert!(
3292            repo_merge_lock(&a).try_lock().is_err(),
3293            "a second merge into the same repository must not proceed concurrently"
3294        );
3295
3296        // A run against a *different* repository must not be blocked by it -
3297        // this is what keeps a slow rebase or `gh pr merge` in one
3298        // repository from also stalling a land-approval resume in another.
3299        assert!(
3300            repo_merge_lock(&b).try_lock().is_ok(),
3301            "a different repository's merge lock must be independent"
3302        );
3303
3304        drop(held);
3305        assert!(
3306            repo_merge_lock(&a).try_lock().is_ok(),
3307            "the lock is released once the holder is done"
3308        );
3309    }
3310
3311    #[test]
3312    fn only_the_merge_choice_merges_and_silence_holds() {
3313        let table = [
3314            (None, Approval::Hold),
3315            (Some("merge"), Approval::Merge),
3316            (Some(" merge\n"), Approval::Merge),
3317            (Some("hold"), Approval::Hold),
3318            (Some(""), Approval::Hold),
3319            (Some("yes"), Approval::Hold),
3320        ];
3321        for (answer, want) in table {
3322            assert_eq!(
3323                approval(answer),
3324                want,
3325                "answer {answer:?} must resolve to {want:?}"
3326            );
3327        }
3328    }
3329
3330    #[tokio::test]
3331    async fn a_first_visit_to_the_merge_gate_files_a_question_and_returns_pending_at_once() {
3332        crate::run::set_home(std::env::temp_dir().join("magi-land-approval-test-home"));
3333        let mut state = run_state();
3334        state.config.graph.land_approval = true;
3335        let pr = green_pr();
3336
3337        let gate = approval_gate(&mut state, &pr, "feat: x").await.unwrap();
3338        assert_eq!(gate, ApprovalGate::Pending, "nobody has answered yet");
3339        assert!(
3340            !state.parked,
3341            "approval_gate itself never sets `parked`; only its caller does"
3342        );
3343
3344        let store = ask::Questions::open();
3345        let filed: Vec<_> = store
3346            .list()
3347            .into_iter()
3348            .filter(|q| q.run == state.id)
3349            .collect();
3350        assert_eq!(filed.len(), 1, "exactly one question is filed");
3351        assert_eq!(filed[0].node, APPROVAL_NODE);
3352        assert_eq!(filed[0].choices, vec![APPROVE.to_owned(), HOLD.to_owned()]);
3353        assert!(filed[0].status.open());
3354
3355        // A second visit - standing in for a resumed run whose slot the
3356        // daemon handed to something else while nobody had answered - must
3357        // find the same question rather than filing a second one.
3358        let again = approval_gate(&mut state, &pr, "feat: x").await.unwrap();
3359        assert_eq!(again, ApprovalGate::Pending);
3360        let still_one = store
3361            .list()
3362            .into_iter()
3363            .filter(|q| q.run == state.id)
3364            .count();
3365        assert_eq!(
3366            still_one, 1,
3367            "asking twice must not double-file the question"
3368        );
3369    }
3370
3371    #[tokio::test]
3372    async fn approving_the_existing_question_is_read_back_as_approved() {
3373        crate::run::set_home(std::env::temp_dir().join("magi-land-approval-test-home"));
3374        let mut state = run_state();
3375        state.config.graph.land_approval = true;
3376        let pr = green_pr();
3377        assert_eq!(
3378            approval_gate(&mut state, &pr, "feat: x").await.unwrap(),
3379            ApprovalGate::Pending
3380        );
3381
3382        let store = ask::Questions::open();
3383        let mut q = store
3384            .list()
3385            .into_iter()
3386            .find(|q| q.run == state.id)
3387            .expect("filed above");
3388        q.answer(ask::Answer::Choice(APPROVE.to_owned())).unwrap();
3389        store.put(&mut q).unwrap();
3390
3391        assert_eq!(
3392            approval_gate(&mut state, &pr, "feat: x").await.unwrap(),
3393            ApprovalGate::Approved
3394        );
3395    }
3396
3397    #[tokio::test]
3398    async fn holding_or_abandoning_the_existing_question_is_read_back_as_held() {
3399        crate::run::set_home(std::env::temp_dir().join("magi-land-approval-test-home"));
3400        let store = ask::Questions::open();
3401
3402        let mut held_state = run_state();
3403        held_state.config.graph.land_approval = true;
3404        let pr = green_pr();
3405        approval_gate(&mut held_state, &pr, "feat: x")
3406            .await
3407            .unwrap();
3408        let mut q = store
3409            .list()
3410            .into_iter()
3411            .find(|q| q.run == held_state.id)
3412            .expect("filed above");
3413        q.answer(ask::Answer::Choice(HOLD.to_owned())).unwrap();
3414        store.put(&mut q).unwrap();
3415        assert_eq!(
3416            approval_gate(&mut held_state, &pr, "feat: x")
3417                .await
3418                .unwrap(),
3419            ApprovalGate::Held
3420        );
3421
3422        let mut abandoned_state = run_state();
3423        abandoned_state.config.graph.land_approval = true;
3424        approval_gate(&mut abandoned_state, &pr, "feat: x")
3425            .await
3426            .unwrap();
3427        let mut q = store
3428            .list()
3429            .into_iter()
3430            .find(|q| q.run == abandoned_state.id)
3431            .expect("filed above");
3432        q.abandon("no answer within the timeout");
3433        store.put(&mut q).unwrap();
3434        assert_eq!(
3435            approval_gate(&mut abandoned_state, &pr, "feat: x")
3436                .await
3437                .unwrap(),
3438            ApprovalGate::Held,
3439            "silence must never merge"
3440        );
3441    }
3442
3443    #[test]
3444    fn the_diffstat_table_is_ordered_by_churn_with_binaries_last() {
3445        let rows = parse_numstat(NUMSTAT);
3446        assert_eq!(
3447            rows.iter().map(|r| r.path.as_str()).collect::<Vec<_>>(),
3448            ["src/web.rs", "src/land.rs", "assets/logo.png"]
3449        );
3450        assert_eq!(rows[2].added, None, "a binary file has no line counts");
3451    }
3452    #[test]
3453    fn the_approval_speaks_the_language_the_repository_is_configured_for() {
3454        // Reported from a real run: the merge question arrived in English on a
3455        // repository with `language = "ja"`. magi's own strings have to follow
3456        // that setting too - "it is a literal in Rust" is not an answer.
3457        let mut state = run_state();
3458        state.config.graph.language = "ja".to_owned();
3459        let pr = green_pr();
3460        let commits = ["c1".to_owned()];
3461
3462        let ja = approval_panel(&state, &pr, "3\t1\tsrc/a.rs", "+ x", &commits, "feat: x");
3463        assert!(ja.contains("lang=\"ja\""), "the document must declare it");
3464        assert!(ja.contains("squash されるコミット"), "{ja}");
3465        assert!(ja.contains("レビューコメント"), "{ja}");
3466        assert!(ja.contains("差分"), "{ja}");
3467        assert!(
3468            !ja.contains("Commits being squashed"),
3469            "no English left over"
3470        );
3471
3472        let w = words("ja");
3473        assert!(w.approval_summary(17, "feat: x").contains("マージ"));
3474        assert!(
3475            w.approval_detail("http://x/1", "main", "feat: x")
3476                .contains("パネル")
3477        );
3478
3479        // The evidence itself is language-neutral and must survive either way.
3480        assert!(ja.contains("src/a.rs"), "the diffstat is not prose");
3481        assert!(ja.contains("feat: x"), "nor is the merge subject");
3482
3483        // English stays the default, and a language magi cannot check falls
3484        // back to it rather than shipping a guess.
3485        state.config.graph.language = "en".to_owned();
3486        let en = approval_panel(&state, &pr, "3\t1\tsrc/a.rs", "+ x", &commits, "feat: x");
3487        assert!(en.contains("Commits being squashed"), "{en}");
3488        assert_eq!(words("Klingon").html_lang, "en");
3489    }
3490}