fix(desktop): merge the right pull request and ignore advisory checks

apps/desktop/src/acp/mock.ts

@@ -2337,6 +2337,84 @@ function configOptions(thread: MockThread, harnesses: HarnessSnapshot): Json[] {
   return options;
 }
 
+/** The pull requests the mock project has; `/view` opens one of these by number. */
+const MOCK_PULLS = [
+  {
+    number: 2788,
+    title: "feat(desktop): Tauri desktop app on every harness",
+    author: "LHagfoss",
+    state: "open",
+    draft: true,
+    review: "review_required",
+    checks: { passed: 5, failed: 0, pending: 1, skipped: 0 },
+    headRef: "feat/desktop-tauri-harnesses",
+    baseRef: "main",
+    headSha: "58961fc5aa",
+    updatedAt: new Date(minutes(20)).toISOString(),
+    url: "https://github.com/LHagfoss/rustcode/pull/2788",
+    additions: 9120,
+    deletions: 12,
+    changedFiles: 64,
+  },
+  {
+    number: 2781,
+    title: "fix(engine): keep the steer queue across a restart",
+    author: "LHagfoss",
+    state: "open",
+    draft: false,
+    review: "approved",
+    checks: { passed: 6, failed: 1, pending: 0, skipped: 0 },
+    headRef: "fix/engine-steer-restart",
+    baseRef: "main",
+    headSha: "a1b2c3d4",
+    updatedAt: new Date(minutes(300)).toISOString(),
+    url: "https://github.com/LHagfoss/rustcode/pull/2781",
+    additions: 40,
+    deletions: 9,
+    changedFiles: 3,
+  },
+  {
+    number: 2795,
+    title: "feat(desktop): add the login form",
+    author: "LHagfoss",
+    state: "open",
+    draft: false,
+    review: "changes_requested",
+    checks: { passed: 4, failed: 1, pending: 0, skipped: 0 },
+    headRef: "feat/login-form",
+    baseRef: "main",
+    headSha: "58961fc5aa",
+    updatedAt: new Date(minutes(30)).toISOString(),
+    url: "https://github.com/LHagfoss/rustcode/pull/2795",
+    additions: 210,
+    deletions: 4,
+    changedFiles: 5,
+  },
+];
+
+/**
+ * Each pull's check runs, matching its row's counts: #2781's one failure is an
+ * advisory job (Merge stays open), #2795's is a real one (Merge is blocked).
+ */
+const MOCK_CHECK_RUNS: Record<number, { name: string; status: string }[]> = {
+  2781: [
+    { name: "desktop / check", status: "passed" },
+    { name: "lint", status: "passed" },
+    { name: "label", status: "passed" },
+    { name: "Desktop UI", status: "passed" },
+    { name: "rust / test", status: "passed" },
+    { name: "clippy", status: "passed" },
+    { name: "Windows and macOS portability coverage", status: "failed" },
+  ],
+  2795: [
+    { name: "desktop / check", status: "passed" },
+    { name: "lint", status: "passed" },
+    { name: "label", status: "passed" },
+    { name: "rust / test", status: "passed" },
+    { name: "Desktop UI", status: "failed" },
+  ],
+};
+
 export class MockEngine {
   /** What this computer knows of the others (`fleet.json`), by host id. */
   private textGenerators: JsonObject = { generators: [], selected: null };
@@ -3647,65 +3725,7 @@ export class MockEngine {
       case "_rustcode/environment/sync":
         return this.environmentSync(obj(params) ?? {});
       case "_rustcode/pulls":
-        return {
-          list: {
-            provider: "github",
-            hasMore: false,
-            pulls: [
-              {
-                number: 2788,
-                title: "feat(desktop): Tauri desktop app on every harness",
-                author: "LHagfoss",
-                state: "open",
-                draft: true,
-                review: "review_required",
-                checks: { passed: 5, failed: 0, pending: 1, skipped: 0 },
-                headRef: "feat/desktop-tauri-harnesses",
-                baseRef: "main",
-                headSha: "58961fc5aa",
-                updatedAt: new Date(minutes(20)).toISOString(),
-                url: "https://github.com/LHagfoss/rustcode/pull/2788",
-                additions: 9120,
-                deletions: 12,
-                changedFiles: 64,
-              },
-              {
-                number: 2781,
-                title: "fix(engine): keep the steer queue across a restart",
-                author: "LHagfoss",
-                state: "open",
-                draft: false,
-                review: "approved",
-                checks: { passed: 6, failed: 1, pending: 0, skipped: 0 },
-                headRef: "fix/engine-steer-restart",
-                baseRef: "main",
-                headSha: "a1b2c3d4",
-                updatedAt: new Date(minutes(300)).toISOString(),
-                url: "https://github.com/LHagfoss/rustcode/pull/2781",
-                additions: 40,
-                deletions: 9,
-                changedFiles: 3,
-              },
-              {
-                number: 2795,
-                title: "feat(desktop): add the login form",
-                author: "LHagfoss",
-                state: "open",
-                draft: false,
-                review: "changes_requested",
-                checks: { passed: 4, failed: 1, pending: 0, skipped: 0 },
-                headRef: "feat/login-form",
-                baseRef: "main",
-                headSha: "58961fc5aa",
-                updatedAt: new Date(minutes(30)).toISOString(),
-                url: "https://github.com/LHagfoss/rustcode/pull/2795",
-                additions: 210,
-                deletions: 4,
-                changedFiles: 5,
-              },
-            ],
-          },
-        };
+        return { list: { provider: "github", hasMore: false, pulls: MOCK_PULLS } };
       case "_rustcode/project/read": {
         // The engine's `_rustcode/project/read`: a folder's git facts.
         const path = str(obj(params)?.path) || "/Users/me/code/rustcode";
@@ -3986,30 +4006,33 @@ export class MockEngine {
           checkout_branch: "main",
         } as unknown as Json;
       }
-      case "_rustcode/pulls/view":
+      case "_rustcode/pulls/view": {
+        // The row the list shows, so the detail agrees with it; a number the list
+        // does not have opens as an open pull with its checks still running.
+        const row = MOCK_PULLS.find((pull) => pull.number === params.number);
         return {
           pull: {
-            number: params.number as number,
             title: "feat(desktop): Tauri desktop app on every harness",
             author: "LHagfoss",
             state: "open",
-            // #2788 is the draft the list shows; any other opens ready to merge.
-            draft: params.number === 2788,
+            draft: false,
             checks: { passed: 5, failed: 0, pending: 1, skipped: 0 },
             headRef: "feat/desktop-tauri-harnesses",
             baseRef: "main",
             headSha: "58961fc5aa",
-            url: `https://github.com/LHagfoss/rustcode/pull/${params.number}`,
             additions: 9120,
             deletions: 12,
             changedFiles: 64,
+            ...row,
+            number: params.number as number,
+            url: `https://github.com/LHagfoss/rustcode/pull/${params.number}`,
             body: "Adds `apps/desktop`: a Tauri shell around `rustcode --acp`.\n\n- Composer, approvals, harness pickers\n- Link chips and Smart UI",
-            checkRuns: [
+            checkRuns: MOCK_CHECK_RUNS[params.number as number] ?? [
               { name: "desktop / check", status: "passed" },
               { name: "lint", status: "passed" },
               { name: "label", status: "passed" },
               { name: "Desktop UI", status: "passed" },
-              { name: "rust / test", status: "pending" },
+              { name: "rust / test", status: "passed" },
               // A matrix job GitHub skipped before it expanded: the template stays in its name.
               { name: ["Desktop shell (", "$", "{{ matrix.name }})"].join(""), status: "pending" },
             ],
@@ -4145,6 +4168,7 @@ export class MockEngine {
             ],
           },
         };
+      }
       case "_rustcode/pulls/diff":
         return {
           diff: [

apps/desktop/src/state/pullMerge.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, test } from "bun:test";
-import { checkName, checksHeadline, mergeControl } from "./pullMerge";
+import { advisoryCheck, checkName, checksHeadline, mergeControl } from "./pullMerge";
 
 const checks = (passed = 0, failed = 0, pending = 0) => ({ passed, failed, pending, skipped: 0 });
 
@@ -40,5 +40,36 @@ describe("check names", () => {
     expect(checksHeadline([run("passed"), run("pending"), run("pending")])).toBe("2 checks running");
     expect(checksHeadline([run("passed"), run("skipped")])).toBe("Checks passed");
     expect(checksHeadline([])).toBe("No checks");
+    expect(checksHeadline([run("passed"), { name: "portability coverage", status: "failed" }])).toBe(
+      "Checks passed",
+    );
+  });
+});
+
+describe("advisory checks", () => {
+  const run = (name: string, status: string) => ({ name, status });
+  const open = { state: "open" as const, draft: false, checks: checks(1, 1) };
+
+  test("are named as the engine names them", () => {
+    expect(advisoryCheck("Windows and macOS portability coverage")).toBe(true);
+    expect(advisoryCheck("portability coverage (windows)")).toBe(true);
+    expect(advisoryCheck("cargo test --workspace")).toBe(false);
+  });
+
+  test("never hold a merge, failing or running", () => {
+    expect(mergeControl(open, [run("lint", "passed"), run("portability coverage", "failed")])).toEqual({
+      kind: "merge",
+    });
+    expect(mergeControl(open, [run("lint", "passed"), run("portability coverage", "pending")])).toEqual({
+      kind: "merge",
+    });
+    expect(mergeControl(open, [run("lint", "failed"), run("portability coverage", "failed")])).toEqual({
+      kind: "blocked",
+      failed: 1,
+    });
+  });
+
+  test("the list's counts stand in until the runs are read", () => {
+    expect(mergeControl(open)).toEqual({ kind: "blocked", failed: 1 });
   });
 });

apps/desktop/src/state/pullMerge.ts

@@ -24,13 +24,40 @@ export type MergeControl =
   | { kind: "merge_when_green"; pending: number }
   | { kind: "blocked"; failed: number };
 
-export function mergeControl(pull: Pick<PullRequest, "state" | "draft" | "checks">): MergeControl {
+export function mergeControl(
+  pull: Pick<PullRequest, "state" | "draft" | "checks">,
+  runs?: readonly CheckRun[],
+): MergeControl {
   if (pull.state !== "open" || pull.draft) return { kind: "none" };
-  if (pull.checks.failed > 0) return { kind: "blocked", failed: pull.checks.failed };
-  if (pull.checks.pending > 0) return { kind: "merge_when_green", pending: pull.checks.pending };
+  // The detail's runs name each check, so the advisory ones the merge ignores
+  // can be left out; the list's counts cannot, and stand in until it is read.
+  const { failed, pending } = runs ? blockingCounts(runs) : pull.checks;
+  if (failed > 0) return { kind: "blocked", failed };
+  if (pending > 0) return { kind: "merge_when_green", pending };
   return { kind: "merge" };
 }
 
+/**
+ * The checks a merge never waits for, as the engine's `ADVISORY_CHECKS`
+ * (rustcode/engine/src/pull_requests.rs) and scripts/pr-merge.sh name them.
+ */
+export const ADVISORY_CHECKS = ["portability coverage"] as const;
+
+export function advisoryCheck(name: string): boolean {
+  const lower = name.toLowerCase();
+  return ADVISORY_CHECKS.some((advisory) => lower.includes(advisory));
+}
+
+const settled = (status: string) => status === "passed" || status === "skipped";
+
+/** Failing and running checks that hold a merge: every one but the advisory jobs. */
+export function blockingCounts(runs: readonly CheckRun[]): { failed: number; pending: number } {
+  const blocking = runs.filter((c) => !advisoryCheck(c.name));
+  const failed = blocking.filter((c) => c.status === "failed").length;
+  const pending = blocking.filter((c) => c.status !== "failed" && !settled(c.status)).length;
+  return { failed, pending };
+}
+
 /**
  * A check's name as people read it. A job GitHub skipped before it expanded its
  * matrix keeps the template in its name (`Desktop shell (${{ matrix.name }})`):
@@ -49,10 +76,8 @@ type CheckRun = { name: string; status: string };
 
 /** The checks' headline: what needs the user first, then that everything passed. */
 export function checksHeadline(runs: readonly CheckRun[]): string {
-  const failed = runs.filter((c) => c.status === "failed").length;
-  const pending = runs.filter(
-    (c) => c.status !== "failed" && c.status !== "passed" && c.status !== "skipped",
-  ).length;
+  // Advisory jobs never hold a merge, so they never lead the headline either.
+  const { failed, pending } = blockingCounts(runs);
   if (failed) return `${failed} ${failed === 1 ? "check failed" : "checks failed"}`;
   if (pending) return `${pending} ${pending === 1 ? "check running" : "checks running"}`;
   return runs.length ? "Checks passed" : "No checks";
@@ -74,7 +99,11 @@ const subscribe = (listener: () => void) => {
 
 /** Whether a merge of this pull is waiting or running. */
 export function useMerging(cwd: string, number: number): boolean {
-  useSyncExternalStore(subscribe, () => version);
+  useSyncExternalStore(
+    subscribe,
+    () => version,
+    () => version,
+  );
   return running.has(keyOf(cwd, number));
 }
 

apps/desktop/src/ui/MergeButton.test.tsx

@@ -0,0 +1,70 @@
+import { describe, expect, test } from "bun:test";
+import { Tooltip } from "radix-ui";
+import { renderToStaticMarkup } from "react-dom/server";
+import type { PullRequest } from "../acp/rustcode";
+import { MergeButton } from "./PullRequests";
+
+const PULL: PullRequest = {
+  number: 2781,
+  title: "fix(engine): keep the steer queue across a restart",
+  author: "LHagfoss",
+  state: "open",
+  draft: false,
+  checks: { passed: 6, failed: 0, pending: 0, skipped: 0 },
+  headRef: "fix/engine-steer-restart",
+  baseRef: "main",
+  headSha: "a1b2c3d4",
+  updatedAt: 0,
+  additions: 40,
+  deletions: 9,
+  changedFiles: 3,
+} as PullRequest;
+
+const run = (name: string, status: string) => ({ name, status });
+
+function render(pull: Partial<PullRequest>, runs?: { name: string; status: string }[]): string {
+  return renderToStaticMarkup(
+    <Tooltip.Provider>
+      <MergeButton cwd="/repo" pull={{ ...PULL, ...pull }} runs={runs} />
+    </Tooltip.Provider>,
+  );
+}
+
+// The attribute, not the `disabled:` styling every button's classes carry.
+const enabled = (html: string) => html.includes("<button") && !/<button[^>]* disabled=""/.test(html);
+
+describe("the merge button", () => {
+  test("merges once every check passed", () => {
+    const html = render({});
+    expect(html).toContain("Merge");
+    expect(html).not.toContain("Merge when checks pass");
+    expect(enabled(html)).toBe(true);
+  });
+
+  test("waits for running checks", () => {
+    const html = render({ checks: { passed: 4, failed: 0, pending: 2, skipped: 0 } });
+    expect(html).toContain("Merge when checks pass");
+    expect(enabled(html)).toBe(true);
+  });
+
+  test("is disabled by a failing check, without repeating the count the Checks headline has", () => {
+    const html = render({}, [run("lint", "passed"), run("Desktop UI", "failed")]);
+    expect(html).toContain("Merge");
+    expect(html).not.toContain("failed");
+    expect(enabled(html)).toBe(false);
+  });
+
+  test("does not let an advisory job block it, as the engine's merge does not", () => {
+    const runs = [run("lint", "passed"), run("Windows and macOS portability coverage", "failed")];
+    // The list counted the advisory failure; the detail's runs say what it was.
+    const html = render({ checks: { passed: 1, failed: 1, pending: 0, skipped: 0 } }, runs);
+    expect(enabled(html)).toBe(true);
+    expect(html).not.toContain("Merge when checks pass");
+  });
+
+  test("offers nothing for a draft or a finished pull", () => {
+    expect(render({ draft: true })).toBe("");
+    expect(render({ state: "merged" })).toBe("");
+    expect(render({ state: "closed" })).toBe("");
+  });
+});