Review navigation: grouped sections, a reading order, and approvals pinned to content

app/build.gradle.kts

@@ -130,6 +130,24 @@ dependencies {
     implementation("org.jetbrains.pty4j:pty4j:0.13.10")
     runtimeOnly("org.slf4j:slf4j-jdk14:2.0.13")
 
+    // Structural parsing for the Review board's change graph (docs/superpowers/
+    // specs/2026-08-22-review-navigation-design.md §10). The core artifact
+    // bundles aarch64/x86_64 macOS, x86_64 Windows and both Linux natives --
+    // exactly the platforms this app supports -- and extracts the matching one
+    // to ~/.tree-sitter/tree-sitter-lib/ on first use. A grammar missing from
+    // the classpath is the lexical path (GrammarRegistry), not an error, so
+    // this list is a packaging decision and may differ per artifact.
+    implementation("io.github.bonede:tree-sitter:0.25.3")
+    implementation("io.github.bonede:tree-sitter-java:0.23.4")
+    implementation("io.github.bonede:tree-sitter-kotlin:0.3.8.1")
+    implementation("io.github.bonede:tree-sitter-python:0.23.4")
+    implementation("io.github.bonede:tree-sitter-javascript:0.23.1")
+    implementation("io.github.bonede:tree-sitter-typescript:0.23.2")
+    implementation("io.github.bonede:tree-sitter-go:0.23.3")
+    implementation("io.github.bonede:tree-sitter-rust:0.23.1")
+    implementation("io.github.bonede:tree-sitter-c:0.23.2")
+    implementation("io.github.bonede:tree-sitter-cpp:0.23.4")
+
     testImplementation(platform("org.junit:junit-bom:5.11.4"))
     testImplementation("org.junit.jupiter:junit-jupiter")
     testRuntimeOnly("org.junit.platform:junit-platform-launcher")

app/src/main/java/app/drydock/DrydockApplication.java

@@ -599,6 +599,14 @@ public CompletableFuture<Void> saveOpenChangedFilesInSkim(boolean value) {
                             // over an hour.
                             case "quit" -> diagQuit(primaryStage);
                             case "shot" -> diagSnapshot(primaryStage, Path.of(arg));
+                            // Documented in this hook's comment since Task 18
+                            // and never implemented until now; it silently hit
+                            // the default branch below, which prints "mark".
+                            case "reviewkey" -> mainWorkspace.diagReviewKey(arg.strip());
+                            // Documented alongside reviewkey and unwired for
+                            // just as long; ReviewDiffColumn.diagOpenComposer
+                            // was already written and had no caller.
+                            case "comment" -> mainWorkspace.diagComment();
                             // forcebanner:<commits>/<files>, or
                             // forcebanner:none for a session that never wrote
                             // a brief, or forcebanner:<c>/<f>/dead for one
@@ -688,7 +696,19 @@ public CompletableFuture<Void> saveOpenChangedFilesInSkim(boolean value) {
                             }
                             case "unwind" -> System.out.println("[diag] explorer unwind -> "
                                     + mainWorkspace.unwindExplorerOverlay());
-                            default -> System.out.println("[diag] mark " + arg);
+                            // mark is a real verb, not a fallthrough. It used to
+                            // BE the default, which is how an unwired verb --
+                            // reviewkey and comment were both documented from
+                            // Task 18 and never wired -- printed a plausible
+                            // beacon and did nothing. A driver could not tell a
+                            // synchronisation marker from a verb that does not
+                            // exist, so a run that did nothing looked like one
+                            // that worked.
+                            case "mark" -> System.out.println("[diag] mark " + arg);
+                            default -> System.out.println(
+                                    "[diag] UNKNOWN explorerScript verb '" + verb + "'"
+                                            + " -- nothing was done. Add a case in"
+                                            + " DrydockApplication or fix the script.");
                         }
                     });
                 }
