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/// Read just a pull request's lifecycle state - open, merged, or closed -
1206/// with none of the checks/reviews/comments [`land`] itself needs to decide
1207/// what to do next.
1208///
1209/// For a caller that only ever wants one fact and must not risk anything
1210/// else: `magi fold --merged` uses this to confirm a URL the operator hands
1211/// it is actually a merged pull request *before* touching a run's state, so a
1212/// typo or a still-open PR fails loudly instead of quietly recording a merge
1213/// that never happened.
1214pub async fn lifecycle(repo: &Path, pr_url: &str) -> Result<PrLifecycle> {
1215    let view = gh(
1216        repo,
1217        &[
1218            "pr".to_owned(),
1219            "view".to_owned(),
1220            pr_url.to_owned(),
1221            "--json".to_owned(),
1222            "state".to_owned(),
1223        ],
1224    )
1225    .await?;
1226    if !view.0 {
1227        bail!("gh pr view {pr_url}: {}", view.1);
1228    }
1229    // `parse_pr` reads every other field of `GhPr` as its serde default
1230    // (empty string, empty vec, zero) when this narrower `--json` selection
1231    // does not carry them - harmless, since only `.state` is read back.
1232    Ok(parse_pr(&view.1)?.state)
1233}
1234
1235/// Parse `gh api repos/{owner}/{repo}/pulls/<n>/comments` into inline review
1236/// comments. No I/O.
1237///
1238/// `gh pr view` does not surface inline comments, and inline is exactly where
1239/// both review bots put their findings - a landing loop that read only the
1240/// top-level thread would never see the thing it is supposed to fix.
1241pub fn parse_inline_comments(json: &str) -> Result<Vec<ReviewComment>> {
1242    let raw: Vec<GhInline> =
1243        serde_json::from_str(json).context("parse `gh api .../pulls/<n>/comments` output")?;
1244    let mut out = Vec::new();
1245    for c in raw {
1246        push_if_outstanding(
1247            &mut out,
1248            ReviewComment {
1249                author: c.user.login,
1250                path: c.path,
1251                line: c.line,
1252                body: c.body,
1253            },
1254        );
1255    }
1256    Ok(out)
1257}
1258
1259/// Keep a comment only when it asks for something.
1260///
1261/// An inline comment always does: it names a file and a line. A top-level
1262/// comment is dropped when it is empty, when it is magi's own, or when it is
1263/// [noise](is_noise).
1264fn push_if_outstanding(out: &mut Vec<ReviewComment>, comment: ReviewComment) {
1265    if comment.body.trim().is_empty() || comment.body.contains(MARKER) {
1266        return;
1267    }
1268    if comment.path.is_none() && is_noise(&comment.body) {
1269        return;
1270    }
1271    out.push(comment);
1272}
1273
1274/// Is this comment body machinery rather than a finding?
1275///
1276/// Two tests, both structural, because guessing from prose is how a "looks
1277/// good to me" turns into a fix round:
1278///
1279/// 1. The bot said so - the body carries one of the [`NOT_A_REVIEW`] markers
1280///    with which CodeRabbit labels its trigger notice, its walkthrough, and its
1281///    footer.
1282/// 2. It asks for nothing - once HTML comments, `<details>` blocks, headings,
1283///    horizontal rules, and the bot's own status banner are removed, every
1284///    remaining line is a task-list item. That is exactly the shape of the
1285///    comment the Claude review job posts while it is still working.
1286///
1287/// Anything else is input, including bot prose. A bot that writes a paragraph
1288/// has said something, and the fix prompt tells the fixer it may decline a
1289/// comment with an argument - a wasted sentence in a prompt is cheaper than a
1290/// missed finding.
1291pub fn is_noise(body: &str) -> bool {
1292    if NOT_A_REVIEW.iter().any(|m| body.contains(m)) {
1293        return true;
1294    }
1295    let mut content = false;
1296    for line in strip_blocks(body).lines() {
1297        let line = unquote(line);
1298        if line.is_empty() || is_checklist(line) || is_decoration(line) || is_banner(line) {
1299            continue;
1300        }
1301        content = true;
1302        break;
1303    }
1304    !content
1305}
1306
1307/// Remove HTML comments and collapsed `<details>` blocks.
1308fn strip_blocks(body: &str) -> String {
1309    let mut out = String::with_capacity(body.len());
1310    let mut rest = body;
1311    loop {
1312        let open = ["<!--", "<details>"]
1313            .iter()
1314            .filter_map(|tag| rest.find(tag).map(|i| (i, *tag)))
1315            .min_by_key(|(i, _)| *i);
1316        let Some((at, tag)) = open else {
1317            out.push_str(rest);
1318            return out;
1319        };
1320        out.push_str(&rest[..at]);
1321        let after = &rest[at + tag.len()..];
1322        let close = if tag == "<!--" { "-->" } else { "</details>" };
1323        match after.find(close) {
1324            Some(end) => rest = &after[end + close.len()..],
1325            // Unterminated: the rest of the body is inside the block.
1326            None => return out,
1327        }
1328    }
1329}
1330
1331/// Strip blockquote markers, which both bots wrap their callouts in.
1332fn unquote(line: &str) -> &str {
1333    let mut s = line.trim();
1334    while let Some(rest) = s.strip_prefix('>') {
1335        s = rest.trim_start();
1336    }
1337    s.trim()
1338}
1339
1340/// `- [ ]` / `- [x]`, in any of the bullet styles GitHub renders.
1341fn is_checklist(line: &str) -> bool {
1342    let rest = line
1343        .strip_prefix("- ")
1344        .or_else(|| line.strip_prefix("* "))
1345        .unwrap_or("");
1346    let rest = rest.trim_start();
1347    matches!(
1348        rest.get(..3),
1349        Some("[ ]") | Some("[x]") | Some("[X]") | Some("[*]")
1350    )
1351}
1352
1353/// A heading, a horizontal rule, or a callout tag - shape, never content.
1354fn is_decoration(line: &str) -> bool {
1355    line.starts_with('#')
1356        || line.starts_with("[!")
1357        || (line.len() >= 3 && line.chars().all(|c| matches!(c, '-' | '=' | '*' | '_')))
1358}
1359
1360/// A line that is nothing but emphasis and links.
1361///
1362/// Both review jobs open with a status banner
1363/// (`**Claude finished ... in 4m 14s** —— [View job](url)`). It reads as prose
1364/// to a line-based test and asks for nothing, so it is measured the same way a
1365/// heading is: strip the markup, and if no word survives, it was decoration.
1366fn is_banner(line: &str) -> bool {
1367    let plain = drop_spans(line, "**", "**");
1368    let plain = if plain.contains("](") {
1369        drop_spans(&plain, "[", ")")
1370    } else {
1371        plain
1372    };
1373    !plain.chars().any(char::is_alphanumeric)
1374}
1375
1376/// Remove every `open` .. `close` span, including the delimiters. An
1377/// unterminated span swallows the rest of the input, which is what a reader
1378/// sees too.
1379fn drop_spans(s: &str, open: &str, close: &str) -> String {
1380    let mut out = String::with_capacity(s.len());
1381    let mut rest = s;
1382    while let Some(at) = rest.find(open) {
1383        out.push_str(&rest[..at]);
1384        let after = &rest[at + open.len()..];
1385        match after.find(close) {
1386            Some(end) => rest = &after[end + close.len()..],
1387            None => return out,
1388        }
1389    }
1390    out.push_str(rest);
1391    out
1392}
1393
1394/// The lock that keeps at most one run per repository actually moving the
1395/// base branch at a time: a rebase push, or `gh pr merge`.
1396///
1397/// Deliberately narrow. Everything else in [`land`]'s loop - watching CI,
1398/// running a fix round in the winner's own worktree, waiting on the owner's
1399/// approval - touches nothing a *different* run in the same repository could
1400/// collide with, and holding a lock across any of that would serialise one
1401/// run's CI wait (up to [`WAIT_CEILING`]) against another run's land-approval
1402/// resume, which is precisely the "must not wait on another task" property
1403/// the daemon's slot-freeing exists to give a resume. Only the two moments
1404/// that actually write to the shared base branch need mutual exclusion, and
1405/// both are brief.
1406///
1407/// One entry per repository, each its own `tokio::sync::Mutex`, so two
1408/// different repositories' runs never wait on each other. The outer
1409/// `std::sync::Mutex` guards only the map itself, held long enough to find or
1410/// insert an entry and clone its `Arc`, never across an `.await`.
1411fn repo_merge_lock(repo: &Path) -> Arc<tokio::sync::Mutex<()>> {
1412    static LOCKS: std::sync::LazyLock<
1413        std::sync::Mutex<BTreeMap<PathBuf, Arc<tokio::sync::Mutex<()>>>>,
1414    > = std::sync::LazyLock::new(|| std::sync::Mutex::new(BTreeMap::new()));
1415    LOCKS
1416        .lock()
1417        .unwrap_or_else(std::sync::PoisonError::into_inner)
1418        .entry(repo.to_path_buf())
1419        .or_insert_with(|| Arc::new(tokio::sync::Mutex::new(())))
1420        .clone()
1421}
1422
1423/// Run the loop against a real pull request until it merges or the budget runs
1424/// out.
1425///
1426/// The caller decides whether landing happens at all: this is only reached when
1427/// `graph.land` is on. Returns the last observation, so the caller can report
1428/// what magi was looking at when it stopped.
1429pub async fn land(state: &mut RunState, pr_url: &str) -> Result<PrState> {
1430    let repo = state.repo.clone();
1431    let budget = state.config.graph.land_rounds;
1432    let mut round = 0usize;
1433    // Counted apart from `round`: a rebase is not a fix, and a base that
1434    // moved is not the change's fault.
1435    let mut rebases = 0usize;
1436    let mut waited = Duration::ZERO;
1437    // Comment bodies the fixer has already been shown. A comment is
1438    // outstanding until it has been handed over once; after that it is a
1439    // recorded decision, not an open question, and re-feeding it would loop the
1440    // budget away on a comment the fixer already declined with an argument.
1441    let mut shown: BTreeSet<String> = BTreeSet::new();
1442
1443    // Marks the run resumable through exactly this function, not through a
1444    // fresh competition: `RunStatus::resumable` excludes only `Merged`,
1445    // `Ready` and `Failed`, and `merge`'s own re-entry guard looks for this
1446    // status specifically to know a resumed run belongs back in `land`
1447    // rather than at a second `gh pr create`. Set on every entry - fresh or
1448    // resumed - because a resume that parked here again must keep reading
1449    // `Landing`, not whatever a first pass through `merge` left behind.
1450    state.status = RunStatus::Landing;
1451    state.event("land", format!("watching {pr_url}"));
1452    state.save()?;
1453
1454    loop {
1455        let seen = observe(&repo, pr_url).await?;
1456        let mut pr = seen.pr;
1457        pr.review_comments.retain(|c| !shown.contains(&c.body));
1458        state.pr = Some(crate::run::PrRecord {
1459            url: pr.url.clone(),
1460            number: pr.number,
1461            state: pr.state.as_str().to_owned(),
1462            checks: pr.checks.as_str().to_owned(),
1463            round,
1464            rounds: budget,
1465        });
1466        state.save()?;
1467
1468        match decide(&pr, round, budget, waited) {
1469            Step::Wait => {
1470                if waited >= WAIT_CEILING {
1471                    let why = format!(
1472                        "checks were still running after {} minutes",
1473                        WAIT_CEILING.as_secs() / 60
1474                    );
1475                    stop(state, &repo, &pr, &why).await?;
1476                    return Ok(pr);
1477                }
1478                waited += POLL;
1479                tokio::time::sleep(POLL).await;
1480            }
1481            Step::Done { merged } => {
1482                state.status = if merged {
1483                    RunStatus::Merged
1484                } else {
1485                    RunStatus::Ready
1486                };
1487                let detail = if merged {
1488                    format!("{} was merged", pr.url)
1489                } else {
1490                    format!("{} was closed without merging", pr.url)
1491                };
1492                state.merge = Some(MergeOutcome {
1493                    mode: MergeMode::Pr,
1494                    ok: merged,
1495                    detail: detail.clone(),
1496                });
1497                state.event("land", detail);
1498                state.save()?;
1499                return Ok(pr);
1500            }
1501            Step::Merge => {
1502                let subject = merge_subject(&seen.title, &state.instruction);
1503                // The owner sees the panel before the one irreversible step,
1504                // and an unanswered question is a hold: silence never merges.
1505                if state.config.graph.land_approval {
1506                    match approval_gate(state, &pr, &subject).await? {
1507                        ApprovalGate::Approved => {}
1508                        ApprovalGate::Held => {
1509                            stop(
1510                                state,
1511                                &repo,
1512                                &pr,
1513                                "the owner did not approve the merge (held or unanswered)",
1514                            )
1515                            .await?;
1516                            return Ok(pr);
1517                        }
1518                        // Filed (or still standing from an earlier visit) and
1519                        // not yet answered. Park here rather than wait: the
1520                        // question survives on disk, the daemon hands this
1521                        // run's slot to something else, and a later resume
1522                        // re-enters `land`, finds the same question, and
1523                        // either merges or stops depending on what it says
1524                        // by then.
1525                        ApprovalGate::Pending => {
1526                            state.parked = true;
1527                            state.event(
1528                                "land",
1529                                "parked awaiting merge approval - resumes once answered",
1530                            );
1531                            state.save()?;
1532                            return Ok(pr);
1533                        }
1534                    }
1535                }
1536                let argv = merge_argv(pr.number, &subject);
1537                let out = {
1538                    let merge_lock = repo_merge_lock(&repo);
1539                    let _merge_slot = merge_lock.lock().await;
1540                    gh(&repo, &argv).await?
1541                };
1542                if out.0 {
1543                    pr.state = PrLifecycle::Merged;
1544                    state.status = RunStatus::Merged;
1545                    state.merge = Some(MergeOutcome {
1546                        mode: MergeMode::Pr,
1547                        ok: true,
1548                        detail: format!("gh {}", argv.join(" ")),
1549                    });
1550                    // The last `state.pr` snapshot is whatever the poll before
1551                    // this merge observed - still `open` - and nothing below
1552                    // refreshes it from GitHub again, so the UI's round rail
1553                    // would otherwise keep animating a merged run forever.
1554                    if let Some(pr_record) = state.pr.as_mut() {
1555                        pr_record.state = pr.state.as_str().to_owned();
1556                    }
1557                    state.event("land", format!("merged {} as `{subject}`", pr.url));
1558                    state.save()?;
1559                    return Ok(pr);
1560                }
1561                let after = observe(&repo, pr_url).await.ok().map(|s| s.pr.state);
1562                if let Some(outcome) = merged_after_all(&argv, &out.1, after) {
1563                    pr.state = PrLifecycle::Merged;
1564                    state.status = RunStatus::Merged;
1565                    state.merge = Some(outcome);
1566                    if let Some(pr_record) = state.pr.as_mut() {
1567                        pr_record.state = pr.state.as_str().to_owned();
1568                    }
1569                    state.event("land", format!("merged {} as `{subject}`", pr.url));
1570                    state.save()?;
1571                    return Ok(pr);
1572                }
1573                stop(
1574                    state,
1575                    &repo,
1576                    &pr,
1577                    &format!("`gh pr merge` failed: {}", out.1),
1578                )
1579                .await?;
1580                return Ok(pr);
1581            }
1582            Step::Rebase => {
1583                // Bounded by the same budget as a fix, because a rebase that
1584                // keeps being needed means the base moves faster than this
1585                // run can land and a person should decide what to do. It
1586                // spends none of that budget: the change is not what is
1587                // wrong.
1588                if rebases >= budget {
1589                    let why = format!(
1590                        "the base moved under this branch {budget} time(s) and it still does \
1591                         not merge; rebasing again would only race it"
1592                    );
1593                    stop(state, &repo, &pr, &why).await?;
1594                    return Ok(pr);
1595                }
1596                rebases += 1;
1597                let Some(branch) = state.winner().map(|w| w.branch.clone()) else {
1598                    stop(
1599                        state,
1600                        &repo,
1601                        &pr,
1602                        "the pull request conflicts and this run has no winning branch to rebase",
1603                    )
1604                    .await?;
1605                    return Ok(pr);
1606                };
1607                let base = state.base_branch.clone();
1608                state.event(
1609                    "land",
1610                    format!("{} no longer merges; rebasing onto {base}", pr.url),
1611                );
1612                state.save()?;
1613
1614                // Onto the base as the *remote* has it: the local ref may be
1615                // behind, and rebasing onto a stale base produces a branch
1616                // that conflicts all over again.
1617                git::fetch(&repo, "origin", &base).await.ok();
1618                let scratch = state.dir().join("rebase");
1619                let onto = format!("origin/{base}");
1620                match git::rebase_branch_in_temp(&repo, &scratch, &branch, &onto).await {
1621                    Ok(None) => {
1622                        let pushed = {
1623                            let merge_lock = repo_merge_lock(&repo);
1624                            let _merge_slot = merge_lock.lock().await;
1625                            git::push_rewritten(&repo, "origin", &branch).await?
1626                        };
1627                        if !pushed.ok() {
1628                            let why = format!(
1629                                "rebased {branch} but could not push it: {}",
1630                                pushed.stderr.trim()
1631                            );
1632                            stop(state, &repo, &pr, &why).await?;
1633                            return Ok(pr);
1634                        }
1635                        state.event("land", format!("rebased {branch} onto {base}"));
1636                        state.save()?;
1637                        // The forge has to re-run its checks against the
1638                        // rebased head before anything else can be decided.
1639                        waited = Duration::ZERO;
1640                        tokio::time::sleep(POLL).await;
1641                    }
1642                    // A conflict is a decision, not a chore.
1643                    Ok(Some(conflict)) => {
1644                        let why = format!(
1645                            "{} conflicts with {base} and the rebase did not apply: {}",
1646                            pr.url,
1647                            conflict.chars().take(600).collect::<String>()
1648                        );
1649                        stop(state, &repo, &pr, &why).await?;
1650                        return Ok(pr);
1651                    }
1652                    Err(e) => {
1653                        let why = format!("could not rebase {branch} onto {base}: {e:#}");
1654                        stop(state, &repo, &pr, &why).await?;
1655                        return Ok(pr);
1656                    }
1657                }
1658            }
1659            Step::GiveUp { reason } => {
1660                stop(state, &repo, &pr, &reason).await?;
1661                return Ok(pr);
1662            }
1663            Step::Fix { reason } => {
1664                round += 1;
1665                waited = Duration::ZERO;
1666                for c in &pr.review_comments {
1667                    shown.insert(c.body.clone());
1668                }
1669                state.event("land", format!("round {round}: {reason}"));
1670                state.save()?;
1671
1672                let logs = failing_logs(&repo, &seen.failing_urls).await;
1673                let was_red = pr.checks == Checks::Red;
1674                match fix_round(state, &pr, round, budget, &reason, &logs).await? {
1675                    Fixed::Committed => {}
1676                    Fixed::Declined if was_red => {
1677                        let why = format!(
1678                            "the fixer produced no commit while {} check(s) were failing \
1679                             ({}); stopping instead of looping on an unchanged tree",
1680                            pr.failing.len(),
1681                            pr.failing.join(", ")
1682                        );
1683                        stop(state, &repo, &pr, &why).await?;
1684                        return Ok(pr);
1685                    }
1686                    // Comment-driven round with no commit: the fixer read the
1687                    // comments and changed nothing, which is a decision it is
1688                    // allowed to make. The comments are recorded as shown, so
1689                    // the next observation sees a clean pull request.
1690                    Fixed::Declined => state.event(
1691                        "land",
1692                        format!("round {round}: fixer declined the comments, nothing committed"),
1693                    ),
1694                    Fixed::Failed(why) => {
1695                        stop(state, &repo, &pr, &format!("the fix round failed: {why}")).await?;
1696                        return Ok(pr);
1697                    }
1698                }
1699                state.save()?;
1700            }
1701        }
1702    }
1703}
1704
1705/// One observation, plus the two things [`PrState`] deliberately does not carry:
1706/// the title (needed for the squash subject) and where the failing checks'
1707/// logs live.
1708struct Seen {
1709    pr: PrState,
1710    title: String,
1711    failing_urls: Vec<(String, String)>,
1712}
1713
1714/// Read the pull request: `gh pr view` for the rollup and the top-level thread,
1715/// `gh api` for the inline review comments `gh pr view` does not report.
1716async fn observe(repo: &Path, pr_url: &str) -> Result<Seen> {
1717    let view = gh(
1718        repo,
1719        &[
1720            "pr".to_owned(),
1721            "view".to_owned(),
1722            pr_url.to_owned(),
1723            "--json".to_owned(),
1724            "url,number,state,title,statusCheckRollup,reviews,comments,mergeStateStatus".to_owned(),
1725        ],
1726    )
1727    .await?;
1728    if !view.0 {
1729        bail!("gh pr view {pr_url}: {}", view.1);
1730    }
1731    let mut pr = parse_pr(&view.1)?;
1732    let raw: GhPr = serde_json::from_str(&view.1).context("re-read pull request json")?;
1733
1734    let inline = gh(
1735        repo,
1736        &[
1737            "api".to_owned(),
1738            format!("repos/{{owner}}/{{repo}}/pulls/{}/comments", pr.number),
1739        ],
1740    )
1741    .await?;
1742    if inline.0 {
1743        match parse_inline_comments(&inline.1) {
1744            Ok(mut comments) => pr.review_comments.append(&mut comments),
1745            // An unreadable inline thread must not end a landing: the rollup
1746            // and the top-level thread are still real signal.
1747            Err(e) => tracing::warn!("inline review comments unreadable: {e}"),
1748        }
1749    } else {
1750        tracing::warn!("gh api pulls/{}/comments: {}", pr.number, inline.1);
1751    }
1752
1753    let failing_urls = raw
1754        .status_check_rollup
1755        .iter()
1756        .filter(|c| c.verdict() == Verdict::Fail)
1757        .filter_map(|c| c.url().map(|u| (c.label(), u.to_owned())))
1758        .collect();
1759
1760    Ok(Seen {
1761        pr,
1762        title: raw.title,
1763        failing_urls,
1764    })
1765}
1766
1767/// What a fix round did.
1768enum Fixed {
1769    /// The fixer committed something.
1770    Committed,
1771    /// The fixer ran and chose to change nothing.
1772    Declined,
1773    /// The fixer could not run, or said nothing usable.
1774    Failed(String),
1775}
1776
1777/// Hand the failures and the comments to the fixer, then commit and push.
1778///
1779/// The fixer works in the winner's own worktree so its commits land on the
1780/// branch the pull request is built from, and it runs with `allow_write` for
1781/// the same reason.
1782async fn fix_round(
1783    state: &mut RunState,
1784    pr: &PrState,
1785    round: usize,
1786    budget: usize,
1787    reason: &str,
1788    logs: &str,
1789) -> Result<Fixed> {
1790    let winner = state
1791        .winner()
1792        .cloned()
1793        .context("landing needs a winning candidate; none is recorded on this run")?;
1794    let roles = state
1795        .config
1796        .resolve_roles()
1797        .context("resolve the roster for the fix round")?;
1798    // Same rule as the review loop: an explicitly configured fixer, otherwise
1799    // the winner's own author continuing its own conversation - the competition
1800    // is over, so its context is pure benefit.
1801    let (spec, seat_key): (AgentSpec, String) = match &roles.fixer {
1802        Some(f) if f.id != winner.agent => (f.clone(), "fix".to_owned()),
1803        _ => (
1804            state
1805                .config
1806                .agent(&winner.agent)
1807                .cloned()
1808                .unwrap_or_else(|_| roles.implementers[winner.index].clone()),
1809            format!("impl-{}", winner.label),
1810        ),
1811    };
1812
1813    let prompt = fix_prompt(state, pr, round, budget, reason, logs);
1814    let mut seat = seat_of(state, &seat_key, &spec.id);
1815    let artifacts = agent::artifacts_dir(&state.dir());
1816    let prompt = if state.config.cache_dir().is_some() {
1817        format!("{prompt}\n\n{}", prompt::build_cache_note("fix"))
1818    } else {
1819        prompt
1820    };
1821    let out = agent::invoke(
1822        &spec,
1823        &mut seat,
1824        &Invocation {
1825            cwd: &winner.worktree,
1826            prompt: &prompt,
1827            timeout: Duration::from_secs(state.config.graph.timeout_fix),
1828            allow_write: true,
1829            sessions: state.config.graph.sessions,
1830            artifacts: &artifacts,
1831            stem: &format!("land-{round}"),
1832            run: &state.id,
1833            node: "land",
1834            cache_dir: state.config.cache_dir().as_deref(),
1835            attachments: &[],
1836        },
1837    )
1838    .await;
1839    state.seats.insert(seat.key.clone(), seat);
1840
1841    match out {
1842        Ok(o) if o.quota_exhausted() => {
1843            return Ok(Fixed::Failed(
1844                "rate limited (quota); the fixer could not run".to_owned(),
1845            ));
1846        }
1847        Ok(o) if !o.usable() => {
1848            return Ok(Fixed::Failed(format!(
1849                "the fixer produced nothing usable (exit {:?}, timed out: {})",
1850                o.exit_code, o.timed_out
1851            )));
1852        }
1853        Ok(_) => {}
1854        Err(e) => return Ok(Fixed::Failed(format!("{e:#}"))),
1855    }
1856
1857    let before = git::rev_parse(&winner.worktree, "HEAD").await?;
1858    // An agent that edited files but never committed would otherwise push
1859    // nothing and look like a refusal.
1860    git::commit_all(
1861        &winner.worktree,
1862        &format!("magi: land round {round} fixes (uncommitted work)"),
1863    )
1864    .await
1865    .ok();
1866    let after = git::rev_parse(&winner.worktree, "HEAD").await?;
1867    if after == before {
1868        return Ok(Fixed::Declined);
1869    }
1870
1871    let remote = state.config.merge.remote.clone();
1872    let push = git::push(&winner.worktree, &remote, &winner.branch).await?;
1873    if !push.ok() {
1874        return Ok(Fixed::Failed(format!(
1875            "pushing {} to {remote} failed: {}",
1876            winner.branch, push.stderr
1877        )));
1878    }
1879    state.event(
1880        "land",
1881        format!("round {round}: pushed a fix to {}", winner.branch),
1882    );
1883    Ok(Fixed::Committed)
1884}
1885
1886/// Fetch or create a seat, keeping its conversation across nodes.
1887fn seat_of(state: &mut RunState, key: &str, agent: &str) -> SeatState {
1888    if let Some(existing) = state.seats.get(key)
1889        && existing.agent == agent
1890    {
1891        return existing.clone();
1892    }
1893    let fresh = SeatState::new(key, agent, state.seed);
1894    state.seats.insert(key.to_owned(), fresh.clone());
1895    fresh
1896}
1897
1898/// What the fixer is told.
1899fn fix_prompt(
1900    state: &RunState,
1901    pr: &PrState,
1902    round: usize,
1903    budget: usize,
1904    reason: &str,
1905    logs: &str,
1906) -> String {
1907    let mut s = format!(
1908        "Your patch is open as a pull request and it is not landing. Land round \
1909         {round} of {budget}.\n\n\
1910         Pull request: {}\n\n\
1911         What is holding it: {reason}\n\n\
1912         # The task\n\n{}\n",
1913        pr.url, state.instruction
1914    );
1915
1916    if pr.failing.is_empty() {
1917        s.push_str("\n# Failing checks\n\n(none)\n");
1918    } else {
1919        let _ = write!(s, "\n# Failing checks\n\n- {}\n", pr.failing.join("\n- "));
1920        if logs.trim().is_empty() {
1921            s.push_str("\nNo log could be read; reproduce the failure locally.\n");
1922        } else {
1923            let _ = write!(s, "\n## Failing log tails\n\n{logs}\n");
1924        }
1925    }
1926
1927    if pr.review_comments.is_empty() {
1928        s.push_str("\n# Review comments\n\n(none)\n");
1929    } else {
1930        s.push_str("\n# Review comments\n");
1931        for c in &pr.review_comments {
1932            let where_ = match (&c.path, c.line) {
1933                (Some(p), Some(l)) => format!(" ({p}:{l})"),
1934                (Some(p), None) => format!(" ({p})"),
1935                _ => String::new(),
1936            };
1937            let _ = write!(s, "\n## {}{where_}\n\n{}\n", c.author, c.body.trim());
1938        }
1939    }
1940
1941    s.push_str(
1942        "\n# Rules\n\n\
1943         1. Fix the cause, never the symptom. Do not delete, skip, or weaken a \
1944            failing test; do not silence a lint with an allow attribute; do not \
1945            stretch a timeout to hide a race. If the check is right, the code is \
1946            wrong.\n\
1947         2. Change nothing the checks and the comments did not raise. A \
1948            drive-by refactor turns a one-line fix into a pull request that \
1949            needs reviewing again.\n\
1950         3. If a comment is wrong, say so with a checkable argument and change \
1951            nothing for it. A declined comment with a reason is a correct \
1952            outcome; a change made to appease a reviewer is not.\n\
1953         4. Commit in this worktree. magi pushes to the pull request's branch \
1954            for you; do not push, merge, or close anything yourself.\n\
1955         5. Never name yourself, your vendor, or your model, anywhere.\n\n\
1956         # Output\n\n\
1957         Say what you changed and why, and what you declined and why.",
1958    );
1959
1960    let language = &state.config.graph.language;
1961    if !(language.trim().is_empty() || language.eq_ignore_ascii_case("en")) {
1962        let _ = write!(s, "\n\nWrite all prose in {language}.");
1963    }
1964    if let Some(overlay) = state.config.prompts.overlay("fix") {
1965        let _ = write!(s, "\n\n{overlay}");
1966    }
1967    s
1968}
1969
1970/// Failing log tails, the way the operator collects them by hand:
1971/// `gh run view --log-failed`.
1972async fn failing_logs(repo: &Path, failing: &[(String, String)]) -> String {
1973    let mut out = String::new();
1974    for (name, url) in failing.iter().take(MAX_LOGS) {
1975        let args = match (job_of(url), run_of(url)) {
1976            (Some(job), _) => vec![
1977                "run".to_owned(),
1978                "view".to_owned(),
1979                "--log-failed".to_owned(),
1980                "--job".to_owned(),
1981                job,
1982            ],
1983            (None, Some(run)) => vec![
1984                "run".to_owned(),
1985                "view".to_owned(),
1986                run,
1987                "--log-failed".to_owned(),
1988            ],
1989            // Not a GitHub Actions check - an external status has no log here.
1990            (None, None) => continue,
1991        };
1992        let (ok, body) = match gh(repo, &args).await {
1993            Ok(v) => v,
1994            Err(e) => (false, format!("{e:#}")),
1995        };
1996        if !ok && body.trim().is_empty() {
1997            continue;
1998        }
1999        let _ = write!(out, "### {name}\n\n```\n{}\n```\n\n", tail(&body, LOG_TAIL));
2000    }
2001    out
2002}
2003
2004/// Job id out of a check's `detailsUrl`
2005/// (`https://github.com/o/r/actions/runs/<run>/job/<job>`).
2006fn job_of(details_url: &str) -> Option<String> {
2007    let after = details_url.split("/job/").nth(1)?;
2008    let id: String = after.chars().take_while(char::is_ascii_digit).collect();
2009    (!id.is_empty()).then_some(id)
2010}
2011
2012/// Workflow run id out of a check's `detailsUrl`.
2013fn run_of(details_url: &str) -> Option<String> {
2014    let after = details_url.split("/actions/runs/").nth(1)?;
2015    let id: String = after.chars().take_while(char::is_ascii_digit).collect();
2016    (!id.is_empty()).then_some(id)
2017}
2018
2019/// Leave the pull request open, say why on it, and mark the run blocked.
2020///
2021/// The comment is what makes an unattended stop actionable: the operator wakes
2022/// up to a pull request that explains itself rather than to a silent queue.
2023async fn stop(state: &mut RunState, repo: &Path, pr: &PrState, why: &str) -> Result<()> {
2024    let body = format!(
2025        "{MARKER}\nmagi stopped landing this pull request: {why}\n\n\
2026         The branch is untouched and the run is `{}`. Nothing was merged.",
2027        state.id
2028    );
2029    let posted = gh(
2030        repo,
2031        &[
2032            "pr".to_owned(),
2033            "comment".to_owned(),
2034            pr.number.to_string(),
2035            "--body".to_owned(),
2036            body,
2037        ],
2038    )
2039    .await;
2040    match posted {
2041        Ok((true, _)) => {}
2042        Ok((false, out)) => tracing::warn!("could not comment on {}: {out}", pr.url),
2043        Err(e) => tracing::warn!("could not comment on {}: {e:#}", pr.url),
2044    }
2045    state.status = RunStatus::Blocked;
2046    state.merge = Some(MergeOutcome {
2047        mode: MergeMode::Pr,
2048        ok: false,
2049        detail: why.to_owned(),
2050    });
2051    state.event("land", format!("stopped: {why}"));
2052    state.save()?;
2053    Ok(())
2054}
2055
2056/// Run `gh` in `repo`, returning success and the combined output.
2057///
2058/// Combined because `gh` reports a refused merge on stderr and the pull request
2059/// json on stdout, and both are evidence.
2060async fn gh(cwd: &Path, args: &[String]) -> Result<(bool, String)> {
2061    let out = tokio::process::Command::new("gh")
2062        .args(args)
2063        .current_dir(cwd)
2064        .quiet()
2065        .stdin(std::process::Stdio::null())
2066        .output()
2067        .await
2068        .with_context(|| format!("spawn gh {}", args.join(" ")))?;
2069    let mut body = String::from_utf8_lossy(&out.stdout).into_owned();
2070    let err = String::from_utf8_lossy(&out.stderr);
2071    if body.trim().is_empty() {
2072        body = err.into_owned();
2073    } else if !err.trim().is_empty() {
2074        body.push_str(&err);
2075    }
2076    Ok((out.status.success(), body.trim().to_owned()))
2077}
2078
2079/// Verdict of one entry in the status rollup.
2080#[derive(Debug, Clone, Copy, PartialEq, Eq)]
2081enum Verdict {
2082    Pass,
2083    Fail,
2084    Pending,
2085    Unknown,
2086}
2087
2088#[derive(Debug, Deserialize)]
2089#[serde(rename_all = "camelCase")]
2090struct GhPr {
2091    #[serde(default)]
2092    url: String,
2093    #[serde(default)]
2094    number: u64,
2095    #[serde(default)]
2096    state: String,
2097    #[serde(default)]
2098    title: String,
2099    #[serde(default)]
2100    status_check_rollup: Vec<GhCheck>,
2101    /// GitHub's own verdict on whether the pull request can be merged.
2102    ///
2103    /// Worth asking for because it is the only place the *required* check set
2104    /// is applied: the rollup lists every check equally, so a repository that
2105    /// deliberately does not require `coverage` still looks red here. See
2106    /// [`Blocking`].
2107    #[serde(default)]
2108    merge_state_status: String,
2109    #[serde(default)]
2110    reviews: Vec<GhReview>,
2111    #[serde(default)]
2112    comments: Vec<GhComment>,
2113}
2114
2115/// One rollup entry. `gh` mixes two GraphQL types in this array: a `CheckRun`
2116/// has `name`/`status`/`conclusion`, while a `StatusContext` - the old commit
2117/// status API, which is how CodeRabbit reports - has `context`/`state` and no
2118/// conclusion at all.
2119#[derive(Debug, Deserialize)]
2120#[serde(rename_all = "camelCase")]
2121struct GhCheck {
2122    #[serde(default)]
2123    name: Option<String>,
2124    #[serde(default)]
2125    context: Option<String>,
2126    #[serde(default)]
2127    status: Option<String>,
2128    #[serde(default)]
2129    conclusion: Option<String>,
2130    #[serde(default)]
2131    state: Option<String>,
2132    #[serde(default)]
2133    details_url: Option<String>,
2134    #[serde(default)]
2135    target_url: Option<String>,
2136}
2137
2138impl GhCheck {
2139    /// Name to show a human and hand to the fixer.
2140    fn label(&self) -> String {
2141        self.name
2142            .clone()
2143            .or_else(|| self.context.clone())
2144            .unwrap_or_else(|| "(unnamed check)".to_owned())
2145    }
2146
2147    /// Where this check's logs live, when it has any.
2148    fn url(&self) -> Option<&str> {
2149        self.details_url
2150            .as_deref()
2151            .or(self.target_url.as_deref())
2152            .filter(|u| !u.is_empty())
2153    }
2154
2155    /// Did it pass?
2156    ///
2157    /// `SKIPPED` and `NEUTRAL` count as passed: the Claude review workflow
2158    /// skips release and bot pull requests by design, and a skip that blocked
2159    /// landing would block exactly the pull requests that need no review.
2160    /// `CANCELLED` counts as failed - a cancelled check did not pass, and
2161    /// merging over one is merging over a check that never ran.
2162    fn verdict(&self) -> Verdict {
2163        if let Some(status) = self.status.as_deref() {
2164            if !status.eq_ignore_ascii_case("COMPLETED") {
2165                return Verdict::Pending;
2166            }
2167        }
2168        let outcome = self
2169            .conclusion
2170            .as_deref()
2171            .or(self.state.as_deref())
2172            .unwrap_or("");
2173        match outcome.to_ascii_uppercase().as_str() {
2174            "SUCCESS" | "SKIPPED" | "NEUTRAL" => Verdict::Pass,
2175            "FAILURE" | "ERROR" | "TIMED_OUT" | "CANCELLED" | "STARTUP_FAILURE"
2176            | "ACTION_REQUIRED" => Verdict::Fail,
2177            "PENDING" | "EXPECTED" | "QUEUED" | "IN_PROGRESS" | "WAITING" | "REQUESTED" => {
2178                Verdict::Pending
2179            }
2180            _ => Verdict::Unknown,
2181        }
2182    }
2183}
2184
2185#[derive(Debug, Deserialize)]
2186struct GhAuthor {
2187    #[serde(default)]
2188    login: String,
2189}
2190
2191#[derive(Debug, Deserialize)]
2192struct GhReview {
2193    #[serde(default)]
2194    author: GhAuthor,
2195    #[serde(default)]
2196    body: String,
2197}
2198
2199#[derive(Debug, Deserialize)]
2200struct GhComment {
2201    #[serde(default)]
2202    author: GhAuthor,
2203    #[serde(default)]
2204    body: String,
2205}
2206
2207#[derive(Debug, Deserialize)]
2208struct GhUser {
2209    #[serde(default)]
2210    login: String,
2211}
2212
2213#[derive(Debug, Deserialize)]
2214struct GhInline {
2215    #[serde(default)]
2216    user: GhUser,
2217    #[serde(default)]
2218    path: Option<String>,
2219    #[serde(default)]
2220    line: Option<u64>,
2221    #[serde(default)]
2222    body: String,
2223}
2224
2225impl Default for GhAuthor {
2226    fn default() -> Self {
2227        Self {
2228            login: "(unknown)".to_owned(),
2229        }
2230    }
2231}
2232
2233impl Default for GhUser {
2234    fn default() -> Self {
2235        Self {
2236            login: "(unknown)".to_owned(),
2237        }
2238    }
2239}
2240
2241#[cfg(test)]
2242mod tests {
2243    use super::*;
2244    use crate::run::{Candidate, ReviewRecord, ReviewRound, Tally};
2245
2246    /// 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.
2247    const GREEN_OPEN: &str = r####"{
2248  "url": "https://github.com/yukimemi/magi/pull/10",
2249  "number": 10,
2250  "state": "OPEN",
2251  "mergeStateStatus": "CLEAN",
2252  "statusCheckRollup": [
2253    {
2254      "__typename": "CheckRun",
2255      "conclusion": "SKIPPED",
2256      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33356278334/job/99378963755",
2257      "name": "review",
2258      "status": "COMPLETED",
2259      "workflowName": "claude-review"
2260    },
2261    {
2262      "__typename": "CheckRun",
2263      "conclusion": "SUCCESS",
2264      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33356278338/job/99378963144",
2265      "name": "check (ubuntu-latest)",
2266      "status": "COMPLETED",
2267      "workflowName": "CI"
2268    },
2269    {
2270      "__typename": "CheckRun",
2271      "conclusion": "SUCCESS",
2272      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33356278338/job/99378963095",
2273      "name": "rustfmt",
2274      "status": "COMPLETED",
2275      "workflowName": "CI"
2276    },
2277    {
2278      "__typename": "StatusContext",
2279      "context": "CodeRabbit",
2280      "state": "SUCCESS",
2281      "targetUrl": ""
2282    }
2283  ],
2284  "reviews": [],
2285  "comments": [
2286    {
2287      "author": {
2288        "login": "coderabbitai"
2289      },
2290      "authorAssociation": "NONE",
2291      "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"
2292    }
2293  ]
2294}"####;
2295
2296    /// Real output for the open pull request #9 (the daily kata-apply), whose `editorconfig` check failed while everything else passed.
2297    const RED_OPEN: &str = r####"{
2298  "url": "https://github.com/yukimemi/magi/pull/9",
2299  "number": 9,
2300  "state": "OPEN",
2301  "mergeStateStatus": "UNSTABLE",
2302  "statusCheckRollup": [
2303    {
2304      "__typename": "CheckRun",
2305      "conclusion": "SUCCESS",
2306      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323744",
2307      "name": "check (ubuntu-latest)",
2308      "status": "COMPLETED",
2309      "workflowName": "CI"
2310    },
2311    {
2312      "__typename": "CheckRun",
2313      "conclusion": "SUCCESS",
2314      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323811",
2315      "name": "rustfmt",
2316      "status": "COMPLETED",
2317      "workflowName": "CI"
2318    },
2319    {
2320      "__typename": "CheckRun",
2321      "conclusion": "FAILURE",
2322      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323572",
2323      "name": "editorconfig",
2324      "status": "COMPLETED",
2325      "workflowName": "CI"
2326    },
2327    {
2328      "__typename": "StatusContext",
2329      "context": "CodeRabbit",
2330      "state": "SUCCESS",
2331      "targetUrl": ""
2332    }
2333  ],
2334  "reviews": [],
2335  "comments": [
2336    {
2337      "author": {
2338        "login": "coderabbitai"
2339      },
2340      "authorAssociation": "NONE",
2341      "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"
2342    }
2343  ]
2344}"####;
2345
2346    /// 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.
2347    const PENDING_OPEN: &str = r####"{
2348  "url": "https://github.com/yukimemi/magi/pull/9",
2349  "number": 9,
2350  "state": "OPEN",
2351  "statusCheckRollup": [
2352    {
2353      "__typename": "CheckRun",
2354      "conclusion": "SUCCESS",
2355      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323744",
2356      "name": "check (ubuntu-latest)",
2357      "status": "COMPLETED",
2358      "workflowName": "CI"
2359    },
2360    {
2361      "__typename": "CheckRun",
2362      "conclusion": "SUCCESS",
2363      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323811",
2364      "name": "rustfmt",
2365      "status": "COMPLETED",
2366      "workflowName": "CI"
2367    },
2368    {
2369      "__typename": "CheckRun",
2370      "conclusion": null,
2371      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323572",
2372      "name": "editorconfig",
2373      "status": "IN_PROGRESS",
2374      "workflowName": "CI"
2375    },
2376    {
2377      "__typename": "StatusContext",
2378      "context": "CodeRabbit",
2379      "state": "SUCCESS",
2380      "targetUrl": ""
2381    }
2382  ],
2383  "reviews": [],
2384  "comments": []
2385}"####;
2386
2387    /// Real output for pull request #16 after it was merged - the shape landing sees when a person merged underneath it.
2388    const MERGED: &str = r####"{
2389  "url": "https://github.com/yukimemi/magi/pull/16",
2390  "number": 16,
2391  "state": "MERGED",
2392  "statusCheckRollup": [
2393    {
2394      "__typename": "CheckRun",
2395      "conclusion": "SUCCESS",
2396      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33636587933/job/100268878095",
2397      "name": "check (ubuntu-latest)",
2398      "status": "COMPLETED",
2399      "workflowName": "CI"
2400    },
2401    {
2402      "__typename": "CheckRun",
2403      "conclusion": "SUCCESS",
2404      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33636587918/job/100268876427",
2405      "name": "review",
2406      "status": "COMPLETED",
2407      "workflowName": "claude-review"
2408    }
2409  ],
2410  "reviews": [],
2411  "comments": []
2412}"####;
2413
2414    /// 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.
2415    const REVIEWED_OPEN: &str = r####"{
2416  "url": "https://github.com/yukimemi/magi/pull/12",
2417  "number": 12,
2418  "state": "OPEN",
2419  "statusCheckRollup": [
2420    {
2421      "__typename": "CheckRun",
2422      "conclusion": "SUCCESS",
2423      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33571212506/job/100065355258",
2424      "name": "check (ubuntu-latest)",
2425      "status": "COMPLETED",
2426      "workflowName": "CI"
2427    },
2428    {
2429      "__typename": "CheckRun",
2430      "conclusion": "SUCCESS",
2431      "detailsUrl": "https://github.com/yukimemi/magi/actions/runs/33571212566/job/100065355810",
2432      "name": "review",
2433      "status": "COMPLETED",
2434      "workflowName": "claude-review"
2435    }
2436  ],
2437  "reviews": [
2438    {
2439      "author": {
2440        "login": "claude"
2441      },
2442      "state": "COMMENTED",
2443      "body": ""
2444    }
2445  ],
2446  "comments": [
2447    {
2448      "author": {
2449        "login": "coderabbitai"
2450      },
2451      "authorAssociation": "NONE",
2452      "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"
2453    },
2454    {
2455      "author": {
2456        "login": "claude"
2457      },
2458      "authorAssociation": "NONE",
2459      "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"
2460    }
2461  ]
2462}"####;
2463
2464    /// Real `gh api repos/{owner}/{repo}/pulls/12/comments` output: one inline finding with its file and line.
2465    const INLINE: &str = r####"[
2466  {
2467    "user": {
2468      "login": "claude[bot]"
2469    },
2470    "path": "src/graph.rs",
2471    "line": 231,
2472    "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"
2473  }
2474]"####;
2475
2476    /// CodeRabbit's real trigger notice: a checkbox, a `<details>` block, and its own "skip review" marker.
2477    const CODERABBIT_TRIGGER: &str = r####"<!-- This is an auto-generated comment: summarize by coderabbit.ai -->
2478<!-- This is an auto-generated comment: skip review by coderabbit.ai -->
2479
2480> [!IMPORTANT]
2481> - [ ] <!-- {"checkboxId":"e9bb8d72-00e8-4f67-9cb2-caf3b22574fe"} --> 🔍 Trigger review
2482> 
2483> This repository does not receive automatic reviews because it has fewer than 10 stars.
2484> 
2485> <details>
2486> <summary>⚙️ Run configuration</summary>
2487> 
2488> **Configuration used**: defaults
2489> 
2490> **Review profile**: CHILL
2491> 
2492> **Plan**: Team
2493> 
2494> **Run ID**: `c1e2a68f-87fc-4b35-9ec4-e75c7854966a`
2495> 
2496> </details>
2497
2498<!-- end of auto-generated comment: skip review by coderabbit.ai -->
2499
2500<!-- tips_start -->
2501
2502---
2503
2504Thanks 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.
2505
2506<details>
2507<summary>❤️ Share</summary>
2508
2509- [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"####;
2510
2511    /// 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.
2512    const CLAUDE_CHECKLIST: &str = r####"**Claude finished @yukimemi's task in 4m 14s** —— [View job](https://github.com/yukimemi/magi/actions/runs/33636587918)
2513
2514---
2515### Reviewing PR #16
2516
2517- [x] Read AGENTS.md conventions
2518- [x] Review `src/daemon.rs` changes
2519- [x] Review `src/main.rs` changes (new `doctor` reporting)
2520- [x] Review `src/web.rs` changes (reuse of unreadable-run count)
2521- [x] Check test coverage for new behavior
2522- [x] Run verification commands (blocked — see note)
2523- [x] Post findings"####;
2524
2525    /// The same job's real comment on pull request #12 once it had something to say.
2526    const CLAUDE_FINDING: &str = r####"**Claude finished @yukimemi's task in 3m 52s** —— [View job](https://github.com/yukimemi/magi/actions/runs/33571212566)
2527
2528---
2529### Review: `magi review <branch>` — cheap-half-only graph
2530
2531Read 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.
2532
2533**Correctness**
2534
2535- 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"####;
2536
2537    fn pr(checks: Checks, failing: &[&str], comments: usize) -> PrState {
2538        PrState {
2539            url: "https://github.com/yukimemi/magi/pull/16".to_owned(),
2540            number: 16,
2541            state: PrLifecycle::Open,
2542            checks,
2543            // These tests are about red-means-fix, so a red here is one the
2544            // forge gates on. Without saying so they would assert the new
2545            // "merge past a check nobody requires" path by accident.
2546            blocking: if matches!(checks, Checks::Red) {
2547                Blocking::Yes
2548            } else {
2549                Blocking::No
2550            },
2551            failing: failing.iter().map(|s| (*s).to_owned()).collect(),
2552            review_comments: (0..comments)
2553                .map(|i| ReviewComment {
2554                    author: "coderabbitai".to_owned(),
2555                    path: Some("src/graph.rs".to_owned()),
2556                    line: Some(231),
2557                    body: format!("finding {i}"),
2558                })
2559                .collect(),
2560        }
2561    }
2562
2563    #[test]
2564    fn a_green_pull_request_with_nothing_outstanding_parses_as_ready_to_merge() {
2565        let state = parse_pr(GREEN_OPEN).expect("green fixture parses");
2566        assert_eq!(state.number, 10);
2567        assert_eq!(state.state, PrLifecycle::Open);
2568        assert_eq!(state.checks, Checks::Green);
2569        assert!(state.failing.is_empty());
2570        assert!(
2571            state.review_comments.is_empty(),
2572            "the only comment is CodeRabbit's trigger notice: {:?}",
2573            state.review_comments
2574        );
2575        assert_eq!(decide(&state, 0, 4, Duration::ZERO), Step::Merge);
2576    }
2577
2578    #[test]
2579    fn a_failing_check_parses_as_red_and_is_named() {
2580        let state = parse_pr(RED_OPEN).expect("red fixture parses");
2581        assert_eq!(state.checks, Checks::Red);
2582        assert_eq!(state.failing, vec!["editorconfig".to_owned()]);
2583        // The captured payload says `UNSTABLE` - mergeable, with a check
2584        // nobody requires red - which is exactly the shape that had to be
2585        // merged by hand. Asserted separately, in
2586        // `a_red_check_nobody_requires_does_not_buy_a_fix_round`. What this
2587        // test is about is that a red check is *named*, so the reason a fixer
2588        // is handed says which one; so it asks the blocking question here.
2589        let mut blocking = state.clone();
2590        blocking.blocking = Blocking::Yes;
2591        match decide(&blocking, 0, 4, Duration::ZERO) {
2592            Step::Fix { reason } => {
2593                assert!(reason.contains("editorconfig"), "reason: {reason}");
2594                assert!(reason.contains("failing"), "reason: {reason}");
2595            }
2596            other => panic!("expected a fix round, got {other:?}"),
2597        }
2598    }
2599
2600    #[test]
2601    fn a_check_still_running_parses_as_pending_and_is_waited_for() {
2602        let state = parse_pr(PENDING_OPEN).expect("pending fixture parses");
2603        assert_eq!(state.checks, Checks::Pending);
2604        assert_eq!(decide(&state, 0, 4, Duration::ZERO), Step::Wait);
2605    }
2606
2607    #[test]
2608    fn a_pull_request_merged_underneath_us_is_done_rather_than_a_failure() {
2609        let state = parse_pr(MERGED).expect("merged fixture parses");
2610        assert_eq!(state.state, PrLifecycle::Merged);
2611        assert_eq!(
2612            decide(&state, 0, 4, Duration::ZERO),
2613            Step::Done { merged: true }
2614        );
2615    }
2616
2617    #[test]
2618    fn a_review_that_found_something_is_outstanding_and_holds_the_merge() {
2619        let state = parse_pr(REVIEWED_OPEN).expect("reviewed fixture parses");
2620        assert_eq!(state.checks, Checks::Green);
2621        let authors: Vec<&str> = state
2622            .review_comments
2623            .iter()
2624            .map(|c| c.author.as_str())
2625            .collect();
2626        assert_eq!(
2627            authors,
2628            vec!["claude"],
2629            "CodeRabbit's walkthrough is machinery; Claude's review is a finding"
2630        );
2631        match decide(&state, 0, 4, Duration::ZERO) {
2632            Step::Fix { reason } => assert!(reason.contains("unresolved"), "reason: {reason}"),
2633            other => panic!("expected a fix round, got {other:?}"),
2634        }
2635    }
2636
2637    #[test]
2638    fn inline_review_comments_keep_their_file_and_line() {
2639        let comments = parse_inline_comments(INLINE).expect("inline fixture parses");
2640        assert_eq!(comments.len(), 1);
2641        assert_eq!(comments[0].author, "claude[bot]");
2642        assert_eq!(comments[0].path.as_deref(), Some("src/graph.rs"));
2643        assert_eq!(comments[0].line, Some(231));
2644        assert!(comments[0].body.contains("empty"), "{}", comments[0].body);
2645    }
2646
2647    #[test]
2648    fn a_status_only_bot_comment_does_not_trigger_a_fix_round() {
2649        assert!(
2650            is_noise(CODERABBIT_TRIGGER),
2651            "CodeRabbit's trigger notice declares itself not a review"
2652        );
2653        assert!(
2654            is_noise(CLAUDE_CHECKLIST),
2655            "a progress checklist asks for nothing"
2656        );
2657        assert!(
2658            !is_noise(CLAUDE_FINDING),
2659            "a review that names a bug is input, not noise"
2660        );
2661
2662        let mut clean = pr(Checks::Green, &[], 0);
2663        clean.review_comments.push(ReviewComment {
2664            author: "coderabbitai".to_owned(),
2665            path: None,
2666            line: None,
2667            body: CODERABBIT_TRIGGER.to_owned(),
2668        });
2669        clean.review_comments.retain(|c| !is_noise(&c.body));
2670        assert_eq!(decide(&clean, 0, 4, Duration::ZERO), Step::Merge);
2671
2672        let mut found = pr(Checks::Green, &[], 0);
2673        found.review_comments.push(ReviewComment {
2674            author: "claude".to_owned(),
2675            path: None,
2676            line: None,
2677            body: CLAUDE_FINDING.to_owned(),
2678        });
2679        found.review_comments.retain(|c| !is_noise(&c.body));
2680        assert!(matches!(
2681            decide(&found, 0, 4, Duration::ZERO),
2682            Step::Fix { .. }
2683        ));
2684    }
2685
2686    #[test]
2687    fn the_policy_table_holds_for_every_combination_that_matters() {
2688        let cases: Vec<(&str, PrState, usize, usize, Duration, Step)> = vec![
2689            (
2690                "pending checks are waited for, even on the last round",
2691                pr(Checks::Pending, &[], 0),
2692                4,
2693                4,
2694                Duration::ZERO,
2695                Step::Wait,
2696            ),
2697            (
2698                "red checks are fixed",
2699                pr(Checks::Red, &["editorconfig"], 0),
2700                0,
2701                4,
2702                Duration::ZERO,
2703                Step::Fix {
2704                    reason: "1 check(s) failing: editorconfig".to_owned(),
2705                },
2706            ),
2707            (
2708                "green with comments is fixed, not merged",
2709                pr(Checks::Green, &[], 2),
2710                1,
2711                4,
2712                Duration::ZERO,
2713                Step::Fix {
2714                    reason: "checks are green but 2 review comment(s) are unresolved: coderabbitai"
2715                        .to_owned(),
2716                },
2717            ),
2718            (
2719                "green and clean merges",
2720                pr(Checks::Green, &[], 0),
2721                3,
2722                4,
2723                Duration::ZERO,
2724                Step::Merge,
2725            ),
2726            (
2727                "an unreadable rollup is waited on while the grace lasts",
2728                pr(Checks::Unknown, &[], 0),
2729                0,
2730                4,
2731                Duration::ZERO,
2732                Step::Wait,
2733            ),
2734            (
2735                "an unreadable rollup is never merged once the grace is spent",
2736                pr(Checks::Unknown, &[], 0),
2737                0,
2738                4,
2739                CHECKS_GRACE,
2740                Step::GiveUp {
2741                    reason: "no check status is readable on the pull request after 3 minute(s); \
2742                             refusing to merge on a guess"
2743                        .to_owned(),
2744                },
2745            ),
2746        ];
2747        for (what, state, round, budget, waited, want) in cases {
2748            assert_eq!(decide(&state, round, budget, waited), want, "{what}");
2749        }
2750    }
2751
2752    #[test]
2753    fn the_forge_verdict_survives_the_round_trip_from_gh() {
2754        // Read off `gh pr view --json ...,mergeStateStatus`, because a field
2755        // requested but never parsed is the kind of thing that looks wired up
2756        // and answers `Unsaid` forever.
2757        let green = parse_pr(GREEN_OPEN).expect("parse");
2758        assert_eq!(green.blocking, Blocking::No);
2759        let red = parse_pr(RED_OPEN).expect("parse");
2760        assert_eq!(
2761            red.blocking,
2762            Blocking::No,
2763            "`UNSTABLE` is mergeable: the red check is one nobody requires"
2764        );
2765        assert_eq!(red.checks, Checks::Red, "and it is still reported as red");
2766        // A payload from an older `gh` has no such field at all.
2767        let quiet =
2768            parse_pr(&GREEN_OPEN.replace("\"mergeStateStatus\": \"CLEAN\",", "")).expect("parse");
2769        assert_eq!(quiet.blocking, Blocking::Unsaid);
2770    }
2771
2772    #[test]
2773    fn a_red_check_nobody_requires_does_not_buy_a_fix_round() {
2774        // Pull request 37's only red check was `editorconfig`, failing
2775        // because the action could not fetch its own binary after
2776        // editorconfig-checker v4 renamed its release assets. The repository
2777        // does not require it. magi answered by asking a fixer to repair a
2778        // change that was fine, and the pull request had to be merged by hand.
2779        let mut nonblocking = pr(Checks::Red, &["editorconfig", "coverage"], 0);
2780        nonblocking.blocking = Blocking::No;
2781        assert_eq!(
2782            decide(&nonblocking, 0, 4, Duration::ZERO),
2783            Step::Merge,
2784            "the forge says nothing is in the way, so nothing is"
2785        );
2786
2787        // The same red, gated on: that is a fix round, as before.
2788        let mut blocking = pr(Checks::Red, &["test (ubuntu-latest)"], 0);
2789        blocking.blocking = Blocking::Yes;
2790        assert!(matches!(
2791            decide(&blocking, 0, 4, Duration::ZERO),
2792            Step::Fix { .. }
2793        ));
2794
2795        // A review comment still outranks green-enough: a non-required red
2796        // must not become a way to merge past an unanswered reviewer.
2797        let mut commented = pr(Checks::Red, &["coverage"], 1);
2798        commented.blocking = Blocking::No;
2799        assert!(matches!(
2800            decide(&commented, 0, 4, Duration::ZERO),
2801            Step::Fix { .. }
2802        ));
2803
2804        // And silence from the forge is not consent.
2805        let mut unsaid = pr(Checks::Red, &["coverage"], 0);
2806        unsaid.blocking = Blocking::Unsaid;
2807        assert!(matches!(
2808            decide(&unsaid, 0, 4, Duration::ZERO),
2809            Step::Fix { .. }
2810        ));
2811    }
2812
2813    #[test]
2814    fn a_branch_the_base_moved_under_is_rebased_not_fixed() {
2815        // Pull requests 35 and 37 were both rebased by hand: a competition
2816        // that runs for two hours against a repository merging pull requests
2817        // all day conflicts on the way in, and that is arithmetic rather
2818        // than a defect in the change.
2819        let mut conflicted = pr(Checks::Green, &[], 0);
2820        conflicted.blocking = Blocking::Conflict;
2821        assert_eq!(decide(&conflicted, 0, 4, Duration::ZERO), Step::Rebase);
2822
2823        // Decided before the checks, and even with the rounds spent: every
2824        // check on a branch that cannot land is an answer about a state that
2825        // cannot land, and a conflict is not the change's fault.
2826        let mut red = pr(Checks::Red, &["test (ubuntu-latest)"], 2);
2827        red.blocking = Blocking::Conflict;
2828        assert_eq!(decide(&red, 4, 4, Duration::ZERO), Step::Rebase);
2829
2830        // The lifecycle still wins over everything, conflict included.
2831        let mut merged = pr(Checks::Red, &[], 0);
2832        merged.blocking = Blocking::Conflict;
2833        merged.state = PrLifecycle::Merged;
2834        assert_eq!(
2835            decide(&merged, 0, 4, Duration::ZERO),
2836            Step::Done { merged: true }
2837        );
2838    }
2839
2840    #[test]
2841    fn the_forge_verdict_is_read_off_merge_state_status() {
2842        // The spellings that mean "mergeable". `UNSTABLE` is the one that
2843        // matters: mergeable, with a non-required check red or still running.
2844        for ok in ["CLEAN", "UNSTABLE", "unstable", "HAS_HOOKS"] {
2845            assert_eq!(Blocking::of(ok), Blocking::No, "{ok}");
2846            assert!(!Blocking::of(ok).stops_a_merge(), "{ok}");
2847        }
2848        assert_eq!(Blocking::of("DIRTY"), Blocking::Conflict);
2849        assert_eq!(Blocking::of("BLOCKED"), Blocking::Yes);
2850        assert_eq!(Blocking::of("BEHIND"), Blocking::Yes);
2851        // An older `gh`, or a token without the scope, says nothing - and
2852        // refusing to guess is the rule everywhere else in this module.
2853        for quiet in ["", "UNKNOWN"] {
2854            assert_eq!(Blocking::of(quiet), Blocking::Unsaid);
2855            assert!(Blocking::of(quiet).stops_a_merge());
2856        }
2857    }
2858
2859    #[test]
2860    fn a_merge_command_that_failed_after_merging_is_still_a_merge() {
2861        let argv = merge_argv(28, "fix: retry uploads on transient network errors");
2862        // The exact stderr from run ec12, in a jj-colocated repository.
2863        let jj = "could not determine current branch: failed to run git: not on any branch";
2864
2865        let landed = merged_after_all(&argv, jj, Some(PrLifecycle::Merged))
2866            .expect("the forge says merged, so it merged");
2867        assert!(landed.ok);
2868        assert!(
2869            landed.detail.contains("but the pull request is merged"),
2870            "the record must not read as a clean success: {}",
2871            landed.detail
2872        );
2873        assert!(
2874            landed.detail.contains("not on any branch"),
2875            "and it must keep what the command actually said: {}",
2876            landed.detail
2877        );
2878
2879        // A pull request still open means the merge really failed.
2880        assert!(merged_after_all(&argv, jj, Some(PrLifecycle::Open)).is_none());
2881        assert!(merged_after_all(&argv, jj, Some(PrLifecycle::Closed)).is_none());
2882        // And an unreadable answer is not evidence of success.
2883        assert!(merged_after_all(&argv, jj, None).is_none());
2884    }
2885
2886    #[test]
2887    fn a_pull_request_closed_underneath_us_is_done_and_not_merged() {
2888        let mut state = pr(Checks::Red, &["editorconfig"], 3);
2889        state.state = PrLifecycle::Closed;
2890        assert_eq!(
2891            decide(&state, 0, 4, Duration::ZERO),
2892            Step::Done { merged: false },
2893            "a human closing the pull request ends the loop, whatever CI says"
2894        );
2895    }
2896
2897    #[test]
2898    fn the_last_round_gives_up_with_a_reason_naming_what_is_still_failing() {
2899        let red = decide(
2900            &pr(Checks::Red, &["editorconfig", "test (macos)"], 0),
2901            4,
2902            4,
2903            Duration::ZERO,
2904        );
2905        match red {
2906            Step::GiveUp { reason } => {
2907                assert!(reason.contains("editorconfig"), "reason: {reason}");
2908                assert!(reason.contains("test (macos)"), "reason: {reason}");
2909                assert!(reason.contains("4 fix round(s)"), "reason: {reason}");
2910            }
2911            other => panic!("expected a give-up, got {other:?}"),
2912        }
2913
2914        let commented = decide(&pr(Checks::Green, &[], 1), 2, 2, Duration::ZERO);
2915        match commented {
2916            Step::GiveUp { reason } => {
2917                assert!(reason.contains("unresolved"), "reason: {reason}");
2918                assert!(reason.contains("2 fix round(s)"), "reason: {reason}");
2919            }
2920            other => panic!("expected a give-up, got {other:?}"),
2921        }
2922    }
2923
2924    #[test]
2925    fn the_merge_command_squashes_deletes_the_branch_and_sets_its_own_subject() {
2926        let candidate_commit = "magi: candidate A (uncommitted work)";
2927        let subject = merge_subject(candidate_commit, "add retries to the uploader");
2928        let argv = merge_argv(16, &subject);
2929
2930        assert!(argv.contains(&"--squash".to_owned()));
2931        assert!(argv.contains(&"--delete-branch".to_owned()));
2932        assert!(argv.contains(&"--subject".to_owned()));
2933        assert_eq!(
2934            argv.last().map(String::as_str),
2935            Some("add retries to the uploader"),
2936            "the subject must not be the candidate commit message"
2937        );
2938        assert_ne!(subject, candidate_commit);
2939    }
2940
2941    #[test]
2942    fn a_real_pull_request_title_is_used_as_the_squash_subject_verbatim() {
2943        assert_eq!(
2944            merge_subject("feat: a queue, an unattended loop, and a phone UI", "task"),
2945            "feat: a queue, an unattended loop, and a phone UI"
2946        );
2947        assert_eq!(
2948            merge_subject("", "# port the retry logic\n\ndetails"),
2949            "port the retry logic",
2950            "an empty title falls back to the task's first line, heading marks stripped"
2951        );
2952    }
2953
2954    #[test]
2955    fn a_failing_checks_details_url_yields_the_job_to_read_logs_from() {
2956        let url = "https://github.com/yukimemi/magi/actions/runs/33587406996/job/100114323572";
2957        assert_eq!(job_of(url).as_deref(), Some("100114323572"));
2958        assert_eq!(run_of(url).as_deref(), Some("33587406996"));
2959        assert_eq!(job_of("https://coderabbit.ai/status"), None);
2960        assert_eq!(run_of(""), None);
2961    }
2962
2963    #[test]
2964    fn magis_own_stop_comment_is_never_read_back_as_a_finding() {
2965        let mut out = Vec::new();
2966        push_if_outstanding(
2967            &mut out,
2968            ReviewComment {
2969                author: "yukimemi".to_owned(),
2970                path: None,
2971                line: None,
2972                body: format!("{MARKER}\nmagi stopped landing this pull request: 1 check failing"),
2973            },
2974        );
2975        assert!(out.is_empty());
2976    }
2977
2978    /// A run with no tally, so [`RunState::winner`] is `None` and the panel
2979    /// falls back to the repository - which keeps these tests free of a
2980    /// worktree, a `git` invocation and a network.
2981    fn run_state() -> RunState {
2982        RunState::new(
2983            std::path::PathBuf::from("/repo/magi"),
2984            "main".to_owned(),
2985            "abcdef1234".to_owned(),
2986            "add retries to the uploader".to_owned(),
2987            crate::config::Config::default(),
2988        )
2989    }
2990
2991    fn green_pr() -> PrState {
2992        PrState {
2993            url: "https://github.com/yukimemi/magi/pull/42".to_owned(),
2994            number: 42,
2995            state: PrLifecycle::Open,
2996            checks: Checks::Green,
2997            // The forge sees nothing in the way unless a test says otherwise.
2998            blocking: Blocking::No,
2999            failing: Vec::new(),
3000            review_comments: vec![ReviewComment {
3001                author: "coderabbitai".to_owned(),
3002                path: Some("src/land.rs".to_owned()),
3003                line: Some(212),
3004                body: "this branch never checks the exit code".to_owned(),
3005            }],
3006        }
3007    }
3008
3009    const NUMSTAT: &str = "12\t3\tsrc/land.rs\n40\t1\tsrc/web.rs\n-\t-\tassets/logo.png";
3010
3011    fn panel() -> String {
3012        approval_panel(
3013            &run_state(),
3014            &green_pr(),
3015            NUMSTAT,
3016            "diff --git a/src/land.rs b/src/land.rs\n@@ -1,2 +1,2 @@\n-old line\n+new line\n context",
3017            &[
3018                "land: ask before merging".to_owned(),
3019                "land: colour the diff".to_owned(),
3020            ],
3021            "feat: merge approval from the phone",
3022        )
3023    }
3024
3025    #[test]
3026    fn the_approval_panel_carries_the_whole_case_for_the_merge() {
3027        let html = panel();
3028        for needle in [
3029            "42",
3030            "main",
3031            "src/land.rs",
3032            "src/web.rs",
3033            "assets/logo.png",
3034            "feat: merge approval from the phone",
3035            "land: ask before merging",
3036            "land: colour the diff",
3037            "coderabbitai",
3038            "this branch never checks the exit code",
3039            "green",
3040        ] {
3041            assert!(html.contains(needle), "the panel must state `{needle}`");
3042        }
3043    }
3044
3045    /// A candidate whose label is `A` and has won, so [`RunState::winner`]
3046    /// resolves to it.
3047    fn winning_candidate(summary: &str) -> Candidate {
3048        Candidate {
3049            index: 0,
3050            label: 'A',
3051            agent: "opus".to_owned(),
3052            branch: "magi/x/A".to_owned(),
3053            worktree: PathBuf::from("/wt/A"),
3054            summary: summary.to_owned(),
3055            stat: String::new(),
3056            files: 1,
3057            commits: 1,
3058            empty: false,
3059            failed: None,
3060            duration_ms: 0,
3061            folded: false,
3062        }
3063    }
3064
3065    fn uncontested_tally() -> Tally {
3066        Tally {
3067            first_choice: BTreeMap::from([('A', 1)]),
3068            borda: BTreeMap::new(),
3069            winner: 'A',
3070            rankings: 1,
3071            unanimous_initial: true,
3072            deliberated: false,
3073            changed_votes: 0,
3074            unanimous_final: true,
3075            tie_break: None,
3076            judges: 1,
3077            present: 1,
3078            quorum: 1,
3079            met_quorum: true,
3080            uncontested: None,
3081        }
3082    }
3083
3084    fn review_record(reviewer: usize, agent: &str, summary: &str) -> ReviewRecord {
3085        ReviewRecord {
3086            reviewer,
3087            agent: agent.to_owned(),
3088            summary: summary.to_owned(),
3089            findings: Vec::new(),
3090            vote: None,
3091            failed: None,
3092            duration_ms: 0,
3093        }
3094    }
3095
3096    fn review_round(round: usize, reviews: Vec<ReviewRecord>) -> ReviewRound {
3097        let answered = reviews.len();
3098        ReviewRound {
3099            round,
3100            head: "abc1234".to_owned(),
3101            verified_head: None,
3102            reviews,
3103            e2e: Vec::new(),
3104            verify_retried: false,
3105            e2e_deferred: false,
3106            e2e_defer_reason: None,
3107            fix: None,
3108            blocking: 0,
3109            answered,
3110            expected: answered,
3111            clean: true,
3112            progressed: false,
3113            vote_split: false,
3114            reconsideration: Vec::new(),
3115            verdict: None,
3116        }
3117    }
3118
3119    #[test]
3120    fn the_approval_panel_states_the_task_verbatim_in_either_language() {
3121        let en = panel();
3122        assert!(en.contains("Task"), "{en}");
3123        assert!(en.contains("add retries to the uploader"), "{en}");
3124
3125        let mut state = run_state();
3126        state.config.graph.language = "ja".to_owned();
3127        let ja = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3128        assert!(ja.contains("タスク"), "{ja}");
3129        assert!(
3130            ja.contains("add retries to the uploader"),
3131            "the task itself is not translated: {ja}"
3132        );
3133    }
3134
3135    #[test]
3136    fn the_approval_panel_omits_what_changed_and_review_verdict_with_no_data() {
3137        // `run_state()` has no candidates, no tally and no reviews - exactly
3138        // the shape a run has before anything has judged or reviewed it, and
3139        // the panel must not print an empty box for either.
3140        let html = panel();
3141        assert!(!html.contains("What changed"), "{html}");
3142        assert!(!html.contains("Review verdict"), "{html}");
3143    }
3144
3145    #[test]
3146    fn the_approval_panel_omits_what_changed_when_the_winners_summary_is_empty() {
3147        let mut state = run_state();
3148        state.candidates = vec![winning_candidate("")];
3149        state.tally = Some(uncontested_tally());
3150        let html = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3151        assert!(
3152            !html.contains("What changed"),
3153            "an empty summary must not render an empty box: {html}"
3154        );
3155    }
3156
3157    #[test]
3158    fn the_approval_panel_shows_the_winners_own_account_in_either_language() {
3159        let mut state = run_state();
3160        state.candidates = vec![winning_candidate(
3161            "Added a retry loop around the uploader PUT call.",
3162        )];
3163        state.tally = Some(uncontested_tally());
3164        let en = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3165        assert!(en.contains("What changed"), "{en}");
3166        assert!(
3167            en.contains("Added a retry loop around the uploader PUT call."),
3168            "{en}"
3169        );
3170
3171        state.config.graph.language = "ja".to_owned();
3172        let ja = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3173        assert!(ja.contains("変更内容"), "{ja}");
3174        assert!(
3175            ja.contains("Added a retry loop around the uploader PUT call."),
3176            "{ja}"
3177        );
3178    }
3179
3180    #[test]
3181    fn the_approval_panel_shows_only_the_last_review_rounds_verdict() {
3182        let mut state = run_state();
3183        state.reviews = vec![
3184            review_round(
3185                1,
3186                vec![review_record(1, "alpha", "found a race, sent back")],
3187            ),
3188            review_round(2, vec![review_record(1, "alpha", "race is fixed, clean")]),
3189        ];
3190        let en = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3191        assert!(en.contains("Review verdict"), "{en}");
3192        assert!(en.contains("race is fixed, clean"), "{en}");
3193        assert!(
3194            !en.contains("found a race, sent back"),
3195            "only the round that actually cleared the merge should show: {en}"
3196        );
3197
3198        state.config.graph.language = "ja".to_owned();
3199        let ja = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3200        assert!(ja.contains("レビューの結論"), "{ja}");
3201        assert!(ja.contains("レビュアー"), "{ja}");
3202        assert!(ja.contains("race is fixed, clean"), "{ja}");
3203    }
3204
3205    /// The `incomplete_review = "warn"` policy (see
3206    /// `graph::Runner::review_loop`) can push a `clean` round to
3207    /// `state.reviews` while one seat's own record still has `failed: Some`
3208    /// and an empty `summary` - a seat that never answered, not one that
3209    /// answered with nothing to say.
3210    fn unanswered_review_record(reviewer: usize, agent: &str, reason: &str) -> ReviewRecord {
3211        ReviewRecord {
3212            reviewer,
3213            agent: agent.to_owned(),
3214            summary: String::new(),
3215            findings: Vec::new(),
3216            vote: None,
3217            failed: Some(reason.to_owned()),
3218            duration_ms: 0,
3219        }
3220    }
3221
3222    #[test]
3223    fn the_approval_panel_never_shows_an_unanswered_seat_as_a_blank_verdict() {
3224        let mut state = run_state();
3225        state.reviews = vec![review_round(
3226            1,
3227            vec![
3228                review_record(1, "alpha", "clean, nothing to add"),
3229                unanswered_review_record(2, "beta", "timed out"),
3230            ],
3231        )];
3232        let en = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3233        assert!(en.contains("clean, nothing to add"), "{en}");
3234        assert!(
3235            en.contains("produced no answer: timed out"),
3236            "a seat that never answered must say so, not render a blank box: {en}"
3237        );
3238        assert!(
3239            !en.contains("<div style=\"white-space:pre-wrap;font-size:13px\"></div>"),
3240            "no reviewer box may be left empty: {en}"
3241        );
3242
3243        state.config.graph.language = "ja".to_owned();
3244        let ja = approval_panel(&state, &green_pr(), NUMSTAT, "", &[], "feat: x");
3245        assert!(ja.contains("回答なし: timed out"), "{ja}");
3246    }
3247
3248    #[test]
3249    fn the_approval_panel_contains_nothing_the_frames_policy_would_block() {
3250        let html = panel();
3251        assert!(!html.contains("<script"), "no script survives the csp");
3252        assert!(!html.contains("<form"), "form-action is 'none'");
3253        let pr = green_pr();
3254        assert_eq!(
3255            html.matches("http").count(),
3256            html.matches(pr.url.as_str()).count(),
3257            "the only http url in the panel is the pull request's own link"
3258        );
3259    }
3260
3261    #[test]
3262    fn added_and_removed_diff_lines_are_distinguishable_without_colour() {
3263        let html = panel();
3264        assert!(
3265            html.contains(">+</span>"),
3266            "an added line carries a `+` in the gutter, not only a background"
3267        );
3268        assert!(
3269            html.contains(">-</span>"),
3270            "a removed line carries a `-` in the gutter, not only a background"
3271        );
3272        assert!(
3273            html.contains(">new line</span>"),
3274            "the marker is moved to the gutter, so the body is printed once without it"
3275        );
3276    }
3277
3278    #[test]
3279    fn a_diff_past_the_threshold_is_cut_with_an_honest_count() {
3280        let total = DIFF_MAX_LINES + 100;
3281        let diff: String = (0..total).map(|i| format!("+line {i}\n")).collect();
3282        let html = approval_panel(
3283            &run_state(),
3284            &green_pr(),
3285            NUMSTAT,
3286            &diff,
3287            &[],
3288            "feat: something long",
3289        );
3290        assert!(
3291            html.contains(&format!("100 of {total} diff lines omitted")),
3292            "the note must say exactly how much was cut"
3293        );
3294        assert!(html.contains(&format!("line {}", DIFF_MAX_LINES - 1)));
3295        assert!(
3296            !html.contains(&format!("line {DIFF_MAX_LINES}")),
3297            "nothing past the threshold is rendered"
3298        );
3299        assert!(
3300            html.contains("/repo/magi"),
3301            "the note says where the rest is"
3302        );
3303    }
3304
3305    #[test]
3306    fn a_path_with_html_metacharacters_is_escaped_rather_than_rendered() {
3307        let html = approval_panel(
3308            &run_state(),
3309            &green_pr(),
3310            "1\t2\tsrc/<b>&\"x\"'.rs",
3311            "",
3312            &[],
3313            "subject",
3314        );
3315        assert!(html.contains("src/&lt;b&gt;&amp;&quot;x&quot;&#39;.rs"));
3316        assert!(
3317            !html.contains("<b>"),
3318            "an agent-influenced path must never become markup"
3319        );
3320    }
3321
3322    #[tokio::test]
3323    async fn the_merge_lock_serialises_one_repository_but_never_a_different_one() {
3324        let a = std::path::PathBuf::from("/repo/a");
3325        let b = std::path::PathBuf::from("/repo/b");
3326
3327        let held = repo_merge_lock(&a).lock_owned().await;
3328
3329        // A second, concurrent land run against the *same* repository must
3330        // wait - `try_lock` fails while `held` is alive.
3331        assert!(
3332            repo_merge_lock(&a).try_lock().is_err(),
3333            "a second merge into the same repository must not proceed concurrently"
3334        );
3335
3336        // A run against a *different* repository must not be blocked by it -
3337        // this is what keeps a slow rebase or `gh pr merge` in one
3338        // repository from also stalling a land-approval resume in another.
3339        assert!(
3340            repo_merge_lock(&b).try_lock().is_ok(),
3341            "a different repository's merge lock must be independent"
3342        );
3343
3344        drop(held);
3345        assert!(
3346            repo_merge_lock(&a).try_lock().is_ok(),
3347            "the lock is released once the holder is done"
3348        );
3349    }
3350
3351    #[test]
3352    fn only_the_merge_choice_merges_and_silence_holds() {
3353        let table = [
3354            (None, Approval::Hold),
3355            (Some("merge"), Approval::Merge),
3356            (Some(" merge\n"), Approval::Merge),
3357            (Some("hold"), Approval::Hold),
3358            (Some(""), Approval::Hold),
3359            (Some("yes"), Approval::Hold),
3360        ];
3361        for (answer, want) in table {
3362            assert_eq!(
3363                approval(answer),
3364                want,
3365                "answer {answer:?} must resolve to {want:?}"
3366            );
3367        }
3368    }
3369
3370    #[tokio::test]
3371    async fn a_first_visit_to_the_merge_gate_files_a_question_and_returns_pending_at_once() {
3372        crate::run::set_home(std::env::temp_dir().join("magi-land-approval-test-home"));
3373        let mut state = run_state();
3374        state.config.graph.land_approval = true;
3375        let pr = green_pr();
3376
3377        let gate = approval_gate(&mut state, &pr, "feat: x").await.unwrap();
3378        assert_eq!(gate, ApprovalGate::Pending, "nobody has answered yet");
3379        assert!(
3380            !state.parked,
3381            "approval_gate itself never sets `parked`; only its caller does"
3382        );
3383
3384        let store = ask::Questions::open();
3385        let filed: Vec<_> = store
3386            .list()
3387            .into_iter()
3388            .filter(|q| q.run == state.id)
3389            .collect();
3390        assert_eq!(filed.len(), 1, "exactly one question is filed");
3391        assert_eq!(filed[0].node, APPROVAL_NODE);
3392        assert_eq!(filed[0].choices, vec![APPROVE.to_owned(), HOLD.to_owned()]);
3393        assert!(filed[0].status.open());
3394
3395        // A second visit - standing in for a resumed run whose slot the
3396        // daemon handed to something else while nobody had answered - must
3397        // find the same question rather than filing a second one.
3398        let again = approval_gate(&mut state, &pr, "feat: x").await.unwrap();
3399        assert_eq!(again, ApprovalGate::Pending);
3400        let still_one = store
3401            .list()
3402            .into_iter()
3403            .filter(|q| q.run == state.id)
3404            .count();
3405        assert_eq!(
3406            still_one, 1,
3407            "asking twice must not double-file the question"
3408        );
3409    }
3410
3411    #[tokio::test]
3412    async fn approving_the_existing_question_is_read_back_as_approved() {
3413        crate::run::set_home(std::env::temp_dir().join("magi-land-approval-test-home"));
3414        let mut state = run_state();
3415        state.config.graph.land_approval = true;
3416        let pr = green_pr();
3417        assert_eq!(
3418            approval_gate(&mut state, &pr, "feat: x").await.unwrap(),
3419            ApprovalGate::Pending
3420        );
3421
3422        let store = ask::Questions::open();
3423        let mut q = store
3424            .list()
3425            .into_iter()
3426            .find(|q| q.run == state.id)
3427            .expect("filed above");
3428        q.answer(ask::Answer::Choice(APPROVE.to_owned())).unwrap();
3429        store.put(&mut q).unwrap();
3430
3431        assert_eq!(
3432            approval_gate(&mut state, &pr, "feat: x").await.unwrap(),
3433            ApprovalGate::Approved
3434        );
3435    }
3436
3437    #[tokio::test]
3438    async fn holding_or_abandoning_the_existing_question_is_read_back_as_held() {
3439        crate::run::set_home(std::env::temp_dir().join("magi-land-approval-test-home"));
3440        let store = ask::Questions::open();
3441
3442        let mut held_state = run_state();
3443        held_state.config.graph.land_approval = true;
3444        let pr = green_pr();
3445        approval_gate(&mut held_state, &pr, "feat: x")
3446            .await
3447            .unwrap();
3448        let mut q = store
3449            .list()
3450            .into_iter()
3451            .find(|q| q.run == held_state.id)
3452            .expect("filed above");
3453        q.answer(ask::Answer::Choice(HOLD.to_owned())).unwrap();
3454        store.put(&mut q).unwrap();
3455        assert_eq!(
3456            approval_gate(&mut held_state, &pr, "feat: x")
3457                .await
3458                .unwrap(),
3459            ApprovalGate::Held
3460        );
3461
3462        let mut abandoned_state = run_state();
3463        abandoned_state.config.graph.land_approval = true;
3464        approval_gate(&mut abandoned_state, &pr, "feat: x")
3465            .await
3466            .unwrap();
3467        let mut q = store
3468            .list()
3469            .into_iter()
3470            .find(|q| q.run == abandoned_state.id)
3471            .expect("filed above");
3472        q.abandon("no answer within the timeout");
3473        store.put(&mut q).unwrap();
3474        assert_eq!(
3475            approval_gate(&mut abandoned_state, &pr, "feat: x")
3476                .await
3477                .unwrap(),
3478            ApprovalGate::Held,
3479            "silence must never merge"
3480        );
3481    }
3482
3483    #[test]
3484    fn the_diffstat_table_is_ordered_by_churn_with_binaries_last() {
3485        let rows = parse_numstat(NUMSTAT);
3486        assert_eq!(
3487            rows.iter().map(|r| r.path.as_str()).collect::<Vec<_>>(),
3488            ["src/web.rs", "src/land.rs", "assets/logo.png"]
3489        );
3490        assert_eq!(rows[2].added, None, "a binary file has no line counts");
3491    }
3492    #[test]
3493    fn the_approval_speaks_the_language_the_repository_is_configured_for() {
3494        // Reported from a real run: the merge question arrived in English on a
3495        // repository with `language = "ja"`. magi's own strings have to follow
3496        // that setting too - "it is a literal in Rust" is not an answer.
3497        let mut state = run_state();
3498        state.config.graph.language = "ja".to_owned();
3499        let pr = green_pr();
3500        let commits = ["c1".to_owned()];
3501
3502        let ja = approval_panel(&state, &pr, "3\t1\tsrc/a.rs", "+ x", &commits, "feat: x");
3503        assert!(ja.contains("lang=\"ja\""), "the document must declare it");
3504        assert!(ja.contains("squash されるコミット"), "{ja}");
3505        assert!(ja.contains("レビューコメント"), "{ja}");
3506        assert!(ja.contains("差分"), "{ja}");
3507        assert!(
3508            !ja.contains("Commits being squashed"),
3509            "no English left over"
3510        );
3511
3512        let w = words("ja");
3513        assert!(w.approval_summary(17, "feat: x").contains("マージ"));
3514        assert!(
3515            w.approval_detail("http://x/1", "main", "feat: x")
3516                .contains("パネル")
3517        );
3518
3519        // The evidence itself is language-neutral and must survive either way.
3520        assert!(ja.contains("src/a.rs"), "the diffstat is not prose");
3521        assert!(ja.contains("feat: x"), "nor is the merge subject");
3522
3523        // English stays the default, and a language magi cannot check falls
3524        // back to it rather than shipping a guess.
3525        state.config.graph.language = "en".to_owned();
3526        let en = approval_panel(&state, &pr, "3\t1\tsrc/a.rs", "+ x", &commits, "feat: x");
3527        assert!(en.contains("Commits being squashed"), "{en}");
3528        assert_eq!(words("Klingon").html_lang, "en");
3529    }
3530}