Review: a global review destination, from queue to submitted PR

app/build.gradle.kts

@@ -93,11 +93,43 @@ dependencies {
     testImplementation(platform("org.junit:junit-bom:5.11.4"))
     testImplementation("org.junit.jupiter:junit-jupiter")
     testRuntimeOnly("org.junit.platform:junit-platform-launcher")
+
+    // Headless JavaFX harness (see the Monocle block in tasks.test). TestFX
+    // supplies the robot and the node-query DSL; Monocle supplies a Glass
+    // platform that needs no window server, so view tests run identically on
+    // a developer's Mac and on a CI runner with no GUI session. hamcrest is
+    // explicit because testfx-core exposes org.hamcrest.Matcher in its public
+    // API but only declares the dependency at runtime scope.
+    testImplementation("org.hamcrest:hamcrest:2.2")
+    testImplementation("org.testfx:testfx-core:4.0.18")
+    testImplementation("org.testfx:testfx-junit5:4.0.18")
+    testRuntimeOnly("org.testfx:openjfx-monocle:21.0.2")
 }
 
 tasks.test {
     useJUnitPlatform()
 
+    // Headless JavaFX. Monocle replaces the platform's Glass backend with one
+    // that renders to memory, so a test can show a real Stage, apply the real
+    // stylesheets and drive real key events without a window server -- which
+    // is what lets the view tests run on the macos-14 CI runner.
+    //
+    // Set for the whole test JVM rather than per test class: these are read
+    // once at toolkit startup, and Gradle runs one JVM for the suite. The
+    // non-FX tests are unaffected.
+    //
+    // headless.geometry is load-bearing: Monocle's default virtual screen is
+    // 1280x800, and a Scene larger than the screen overflows the software
+    // pixel buffer (BufferOverflowException from the Prism SW pipeline). The
+    // Review view's rails only stay expanded above 1320px wide, so the
+    // virtual screen has to be bigger than the widest scene any test builds.
+    systemProperty("glass.platform", "Monocle")
+    systemProperty("monocle.platform", "Headless")
+    systemProperty("prism.order", "sw")
+    systemProperty("javafx.headless", "true")
+    systemProperty("headless.geometry", "1920x1200-32")
+
+
     // Inputs for RuntimeImageModuleListTest: the packaged app jar, its runtime
     // classpath, and the convention-plugin source that declares the jlink
     // --add-modules list. The test runs jdeps against the jar the packaging

app/src/main/java/app/drydock/git/DiffService.java

@@ -28,6 +28,15 @@
  */
 public final class DiffService implements AutoCloseable {
 
+    /** git's own default: three unchanged lines either side of a change. */
+    public static final int DEFAULT_CONTEXT_LINES = 3;
+
+    /**
+     * What the Review diff column asks for. Wide enough that a fold is worth
+     * making -- see {@link #diff(Path, DiffScope, String, int)}.
+     */
+    public static final int REVIEW_CONTEXT_LINES = 12;
+
     /** Every command here is a quick read-only query; a hung git must not park futures forever. */
     private static final Duration PROCESS_TIMEOUT = Duration.ofSeconds(15);
 
@@ -62,7 +71,23 @@ private DiffService(GitExecutableLocator locator, ExecutorService executor, bool
      * {@link java.util.concurrent.CompletionException}) on any failure.
      */
     public CompletableFuture<UnifiedDiff> diff(Path checkoutRoot, DiffScope scope, String baseBranch) {
-        return CompletableFuture.supplyAsync(() -> diffBlocking(checkoutRoot, scope, baseBranch), executor);
+        return diff(checkoutRoot, scope, baseBranch, DEFAULT_CONTEXT_LINES);
+    }
+
+    /**
+     * As {@link #diff(Path, DiffScope, String)}, with an explicit number of
+     * unchanged lines around each change ({@code git diff -U}).
+     *
+     * <p>Review asks for {@link #REVIEW_CONTEXT_LINES} rather than git's
+     * default three: its diff column folds long unchanged runs into a single
+     * {@code ⋯ N unchanged} row, and with a three-line window there is never
+     * a run long enough to be worth folding -- the feature would render, and
+     * simply never appear. Showing more and folding it is the point.</p>
+     */
+    public CompletableFuture<UnifiedDiff> diff(Path checkoutRoot, DiffScope scope, String baseBranch,
+                                               int contextLines) {
+        return CompletableFuture.supplyAsync(
+                () -> diffBlocking(checkoutRoot, scope, baseBranch, contextLines), executor);
     }
 
     /**
@@ -72,6 +97,13 @@ public CompletableFuture<UnifiedDiff> diff(Path checkoutRoot, DiffScope scope, S
      * application thread.
      */
     UnifiedDiff diffBlocking(Path checkoutRoot, DiffScope scope, String baseBranch) {
+        return diffBlocking(checkoutRoot, scope, baseBranch, DEFAULT_CONTEXT_LINES);
+    }
+
+    UnifiedDiff diffBlocking(Path checkoutRoot, DiffScope scope, String baseBranch, int contextLines) {
+        if (contextLines < 0) {
+            throw new IllegalArgumentException("contextLines must be non-negative: " + contextLines);
+        }
         Path git = locator.locate()
                 .orElseThrow(() -> new GitExecutableNotFoundException(locator.describeSearched()));
 
@@ -84,7 +116,8 @@ UnifiedDiff diffBlocking(Path checkoutRoot, DiffScope scope, String baseBranch)
         // reach git as a revision, never be parsed as a flag.
         List<String> command = List.of(
                 git.toString(), "-C", checkoutRoot.toString(),
-                "diff", "--no-color", "--no-ext-diff", "--end-of-options", range);
+                "diff", "--no-color", "--no-ext-diff", "-U" + contextLines,
+                "--end-of-options", range);
 
         ProcessResult result = run(command);
         if (result.exitCode() != 0) {
@@ -138,6 +171,16 @@ public void close() {
      * carries the gutter values (and stable annotation keys). Binary
      * files produce a file entry with no hunks.
      */
+    /**
+     * Parses a unified diff that did not come from this service's own
+     * {@code git diff} -- {@code gh pr diff} for the "Read the patch only"
+     * path, which has no local checkout to run git in. Same parser, so a PR
+     * read without a worktree renders exactly like one with.
+     */
+    public static UnifiedDiff parseUnified(String unifiedDiff) {
+        return parse(unifiedDiff, Set.of());
+    }
+
     static UnifiedDiff parse(String stdout, Set<String> stagedPaths) {
         List<UnifiedDiff.FileDiff> files = new ArrayList<>();
 

app/src/main/java/app/drydock/git/GhCliService.java

@@ -6,6 +6,7 @@
 import app.drydock.state.json.JsonParseException;
 import app.drydock.state.json.JsonParser;
 import app.drydock.state.json.JsonValue;
+import app.drydock.state.json.JsonValue.JsonArray;
 import app.drydock.state.json.JsonValue.JsonNumber;
 import app.drydock.state.json.JsonValue.JsonObject;
 import app.drydock.state.json.JsonValue.JsonString;
@@ -17,20 +18,23 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Locale;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.regex.Pattern;
 
 /**
- * READ-ONLY queries against the GitHub CLI ({@code gh}), used solely to
- * reconcile a worktree session's PR chip after a hand-off (design handoff
- * section B: the app itself never runs {@code gh pr create} or any other
- * mutation -- Claude in the terminal does; this service only observes the
- * result via {@code gh pr view}).
+ * READ-ONLY queries against the GitHub CLI ({@code gh}): the PR chip a
+ * worktree session reconciles after a hand-off, the review-requested queue,
+ * and the patch behind "Read the patch only". The app never runs
+ * {@code gh pr create} or any other mutation -- Claude in the terminal does;
+ * this service only observes. Checking a PR out is a working-tree change and
+ * lives in {@code PrCheckoutService}, not here.
  *
  * <p>{@code gh} is optional: when it is not installed, {@link #viewPr}
  * completes with an empty result and callers fall back to an optimistic
@@ -53,6 +57,26 @@ public record PrInfo(int number, PrLifecycle state, Optional<String> url) {
         public enum PrLifecycle { OPEN, MERGED, CLOSED, UNKNOWN }
     }
 
+    /**
+     * One row of {@code gh pr list --search "review-requested:@me"} (Review
+     * spec §4.1, the REQUESTED queue group). Carries what the queue rail
+     * renders plus the refs the checkout gate needs.
+     */
+    public record ReviewRequest(int number, String title, String headRefName, String baseRefName,
+                                Optional<String> author, Optional<String> url, int changedFiles,
+                                boolean draft) {
+        public ReviewRequest {
+            Objects.requireNonNull(title, "title");
+            Objects.requireNonNull(headRefName, "headRefName");
+            Objects.requireNonNull(baseRefName, "baseRefName");
+            Objects.requireNonNull(author, "author");
+            Objects.requireNonNull(url, "url");
+        }
+    }
+
+    /** How many review-requested PRs one {@code gh pr list} call may return. */
+    private static final int REVIEW_REQUEST_LIMIT = 50;
+
     private final ExecutorService executor;
     private final boolean ownsExecutor;
     private volatile Optional<Path> cachedExecutable;
@@ -130,6 +154,110 @@ Optional<PrInfo> viewPrBlocking(Path root, String branch) {
         }
     }
 
+    /**
+     * The open PRs in {@code root}'s repository that ask this user for a
+     * review -- the REQUESTED group of the Review queue.
+     *
+     * <p>An empty list means "nothing to tell you", and covers every
+     * no-information case alike: {@code gh} missing, not authenticated, the
+     * repository having no GitHub remote, or genuinely no requests. Review
+     * degrades rather than blocks (spec §6), so no caller distinguishes
+     * them; each is logged.</p>
+     */
+    public CompletableFuture<List<ReviewRequest>> listReviewRequests(Path root) {
+        return CompletableFuture.supplyAsync(() -> listReviewRequestsBlocking(root), executor);
+    }
+
+    List<ReviewRequest> listReviewRequestsBlocking(Path root) {
+        Path gh = locate().orElse(null);
+        if (gh == null) {
+            return List.of();
+        }
+        ProcessResult result = runIn(root, List.of(gh.toString(), "pr", "list",
+                "--search", "review-requested:@me",
+                "--state", "open",
+                "--limit", String.valueOf(REVIEW_REQUEST_LIMIT),
+                "--json", "number,title,headRefName,baseRefName,author,url,changedFiles,isDraft"));
+        if (result == null) {
+            return List.of();
+        }
+        if (result.exitCode() != 0) {
+            LOG.log(Level.DEBUG, "gh pr list (review-requested) in " + root + " exited " + result.exitCode()
+                    + (result.stderr().isBlank() ? "" : ": " + ProcessRunner.excerpt(result.stderr())));
+            return List.of();
+        }
+        try {
+            if (!(JsonParser.parse(result.stdout()) instanceof JsonArray array)) {
+                return List.of();
+            }
+            List<ReviewRequest> requests = new ArrayList<>();
+            for (JsonValue element : array.elements()) {
+                parseReviewRequest(element).ifPresent(requests::add);
+            }
+            return List.copyOf(requests);
+        } catch (JsonParseException | NumberFormatException e) {
+            LOG.log(Level.DEBUG, "Unparseable gh pr list output", e);
+            return List.of();
+        }
+    }
+
+    /**
+     * A row missing any of the fields the queue needs is skipped rather than
+     * failing the whole list: one malformed PR must not empty the REQUESTED
+     * group.
+     */
+    private static Optional<ReviewRequest> parseReviewRequest(JsonValue element) {
+        if (!(element instanceof JsonObject obj)
+                || !(obj.get("number") instanceof JsonNumber number)
+                || !(obj.get("headRefName") instanceof JsonString head)
+                || !(obj.get("baseRefName") instanceof JsonString base)) {
+            return Optional.empty();
+        }
+        int prNumber = number.asInt();
+        if (prNumber <= 0) {
+            return Optional.empty();
+        }
+        String title = obj.get("title") instanceof JsonString t ? t.value() : head.value();
+        Optional<String> author = obj.get("author") instanceof JsonObject a
+                && a.get("login") instanceof JsonString login
+                ? Optional.of(login.value())
+                : Optional.empty();
+        Optional<String> url = obj.get("url") instanceof JsonString u ? Optional.of(u.value()) : Optional.empty();
+        int changedFiles = obj.get("changedFiles") instanceof JsonNumber c ? c.asInt() : 0;
+        boolean draft = obj.get("isDraft") instanceof JsonValue.JsonBoolean d && d.value();
+        return Optional.of(new ReviewRequest(prNumber, title, head.value(), base.value(),
+                author, url, changedFiles, draft));
+    }
+
+    /**
+     * The unified diff of a pull request, as {@code gh pr diff} prints it --
+     * the "Read the patch only" path (Review handoff §6), which needs no
+     * worktree and therefore no session and no agent.
+     *
+     * <p>Empty when {@code gh} is missing, unauthenticated, or the PR cannot
+     * be read; the caller shows the gate rather than an empty diff.</p>
+     */
+    public CompletableFuture<Optional<String>> prDiff(Path root, int prNumber) {
+        return CompletableFuture.supplyAsync(() -> prDiffBlocking(root, prNumber), executor);
+    }
+
+    Optional<String> prDiffBlocking(Path root, int prNumber) {
+        Path gh = locate().orElse(null);
+        if (gh == null || prNumber <= 0) {
+            return Optional.empty();
+        }
+        ProcessResult result = runIn(root,
+                List.of(gh.toString(), "pr", "diff", String.valueOf(prNumber), "--patch"));
+        if (result == null || result.exitCode() != 0) {
+            if (result != null) {
+                LOG.log(Level.DEBUG, "gh pr diff " + prNumber + " exited " + result.exitCode()
+                        + (result.stderr().isBlank() ? "" : ": " + ProcessRunner.excerpt(result.stderr())));
+            }
+            return Optional.empty();
+        }
+        return result.stdout().isBlank() ? Optional.empty() : Optional.of(result.stdout());
+    }
+
     private static PrInfo.PrLifecycle lifecycleOf(String raw) {
         return switch (raw.toUpperCase(Locale.ROOT)) {
             case "OPEN" -> PrInfo.PrLifecycle.OPEN;

app/src/main/java/app/drydock/git/GitStatusService.java

@@ -361,6 +361,62 @@ void fetchAllBlocking(Path repositoryRoot) {
      * {@link BranchCatalog#merge}, which composes this with
      * {@link WorktreeService#list}.
      */
+    /**
+     * The repository's default branch -- what a review diffs against.
+     *
+     * <p>Resolved from {@code refs/remotes/origin/HEAD} when it exists,
+     * otherwise the first of {@code main} / {@code master} / {@code trunk}
+     * / {@code develop} that does, otherwise empty.
+     *
+     * <p>Deliberately <em>not</em> the main checkout's current branch. That
+     * is what Review used to use, and it made the base -- and therefore every
+     * queue item's diff -- follow whatever branch the user happened to have
+     * checked out, so a {@code git switch} in another terminal silently
+     * recomputed every review against the wrong thing.</p>
+     */
+    public CompletableFuture<Optional<String>> defaultBranch(Path repositoryRoot) {
+        return CompletableFuture.supplyAsync(() -> defaultBranchBlocking(repositoryRoot), executor);
+    }
+
+    /** Synchronous form of {@link #defaultBranch}, package-private for tests. */
+    Optional<String> defaultBranchBlocking(Path repositoryRoot) {
+        Path git = locator.locate()
+                .orElseThrow(() -> new GitExecutableNotFoundException(locator.describeSearched()));
+
+        // origin/HEAD is the repository's own statement of its default. A
+        // missing one is an ANSWER, not a failure -- a local-only repository
+        // simply has none -- so this probe tolerates the non-zero exit that
+        // runLines would otherwise throw on.
+        for (String line : runLinesAllowingFailure(git, repositoryRoot, List.of(
+                "symbolic-ref", "--quiet", "--short", "refs/remotes/origin/HEAD"))) {
+            String head = line.strip();
+            if (!head.startsWith("origin/")) {
+                continue;
+            }
+            String name = head.substring("origin/".length());
+            // The LOCAL branch when there is one, otherwise the
+            // remote-tracking ref. `git clone -b feat/x` (and deleting a
+            // local main after a merge) leaves origin/HEAD pointing at a
+            // branch with no local counterpart, and returning the bare name
+            // there would hand every diff a revision git cannot resolve.
+            return Optional.of(resolves(git, repositoryRoot, "refs/heads/" + name) ? name : head);
+        }
+        // No origin/HEAD (a local-only repository, or one never cloned): fall
+        // back to the conventional names, and only to ones that exist.
+        for (String candidate : List.of("main", "master", "trunk", "develop")) {
+            if (resolves(git, repositoryRoot, "refs/heads/" + candidate)) {
+                return Optional.of(candidate);
+            }
+        }
+        return Optional.empty();
+    }
+
+    /** Whether {@code ref} exists in {@code repositoryRoot}. */
+    private boolean resolves(Path git, Path repositoryRoot, String ref) {
+        return !runLines(git, repositoryRoot, List.of(
+                "for-each-ref", "--format=%(refname:short)", ref)).isEmpty();
+    }
+
     public CompletableFuture<BranchListing> listBranches(Path repositoryRoot) {
         return CompletableFuture.supplyAsync(() -> listBranchesBlocking(repositoryRoot), executor);
     }
@@ -394,6 +450,21 @@ BranchListing listBranchesBlocking(Path repositoryRoot) {
         return new BranchListing(List.copyOf(branches), remotes);
     }
 
+    /**
+     * As {@link #runLines}, but a non-zero exit yields no lines instead of
+     * throwing. Only for probes where "the thing is not there" is a normal
+     * answer the caller has a plan for.
+     */
+    private List<String> runLinesAllowingFailure(Path git, Path repositoryRoot, List<String> arguments) {
+        List<String> command = new ArrayList<>(List.of(git.toString(), "-C", repositoryRoot.toString()));
+        command.addAll(arguments);
+        ProcessResult result = run(command);
+        if (result.exitCode() != 0) {
+            return List.of();
+        }
+        return result.stdout().lines().map(String::strip).filter(s -> !s.isEmpty()).toList();
+    }
+
     /** Runs a read-only git subcommand in {@code repositoryRoot}, returning its non-blank stdout lines. */
     private List<String> runLines(Path git, Path repositoryRoot, List<String> arguments) {
         List<String> command = new ArrayList<>(List.of(git.toString(), "-C", repositoryRoot.toString()));