@@ -1455,6 +1475,28 @@ private void diagTabStep(Stage stage, RepositorySidebar sidebar, String verb, St
                 // See the explorerScript driver for why every script has this.
                 case "quit" -> diagQuit(stage);
                 case "shot" -> diagSnapshot(stage, Path.of(arg));
+                // The Review board's out-of-diff fan-in popover, and a
+                // snapshot OF it: a Popup is its own window, so `shot` above
+                // photographs the board behind it rather than the popover.
+                case "fanin" -> System.out.println("[diag] fanin -> "
+                        + mainWorkspace.diagOpenFanIn());
+                case "popupshot" -> diagPopupSnapshot(Path.of(arg));
+                // Opening AND photographing in one FX block, not two script
+                // steps: the popover sets autoHide, and a diag run's window
+                // is not the focused one, so it closes itself the moment the
+                // pulse that opened it ends. "no popup window is showing" is
+                // what a two-step script actually captures.
+                case "faninshot" -> {
+                    // Focused FIRST. A Popup with autoHide closes itself the
+                    // instant its owner window loses (or never had) focus,
+                    // and a diag run's window is not the one the user is
+                    // looking at -- so without this the popover is gone
+                    // before the snapshot in the very same pulse.
+                    stage.toFront();
+                    stage.requestFocus();
+                    System.out.println("[diag] fanin -> " + mainWorkspace.diagOpenFanIn());
+                    diagPopupSnapshot(Path.of(arg));
+                }
                 // DIAG-ONLY, added for the sidebar row-layout visual pass: the
                 // row-overlay's hover fade and pickOnBounds=false passthrough
                 // have no other observable hook (Node.hoverProperty is driven
@@ -1504,7 +1546,13 @@ private void diagTabStep(Stage stage, RepositorySidebar sidebar, String verb, St
                 // the code. Two rounds were lost to a plausible-but-wrong
                 // theory that the picture had already contradicted.
                 case "fadeinfo" -> diagFadeInfo(sidebar, arg);
-                default -> System.out.println("[diag] mark " + arg);
+                // See the explorerScript dispatcher: mark is a verb, and an
+                // unrecognised one has to say so rather than impersonate it.
+                case "mark" -> System.out.println("[diag] mark " + arg);
+                default -> System.out.println(
+                        "[diag] UNKNOWN tabScript verb '" + verb + "'"
+                                + " -- nothing was done. Add a case in"
+                                + " DrydockApplication or fix the script.");
             }
         } catch (RuntimeException e) {
             System.out.println("[diag] tab step '" + verb + "' failed: " + e);
@@ -1660,7 +1708,34 @@ private static void diagOpenNewWorktree(MainWorkspace mainWorkspace, AppShell ap
     }
 
     private static void diagSnapshot(Stage stage, Path target) {
-        WritableImage image = stage.getScene().snapshot(null);
+        diagSnapshotScene(stage.getScene(), target);
+    }
+
+    /**
+     * Snapshots the topmost showing {@code Popup} instead of the primary
+     * stage. A popover is its own window: {@code Stage.getScene().snapshot}
+     * cannot see one at all, so without this a visual pass over the symbol
+     * lens or the out-of-diff fan-in popover would photograph the board
+     * BEHIND them and read as a clean result.
+     */
+    private static void diagPopupSnapshot(Path target) {
+        javafx.stage.Window popup = javafx.stage.Window.getWindows().stream()
+                .filter(window -> window instanceof javafx.stage.PopupWindow && window.isShowing())
+                .reduce((first, second) -> second)
+                .orElse(null);
+        if (popup == null || popup.getScene() == null) {
+            System.out.println("[diag] popupshot: no popup window is showing; windows="
+                    + javafx.stage.Window.getWindows().stream()
+                            .map(window -> window.getClass().getSimpleName() + "(showing="
+                                    + window.isShowing() + ",focused=" + window.isFocused() + ")")
+                            .toList());
+            return;
+        }
+        diagSnapshotScene(popup.getScene(), target);
+    }
+
+    private static void diagSnapshotScene(javafx.scene.Scene scene, Path target) {
+        WritableImage image = scene.snapshot(null);
         int width = (int) image.getWidth();
         int height = (int) image.getHeight();
         // The snapshot is a fresh, detached copy that nothing else references

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

@@ -728,6 +728,44 @@ public Optional<String> headCommitBlocking(Path workingDirectory) {
         return sha.isEmpty() ? Optional.empty() : Optional.of(sha);
     }
 
+    /**
+     * The commit {@code ref} names in {@code workingDirectory}, or empty when
+     * it names none -- a branch that does not exist here, a tag that was
+     * never fetched, or a directory that is not a repository.
+     *
+     * <p>Empty rather than throwing, for {@link #headCommitBlocking}'s
+     * reason: the caller is stamping or comparing metadata, and a base branch
+     * that cannot be resolved right now is an ordinary state of a fresh
+     * worktree, not a failure worth costing the caller its operation. What
+     * the caller must NOT do is fall back to the ref name -- a verdict
+     * recorded against {@code "main"} and compared against {@code "main"}
+     * would never read as stale, which is the inert no-op this method
+     * exists to end.</p>
+     *
+     * <p>{@code --end-of-options} precedes the ref because a ref may begin
+     * with {@code -} and would otherwise be read as a flag. Blocking; never
+     * call on the FX thread.</p>
+     */
+    public Optional<String> commitForRefBlocking(Path workingDirectory, String ref) {
+        Optional<Path> git = locator.locate();
+        if (git.isEmpty() || ref == null || ref.isBlank()) {
+            return Optional.empty();
+        }
+        ProcessResult result = run(List.of(git.get().toString(), "-C", workingDirectory.toString(),
+                "rev-parse", "--verify", "--end-of-options", ref + "^{commit}"));
+        if (result.exitCode() != 0) {
+            // Logged rather than folded silently into the empty result: an
+            // unresolvable base is what makes every verdict on the scope read
+            // as stale, and a reader asking why must be able to find out.
+            LOG.log(Level.WARNING, "git rev-parse --verify " + ref + " failed (exit "
+                    + result.exitCode() + ") in " + workingDirectory + ": "
+                    + ProcessRunner.excerpt(result.stderr()));
+            return Optional.empty();
+        }
+        String sha = result.stdout().strip();
+        return sha.isEmpty() ? Optional.empty() : Optional.of(sha);
+    }
+
     /** Async form of {@link #headCommitBlocking}, on this service's background executor. */
     public CompletableFuture<Optional<String>> headCommit(Path workingDirectory) {
         return CompletableFuture.supplyAsync(() -> headCommitBlocking(workingDirectory), executor);

app/src/main/java/app/drydock/mcp/McpSessionContext.java

@@ -3,6 +3,7 @@
 import app.drydock.domain.HandoffBrief;
 import app.drydock.domain.ManagedSessionId;
 import app.drydock.git.UnifiedDiff;
+import app.drydock.review.RecheckAssessment;
 import app.drydock.review.ReviewAnnotation;
 import app.drydock.review.ReviewIntent;
 import app.drydock.review.ReviewScope;
@@ -93,15 +94,51 @@ Optional<ReviewAnnotation> mutateAnnotation(ReviewAnnotation.Key key,
     /** Replaces a scope's intent grouping ({@code review_intents}). */
     void putIntents(String scopeId, List<ReviewIntent> intents);
 
+    /**
+     * {@code scopeId}'s intents over {@code diff}: the reviewer's grouping
+     * when {@link #putIntents} supplied one, otherwise the by-file fallback
+     * -- the same choice {@code SessionReviewView.Host#intents} makes for the
+     * UI. {@code review_state} joins this against {@link #verdictsOf} to
+     * report a verdict under the id an agent actually sent to {@code
+     * review_intents}, rather than whatever internal key a verdict happens
+     * to be stored under.
+     */
+    List<ReviewIntent> intentsOf(String scopeId, UnifiedDiff diff);
+
     /** Upserts findings on {@code finding.id}, so a re-run keeps existing threads. */
     void upsertFindings(List<ReviewAnnotation> findings);
 
     /** Every finding of one scope, whatever its state. */
     List<ReviewAnnotation> findingsOf(String scopeId);
 
-    /** The verdicts recorded on one scope's intents. */
+    /** The verdicts recorded on one scope's hunks (spec §9.2). */
     List<ReviewVerdict> verdictsOf(String scopeId);
 
+    /**
+     * The commit {@code scope}'s base REF resolves to right now, empty when
+     * git cannot say.
+     *
+     * <p>A commit, never the ref name, for {@link ReviewVerdict#staleAgainst}'s
+     * reason: a verdict recorded against {@code "main"} and compared against
+     * {@code "main"} could never be stale. This is the {@code toBase} half of
+     * a {@link RecheckAssessment}'s key, so it has to be the very same string
+     * the board will later ask {@code assessedAffected} with, or the recheck
+     * is stored under a key nobody reads.</p>
+     *
+     * <p>Empty rather than {@code SessionReviewView.UNRESOLVED_BASE}: the
+     * board needs a sentinel that reads as stale on a path it cannot fail,
+     * whereas {@code review_recheck} can simply refuse -- there is no base
+     * move to assess when the current base is not a commit.</p>
+     */
+    Optional<String> currentReviewBase(ReviewScope scope);
+
+    /**
+     * Records agent rechecks (spec §9.7). Decoded in full before anything is
+     * stored, like {@link #upsertFindings}: a batch with one bad entry writes
+     * nothing rather than half a recheck.
+     */
+    void putAssessments(List<RecheckAssessment> assessments);
+
     /** Whether the human has submitted this scope's review. */
     boolean reviewSubmitted(String scopeId);