diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 1e64576e..ceaf6521 100644 --- a/app/build.gradle.kts +++ b/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 diff --git a/app/src/main/java/app/drydock/DrydockApplication.java b/app/src/main/java/app/drydock/DrydockApplication.java index 3e89fcaf..4707b11a 100644 --- a/app/src/main/java/app/drydock/DrydockApplication.java +++ b/app/src/main/java/app/drydock/DrydockApplication.java @@ -18,11 +18,17 @@ import app.drydock.github.GitHubService; import app.drydock.launcher.DockIcon; import app.drydock.mcp.McpConfigWriter; +import app.drydock.mcp.McpActivityLog; import app.drydock.mcp.McpServer; import app.drydock.mcp.McpSessionRegistry; import app.drydock.mcp.McpToolRouter; import app.drydock.mcp.WorkspaceMcpSessionContext; +import app.drydock.review.AnnotationStatus; import app.drydock.review.AnnotationStore; +import app.drydock.review.Confidence; +import app.drydock.review.ReviewAnnotation; +import app.drydock.review.Severity; +import app.drydock.review.ReviewScopeRegistry; import app.drydock.search.SessionSearchService; import app.drydock.state.JsonApplicationStateRepository; import app.drydock.ui.AppShell; @@ -30,10 +36,10 @@ import app.drydock.ui.MainWorkspace; import app.drydock.ui.RemoteRepositoryModal; import app.drydock.ui.RepositorySidebar; +import app.drydock.ui.review.ReviewDestinationView; import app.drydock.ui.SettingsModal; import app.drydock.ui.SizeSetting; import app.drydock.ui.model.WorkspaceViewModel; -import app.drydock.ui.review.ReviewView; import javafx.application.Application; import javafx.application.Platform; import javafx.scene.control.Alert; @@ -64,6 +70,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.Base64; +import java.util.List; import java.util.Optional; import java.util.function.Supplier; @@ -125,6 +132,10 @@ public final class DrydockApplication extends Application { private AppShell appShell; private GitHubService gitHubService; private AnnotationStore annotationStore; + /** Cross-repo review scope handles, shared by the Review destination and the MCP tool router. */ + private ReviewScopeRegistry reviewScopeRegistry; + /** Shared by the MCP server (writer) and the Review activity panel (reader). */ + private McpActivityLog mcpActivityLog; private McpServer mcpServer; private boolean shutdownConfirmed; @@ -197,12 +208,23 @@ private void startOnFxThread(Stage primaryStage) { WorkspaceViewModel viewModel = new WorkspaceViewModel(); viewModel.setSessions(sessionManager.sessions()); + // Cross-repo review scope handles. Owned here rather than by the + // workspace because the MCP tool router addresses scopes too, and + // both must resolve the same handle to the same review. + reviewScopeRegistry = new ReviewScopeRegistry(annotationStore.scopeIdSecret()); + // Created before both readers: the Review view is built now, the MCP + // server starts below, and they must share one log. + mcpActivityLog = new McpActivityLog(); + mainWorkspace = new MainWorkspace(sessionManager, agentRegistry, repositoryManager, gitStatusService, searchService, ghCliService, worktreeService, diffService, changedLineService, annotationStore, - viewModel, primaryStage); + reviewScopeRegistry, mcpActivityLog, viewModel, primaryStage); RepositorySidebar sidebar = new RepositorySidebar(repositoryManager, gitStatusService, worktreeService, sessionManager, agentRegistry, mainWorkspace, viewModel); + sidebar.setReviewQueueSize(mainWorkspace::reviewQueueSize); + sidebar.setOpenFindingsAt(mainWorkspace::openFindingsAt); + mainWorkspace.setOnReviewQueueChanged(sidebar::refreshReviewBadges); installSessionActivityHooks(activityDir); startMcpServer(stateDir); @@ -217,9 +239,12 @@ private void startOnFxThread(Stage primaryStage) { // ghostty surface in place (no session restart needed). mainWorkspace.applyTerminalTheme(theme); }, - DEFAULT_SCENE_WIDTH, DEFAULT_SCENE_HEIGHT); + sceneWidth(), sceneHeight()); mainWorkspace.setThemeProvider(() -> appShell.themeManager().theme()); + // Review's ? button shares the one overlay, so the table stays in + // one place and cannot drift from what is actually bound. + mainWorkspace.setOnShowShortcuts(appShell::showShortcutsOverlay); mainWorkspace.setTerminalFontSizeProvider( () -> repositoryManager.state().ui().terminalFontSize()); // Warms the ghostty config cache for the pair the FIRST opened @@ -496,28 +521,50 @@ public CompletableFuture saveWorktreesDirectory(Optional directory) driver.start(); } - // Diagnostic hook for the Review tab's visual pass: switches the - // selected tab to Review and shows the working-tree diff (the only - // non-empty scope for a session opened in a repository root) after - // . Inert unless -Dapp.drydock.diag.openReview is set. + // Diagnostic hook for the Review destination's visual pass: shows + // Review and lets its queue assemble, after . Inert + // unless -Dapp.drydock.diag.openReview is set. String openReview = System.getProperty("app.drydock.diag.openReview"); if (openReview != null) { long reviewDelayMillis = (long) (Double.parseDouble(openReview.strip()) * 1000); Thread reviewOpener = new Thread(() -> { try { Thread.sleep(reviewDelayMillis); - ReviewView review = onFx(mainWorkspace::diagShowReview); - if (review == null) { - System.out.println("[diag] openReview: no Review view on the selected tab"); - return; + ReviewDestinationView review = onFx(mainWorkspace::diagShowReview); + // The queue assembles off-thread (git + gh per repo). + Thread.sleep(3_000); + System.out.println("[diag] review opened with " + + onFx(() -> review.diagItems().size()) + " queue items"); + // The diff of the selected item lands a moment later. + Thread.sleep(3_000); + System.out.println("[diag] diff column: " + onFx(review::diagDiffSummary)); + for (String line : onFx(review::diagAllItemDiffs)) { + System.out.println("[diag] item: " + line); + } + // Renders the scene graph to a PNG from inside the + // process. Unlike a screen capture this needs no OS + // screen-recording permission and cannot pick up another + // window, so it is the only visual evidence that is both + // available headlessly and guaranteed to be OUR UI. + String item = System.getProperty("app.drydock.diag.reviewItem"); + if (item != null) { + int index = Integer.parseInt(item.strip()); + onFx(() -> { + review.diagSelectItem(index); + return null; + }); + // The selected item's diff is a git process. + Thread.sleep(3_000); + System.out.println("[diag] selected item " + index); + } + if (Boolean.getBoolean("app.drydock.diag.seedFindings")) { + System.out.println("[diag] seeded: " + onFx(() -> seedFindings(review))); + Thread.sleep(1_500); + } + String shot = System.getProperty("app.drydock.diag.screenshot"); + if (shot != null) { + System.out.println("[diag] screenshot: " + onFx(() -> snapshotScene(shot))); } - // The sub-tab switch and first diff render need a few pulses. - Thread.sleep(2_000); - onFx(() -> { - review.diagSelectWorkingTree(); - return null; - }); - System.out.println("[diag] review opened on the working-tree scope"); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } catch (RuntimeException e) { @@ -661,9 +708,18 @@ private void installGlobalShortcuts(RepositorySidebar sidebar) { boolean cmd = event.isShortcutDown(); if (event.getCode() == KeyCode.ESCAPE) { + // Topmost-first (Review spec section 5): the modal, then the + // Review destination, then the tab selection. if (appShell.modalLayer().isShowingModal()) { appShell.modalLayer().close(); event.consume(); + } else if (!inTextInput && mainWorkspace.unwindReviewOverlay()) { + // Review had something open (the symbol lens, the MCP + // panel); that closes first and Review stays. + event.consume(); + } else if (!inTextInput && mainWorkspace.isReviewShowing()) { + mainWorkspace.hideReview(); + event.consume(); } else if (!inTextInput) { mainWorkspace.showPicker(); event.consume(); @@ -711,7 +767,7 @@ private void installGlobalShortcuts(RepositorySidebar sidebar) { mainWorkspace.showExplorerSubTab(); event.consume(); } else if (cmd && event.getCode() == KeyCode.DIGIT4) { - mainWorkspace.showReviewSubTab(); + mainWorkspace.showReviewForCurrentSession(); event.consume(); } else if (cmd && event.getCode() == KeyCode.R) { mainWorkspace.activeSessionId().flatMap(id -> sessionManager.sessions().stream() @@ -735,6 +791,137 @@ private void installGlobalShortcuts(RepositorySidebar sidebar) { }); } + /** + * Window size for this launch. Overridable only for the visual pass + * (-Dapp.drydock.diag.windowSize=1800x1000): Review's rails collapse at + * documented widths, so photographing them expanded means being able to + * ask for a window wide enough. + */ + private static double sceneWidth() { + return diagWindowSize().map(size -> size[0]).orElse(DEFAULT_SCENE_WIDTH); + } + + private static double sceneHeight() { + return diagWindowSize().map(size -> size[1]).orElse(DEFAULT_SCENE_HEIGHT); + } + + private static Optional diagWindowSize() { + String raw = System.getProperty("app.drydock.diag.windowSize"); + if (raw == null) { + return Optional.empty(); + } + String[] parts = raw.toLowerCase(java.util.Locale.ROOT).split("x"); + if (parts.length != 2) { + return Optional.empty(); + } + try { + return Optional.of(new double[] { + Double.parseDouble(parts[0].strip()), Double.parseDouble(parts[1].strip()) }); + } catch (NumberFormatException e) { + return Optional.empty(); + } + } + + /** + * Seeds representative findings against the selected scope, anchored to + * lines that are actually in the diff. Diagnostic-only: the findings + * margin renders severity pills, pins, patches and ASK chips, and none of + * that can be seen -- or shown to a reviewer -- without findings to + * render. + */ + private String seedFindings(ReviewDestinationView review) { + Optional scopeId = review.diagSelectedScopeId(); + if (scopeId.isEmpty()) { + return "no scope selected"; + } + List anchors = review.diagAnchors(4); + if (anchors.isEmpty()) { + return "no changed lines to anchor to"; + } + java.time.Instant now = java.time.Instant.now(); + String[] a0 = anchors.get(0); + String[] a1 = anchors.get(Math.min(1, anchors.size() - 1)); + String[] a2 = anchors.get(Math.min(2, anchors.size() - 1)); + String[] a3 = anchors.get(Math.min(3, anchors.size() - 1)); + + annotationStore.upsert(new ReviewAnnotation(scopeId.get(), "f_leak_1", Optional.empty(), + a0[0], a0[1], a0[1], Severity.BLOCKING, Confidence.HIGH, + Optional.of("Event filter never detached"), "Claude", now, + List.of(), Optional.of(new ReviewAnnotation.Patch( + "- scene.addEventFilter(MOUSE_DRAGGED, tracker);\n" + + "+ scene.removeEventFilter(MOUSE_DRAGGED, tracker);", + "one line in onRelease")), + Optional.empty(), + List.of(new ReviewAnnotation.Ask("Why is it a leak?", "Explain why this leaks."), + new ReviewAnnotation.Ask("What breaks?", "What breaks in practice?")), + List.of(new ReviewAnnotation.Message("Claude", now, + "The filter is added on every press and never removed on release -- one " + + "leaked listener per drag. After a few minutes of resizing, every " + + "mouse move runs dozens of stale trackers.")), + Optional.empty(), AnnotationStatus.OPEN)); + + annotationStore.upsert(new ReviewAnnotation(scopeId.get(), "f_clamp_2", Optional.empty(), + a1[0], a1[1], a1[1], Severity.QUESTION, Confidence.MEDIUM, + Optional.of("Width is committed unclamped"), "Claude", now, + List.of(), Optional.empty(), Optional.empty(), List.of(), + List.of(new ReviewAnnotation.Message("Claude", now, + "This commits the raw width before the clamp runs. Deliberate?"), + new ReviewAnnotation.Message("You", now, "No -- good catch, I will fix it.")), + Optional.empty(), AnnotationStatus.OPEN)); + + annotationStore.upsert(new ReviewAnnotation(scopeId.get(), "f_dev_3", Optional.empty(), + a2[0], a2[1], a2[1], Severity.DEVIATION, Confidence.HIGH, + Optional.of("Minimum width is 220, you asked for 240"), "Claude", now, + List.of(), Optional.empty(), + Optional.of(new ReviewAnnotation.DeviatesFrom("min width 240", Optional.of(9))), + List.of(), + List.of(new ReviewAnnotation.Message("Claude", now, + "Step 5 set the clamp to 240; step 7 reverted it to 220.")), + Optional.empty(), AnnotationStatus.OPEN)); + + annotationStore.upsert(new ReviewAnnotation(scopeId.get(), "f_nit_4", Optional.empty(), + a3[0], a3[1], a3[1], Severity.NIT, Confidence.UNSURE, + Optional.of("Spelling in a comment"), "Claude", now, + List.of(), Optional.empty(), Optional.empty(), List.of(), + List.of(new ReviewAnnotation.Message("Claude", now, "\"recieve\" -> \"receive\".")), + Optional.empty(), AnnotationStatus.RESOLVED)); + + return "4 findings against " + scopeId.get(); + } + + /** + * Writes the current scene to {@code path} as a PNG, on the FX thread. + * Diagnostic-only: the visual pass has no other way to see the real UI, + * because the FX layer has no headless harness inside the running app + * (docs/architecture.md). + */ + private String snapshotScene(String path) { + try { + javafx.scene.image.WritableImage image = appShell.scene().snapshot(null); + int width = (int) image.getWidth(); + int height = (int) image.getHeight(); + // Pixels are copied by hand rather than through SwingFXUtils: that + // lives in javafx.swing, which the runtime image does not carry. + // java.desktop (BufferedImage/ImageIO) is already on the module + // list, so this adds no module to the packaged app. + int[] argb = new int[width * height]; + image.getPixelReader().getPixels(0, 0, width, height, + javafx.scene.image.PixelFormat.getIntArgbInstance(), argb, 0, width); + java.awt.image.BufferedImage out = new java.awt.image.BufferedImage( + width, height, java.awt.image.BufferedImage.TYPE_INT_ARGB); + out.setRGB(0, 0, width, height, argb, 0, width); + java.io.File file = new java.io.File(path); + java.io.File parent = file.getParentFile(); + if (parent != null) { + parent.mkdirs(); + } + javax.imageio.ImageIO.write(out, "png", file); + return path + " (" + width + "x" + height + ")"; + } catch (Exception e) { + return "FAILED: " + e; + } + } + /** ⌘N target: the active tab's repository, else the first registered one. */ private Optional activeOrFirstRepository() { Optional active = mainWorkspace.activeSessionId() @@ -802,6 +989,9 @@ public void stop() { closeQuietly("sidebar-width persistence", () -> repositoryManager.updateSidebarWidth(sidebarWidth)); } } + if (mainWorkspace != null) { + closeQuietly("Review services", mainWorkspace::closeReviewServices); + } closeQuietly("UserConfig saves", UserConfig::flushPendingSaves); if (gitHubService != null) { closeQuietly("GitHubService", gitHubService::close); @@ -910,11 +1100,15 @@ private void startMcpServer(Path stateDirectory) { sessionManager::sessions, repositoryManager::repositories, annotationStore, + reviewScopeRegistry, + agentRegistry, + mainWorkspace.intentGrouping(), + diffService, gitStatusService, worktreeService, UserConfig::load, (worktree, prompt) -> mainWorkspace.startAgentSession(worktree, prompt)); - McpServer server = new McpServer(registry, new McpToolRouter(context, registry)); + McpServer server = new McpServer(registry, new McpToolRouter(context, registry), mcpActivityLog); // Published before start() so a shutdown racing startup still reaches // it. Publication alone would not be enough -- a close() that wins the // race would find nothing bound yet and no-op -- which is why diff --git a/app/src/main/java/app/drydock/git/DiffService.java b/app/src/main/java/app/drydock/git/DiffService.java index 59560213..adbf4c89 100644 --- a/app/src/main/java/app/drydock/git/DiffService.java +++ b/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 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}). + * + *

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.

+ */ + public CompletableFuture diff(Path checkoutRoot, DiffScope scope, String baseBranch, + int contextLines) { + return CompletableFuture.supplyAsync( + () -> diffBlocking(checkoutRoot, scope, baseBranch, contextLines), executor); } /** @@ -72,6 +97,13 @@ public CompletableFuture 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 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 stagedPaths) { List files = new ArrayList<>(); diff --git a/app/src/main/java/app/drydock/git/GhCliService.java b/app/src/main/java/app/drydock/git/GhCliService.java index 82746e2e..984ebd13 100644 --- a/app/src/main/java/app/drydock/git/GhCliService.java +++ b/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,8 +18,10 @@ 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; @@ -26,11 +29,12 @@ 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. * *

{@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 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 author, Optional 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 cachedExecutable; @@ -130,6 +154,110 @@ Optional 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. + * + *

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.

+ */ + public CompletableFuture> listReviewRequests(Path root) { + return CompletableFuture.supplyAsync(() -> listReviewRequestsBlocking(root), executor); + } + + List 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 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 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 author = obj.get("author") instanceof JsonObject a + && a.get("login") instanceof JsonString login + ? Optional.of(login.value()) + : Optional.empty(); + Optional 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. + * + *

Empty when {@code gh} is missing, unauthenticated, or the PR cannot + * be read; the caller shows the gate rather than an empty diff.

+ */ + public CompletableFuture> prDiff(Path root, int prNumber) { + return CompletableFuture.supplyAsync(() -> prDiffBlocking(root, prNumber), executor); + } + + Optional 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; diff --git a/app/src/main/java/app/drydock/git/GitStatusService.java b/app/src/main/java/app/drydock/git/GitStatusService.java index df82fc5b..c6f70ad3 100644 --- a/app/src/main/java/app/drydock/git/GitStatusService.java +++ b/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. + * + *

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. + * + *

Deliberately not 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.

+ */ + public CompletableFuture> defaultBranch(Path repositoryRoot) { + return CompletableFuture.supplyAsync(() -> defaultBranchBlocking(repositoryRoot), executor); + } + + /** Synchronous form of {@link #defaultBranch}, package-private for tests. */ + Optional 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 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 runLinesAllowingFailure(Path git, Path repositoryRoot, List arguments) { + List 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 runLines(Path git, Path repositoryRoot, List arguments) { List command = new ArrayList<>(List.of(git.toString(), "-C", repositoryRoot.toString())); diff --git a/app/src/main/java/app/drydock/git/PrCheckoutService.java b/app/src/main/java/app/drydock/git/PrCheckoutService.java new file mode 100644 index 00000000..49c2a118 --- /dev/null +++ b/app/src/main/java/app/drydock/git/PrCheckoutService.java @@ -0,0 +1,222 @@ +package app.drydock.git; + +import app.drydock.process.ProcessResult; +import app.drydock.process.ProcessRunner; +import app.drydock.process.ProcessTimeoutException; + +import java.io.File; +import java.io.IOException; +import java.lang.System.Logger; +import java.lang.System.Logger.Level; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Duration; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.regex.Pattern; + +/** + * Checks a GitHub pull request out into its own worktree, so reviewing it + * never disturbs the main checkout (Review handoff §6, the checkout gate). + * + *

The handoff's literal command does not exist. It gives + * the sequence as {@code gh pr checkout --worktree}; {@code gh} has no + * such flag (checked against 2.96: {@code gh pr checkout} takes + * {@code --branch}, {@code --detach}, {@code --force} and + * {@code --recurse-submodules}, and always operates on the working tree it + * is run in). Running it in the repository root would therefore move the + * main checkout onto the PR branch -- exactly what a reviewer must + * not have happen to them mid-task.

+ * + *

What this does instead, to the same end:

+ *
    + *
  1. {@code git worktree add --detach } -- a new working tree at the + * current commit, main checkout untouched;
    2. + *
  2. {@code gh pr checkout --branch } run inside that + * worktree, where {@code gh} resolves the repository from the working + * directory and checks the PR out there.
    3. + *
+ * + *

A failure at step 2 leaves a detached worktree behind, which is + * removed before the failure is reported: a half-made checkout is worse + * than none, because the next attempt would then collide with it.

+ * + *

This is deliberately not part of {@link GhCliService}, whose contract + * is read-only observation. Checking a PR out changes the working tree.

+ */ +public final class PrCheckoutService implements AutoCloseable { + + private static final Logger LOG = System.getLogger(PrCheckoutService.class.getName()); + + /** A network fetch of a whole branch; long, but never unbounded. */ + private static final Duration CHECKOUT_TIMEOUT = Duration.ofMinutes(5); + private static final Duration GIT_TIMEOUT = Duration.ofSeconds(60); + + private static final List GH_FALLBACKS = List.of( + Path.of("/usr/local/bin/gh"), + Path.of("/opt/homebrew/bin/gh")); + + /** Why a checkout could not be made. Each is a distinct, actionable message. */ + public static final class PrCheckoutException extends RuntimeException { + public PrCheckoutException(String message) { + super(message); + } + + public PrCheckoutException(String message, Throwable cause) { + super(message, cause); + } + } + + private final GitExecutableLocator locator; + private final ExecutorService executor; + private final boolean ownsExecutor; + private volatile Optional cachedGh; + + public PrCheckoutService() { + this(new GitExecutableLocator(), Executors.newVirtualThreadPerTaskExecutor(), true); + } + + public PrCheckoutService(GitExecutableLocator locator, ExecutorService executor) { + this(locator, executor, false); + } + + private PrCheckoutService(GitExecutableLocator locator, ExecutorService executor, boolean ownsExecutor) { + this.locator = locator; + this.executor = executor; + this.ownsExecutor = ownsExecutor; + } + + /** The local branch name a PR is checked out under. */ + public static String localBranchFor(int prNumber) { + return "pr-" + prNumber; + } + + /** + * Checks PR {@code prNumber} of the repository at {@code repositoryRoot} + * out into {@code worktreeDirectory}, on this service's background + * executor. + * + * @return the worktree that now holds the PR + */ + public CompletableFuture checkout(Path repositoryRoot, Path worktreeDirectory, int prNumber) { + return CompletableFuture.supplyAsync( + () -> checkoutBlocking(repositoryRoot, worktreeDirectory, prNumber), executor); + } + + /** Synchronous form; must never run on the JavaFX application thread. */ + Path checkoutBlocking(Path repositoryRoot, Path worktreeDirectory, int prNumber) { + if (prNumber <= 0) { + throw new PrCheckoutException("PR number must be positive: " + prNumber); + } + Path gh = locateGh().orElseThrow(() -> new PrCheckoutException( + "The GitHub CLI (gh) is not installed, so a pull request cannot be checked out. " + + "Install it, or use \"Read the patch only\".")); + Path git = locator.locate() + .orElseThrow(() -> new GitExecutableNotFoundException(locator.describeSearched())); + + Path worktree = worktreeDirectory.toAbsolutePath().normalize(); + if (Files.exists(worktree)) { + throw new PrCheckoutException("There is already something at " + worktree + + "; remove it or pick another directory."); + } + + // Step 1: a detached worktree at the current commit. Detached on + // purpose -- gh assigns the branch in step 2, and creating one here + // would collide with it. + ProcessResult added = run(List.of(git.toString(), "-C", repositoryRoot.toString(), + "worktree", "add", "--detach", worktree.toString()), repositoryRoot, GIT_TIMEOUT); + if (added.exitCode() != 0) { + throw new PrCheckoutException("Could not create a worktree at " + worktree + ": " + + ProcessRunner.excerpt(added.stderr())); + } + + // Step 2: gh resolves the repository from its working directory, so + // running it INSIDE the new worktree checks the PR out there and + // leaves the main checkout alone. + String branch = localBranchFor(prNumber); + try { + ProcessResult checkedOut = run(List.of(gh.toString(), "pr", "checkout", + String.valueOf(prNumber), "--branch", branch), worktree, CHECKOUT_TIMEOUT); + if (checkedOut.exitCode() == 0) { + return worktree; + } + throw new PrCheckoutException("Could not check out PR #" + prNumber + ": " + + ProcessRunner.excerpt(checkedOut.stderr())); + } catch (RuntimeException failure) { + // Timeouts, interrupts and process-launch failures from run() + // also happen after step 1 and must not strand that worktree. + removeQuietly(git, repositoryRoot, worktree); + throw failure; + } + } + + private void removeQuietly(Path git, Path repositoryRoot, Path worktree) { + try { + ProcessResult removed = run(List.of(git.toString(), "-C", repositoryRoot.toString(), + "worktree", "remove", "--force", worktree.toString()), repositoryRoot, GIT_TIMEOUT); + if (removed.exitCode() != 0) { + LOG.log(Level.WARNING, "Could not clean up the worktree at " + worktree + + " after a failed PR checkout; remove it by hand: " + + ProcessRunner.excerpt(removed.stderr())); + } + } catch (RuntimeException e) { + LOG.log(Level.WARNING, "Could not clean up the worktree at " + worktree + + " after a failed PR checkout; remove it by hand", e); + } + } + + private ProcessResult run(List command, Path workingDirectory, Duration timeout) { + try { + return ProcessRunner.run(command, workingDirectory, timeout); + } catch (IOException e) { + throw new PrCheckoutException("Could not run " + command.get(0) + ": " + e.getMessage(), e); + } catch (ProcessTimeoutException e) { + throw new PrCheckoutException(command.get(0) + " timed out after " + + timeout.toSeconds() + "s and was killed", e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new PrCheckoutException("Interrupted while running " + command.get(0), e); + } + } + + private Optional locateGh() { + Optional cached = cachedGh; + if (cached != null) { + return cached; + } + Optional found = discoverGh(); + cachedGh = found; + return found; + } + + private static Optional discoverGh() { + String pathEnv = System.getenv("PATH"); + if (pathEnv != null) { + for (String dir : pathEnv.split(Pattern.quote(File.pathSeparator))) { + if (dir.isBlank()) { + continue; + } + Path candidate = Path.of(dir).resolve("gh"); + if (Files.isRegularFile(candidate) && Files.isExecutable(candidate)) { + return Optional.of(candidate); + } + } + } + for (Path candidate : GH_FALLBACKS) { + if (Files.isRegularFile(candidate) && Files.isExecutable(candidate)) { + return Optional.of(candidate); + } + } + return Optional.empty(); + } + + @Override + public void close() { + if (ownsExecutor) { + executor.shutdown(); + } + } +} diff --git a/app/src/main/java/app/drydock/mcp/McpActivityLog.java b/app/src/main/java/app/drydock/mcp/McpActivityLog.java new file mode 100644 index 00000000..d3b9415a --- /dev/null +++ b/app/src/main/java/app/drydock/mcp/McpActivityLog.java @@ -0,0 +1,107 @@ +package app.drydock.mcp; + +import java.time.Instant; +import java.util.ArrayDeque; +import java.util.Deque; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.function.Consumer; + +/** + * A bounded ring buffer of MCP traffic, for the Review destination's + * activity panel (spec §4.7): the wiring made visible, and the thing you + * read first when a reviewer is not doing what you expected. + * + *

Bounded on purpose. A long review can make thousands of calls, and a + * panel that keeps them all would be an unbounded leak behind a UI nobody + * has open. The oldest entries fall off; the counters do not, so the budget + * bar still reflects the whole session.

+ * + *

Thread-safe: the MCP server writes from its request threads, the UI + * reads on the FX thread.

+ */ +public final class McpActivityLog { + + /** How many entries the panel can show before the oldest fall off. */ + private static final int CAPACITY = 500; + + /** Which way a call went, as the panel's arrow column shows it. */ + public enum Direction { + /** An agent wrote to drydock ({@code ←} in the panel). */ + INBOUND("←"), + /** drydock answered, or a human action was recorded ({@code →}). */ + OUTBOUND("→"); + + private final String glyph; + + Direction(String glyph) { + this.glyph = glyph; + } + + public String glyph() { + return glyph; + } + } + + /** One logged call: what it was, when, how big, and whether it failed. */ + public record Entry(Instant at, Direction direction, String tool, String detail, + Optional scopeId, int responseBytes, boolean failed) { + public Entry { + Objects.requireNonNull(at, "at"); + Objects.requireNonNull(direction, "direction"); + Objects.requireNonNull(tool, "tool"); + Objects.requireNonNull(detail, "detail"); + Objects.requireNonNull(scopeId, "scopeId"); + } + } + + private final Deque entries = new ArrayDeque<>(); + private final List> listeners = new CopyOnWriteArrayList<>(); + private long totalBytes; + private long totalCalls; + + /** Records one call. Safe to call from any thread. */ + public void record(Entry entry) { + synchronized (this) { + entries.addLast(entry); + while (entries.size() > CAPACITY) { + entries.removeFirst(); + } + totalBytes += entry.responseBytes(); + totalCalls++; + } + // Fired outside the monitor: a listener rebuilding an FX panel must + // not hold the lock the server's request threads are writing under. + for (Consumer listener : listeners) { + try { + listener.accept(entry); + } catch (RuntimeException e) { + // A panel that throws must not fail the tool call behind it. + } + } + } + + /** The entries currently held, oldest first. */ + public synchronized List entries() { + return List.copyOf(entries); + } + + /** Total response bytes over the whole session, including entries that have fallen off. */ + public synchronized long totalBytes() { + return totalBytes; + } + + /** Total calls over the whole session, including entries that have fallen off. */ + public synchronized long totalCalls() { + return totalCalls; + } + + /** Subscribes to new entries; the returned runnable unsubscribes. */ + public Runnable addListener(Consumer listener) { + Objects.requireNonNull(listener, "listener"); + listeners.add(listener); + return () -> listeners.remove(listener); + } +} diff --git a/app/src/main/java/app/drydock/mcp/McpServer.java b/app/src/main/java/app/drydock/mcp/McpServer.java index e58d7a7c..f0873ee9 100644 --- a/app/src/main/java/app/drydock/mcp/McpServer.java +++ b/app/src/main/java/app/drydock/mcp/McpServer.java @@ -16,6 +16,7 @@ import com.sun.net.httpserver.HttpServer; import java.io.IOException; +import java.time.Instant; import java.io.InputStream; import java.net.InetAddress; import java.net.InetSocketAddress; @@ -81,6 +82,13 @@ public final class McpServer implements AutoCloseable { private final McpSessionRegistry registry; private final McpToolRouter router; + /** + * The bounded traffic log the Review destination's activity panel renders + * (spec §4.7). Supplied rather than owned: the Review view is built + * before the server starts, and both must see the same log. + */ + private final McpActivityLog activityLog; + // Volatile: {@link #start()} runs on a startup virtual thread while // {@link #close()} is called from the JavaFX shutdown path, so a // non-volatile field would let a stop() racing startup read a stale null @@ -98,7 +106,17 @@ public final class McpServer implements AutoCloseable { */ private volatile boolean closed; + /** The traffic log, for the Review activity panel. */ + public McpActivityLog activityLog() { + return activityLog; + } + public McpServer(McpSessionRegistry registry, McpToolRouter router) { + this(registry, router, new McpActivityLog()); + } + + public McpServer(McpSessionRegistry registry, McpToolRouter router, McpActivityLog activityLog) { + this.activityLog = activityLog; this.registry = registry; this.router = router; } @@ -271,15 +289,50 @@ private JsonValue toolsCall(ManagedSessionId caller, JsonValue params, JsonValue try { JsonValue toolResult = router.call(caller, name, arguments); + logActivity(name, arguments, toolResult, false); return successResponse(id, toolCallResult(toolResult, false)); } catch (McpToolException e) { // A tool failure is not a transport failure: it comes back as a // 200 JSON-RPC result with isError: true, so the agent can read // and act on the message rather than the transport swallowing it. + logActivity(name, arguments, new JsonString(e.getMessage()), true); return successResponse(id, toolCallResult(new JsonString(e.getMessage()), true)); } } + /** + * Records one call in the activity log the Review panel renders. + * Never allowed to affect the call: a logging failure is not a tool + * failure, and the agent must get its answer either way. + */ + private void logActivity(String tool, JsonValue arguments, JsonValue result, boolean failed) { + try { + int bytes = JsonWriter.write(result) + .getBytes(java.nio.charset.StandardCharsets.UTF_8).length; + Optional scopeId = arguments instanceof JsonObject args + && args.get("scopeId") instanceof JsonString scope + ? Optional.of(scope.value()) + : Optional.empty(); + activityLog.record(new McpActivityLog.Entry(Instant.now(), + tool.startsWith("review_") && !tool.equals("review_scope") + && !tool.equals("review_state") + ? McpActivityLog.Direction.INBOUND + : McpActivityLog.Direction.OUTBOUND, + tool, summarize(arguments), scopeId, bytes, failed)); + } catch (RuntimeException e) { + LOG.log(Level.FINE, "Could not log MCP activity for " + tool, e); + } + } + + /** A one-line detail for the panel: the arguments, bounded. */ + private static String summarize(JsonValue arguments) { + if (arguments == null) { + return ""; + } + String text = JsonWriter.write(arguments).replaceAll("\\s+", " "); + return text.length() <= 160 ? text : text.substring(0, 159) + "…"; + } + private JsonValue initializeResult(JsonValue params) { return JsonObject.empty() .put("protocolVersion", new JsonString(negotiatedProtocolVersion(params))) diff --git a/app/src/main/java/app/drydock/mcp/McpSessionContext.java b/app/src/main/java/app/drydock/mcp/McpSessionContext.java index 928d0ca8..7fb34b62 100644 --- a/app/src/main/java/app/drydock/mcp/McpSessionContext.java +++ b/app/src/main/java/app/drydock/mcp/McpSessionContext.java @@ -1,7 +1,11 @@ package app.drydock.mcp; import app.drydock.domain.ManagedSessionId; +import app.drydock.git.UnifiedDiff; import app.drydock.review.ReviewAnnotation; +import app.drydock.review.ReviewIntent; +import app.drydock.review.ReviewScope; +import app.drydock.review.ReviewVerdict; import java.nio.file.Path; import java.util.List; @@ -43,13 +47,16 @@ public interface McpSessionContext { /** Base branch the caller's Review scope diffs against, for {@code review_comments}. */ Optional baseBranch(ManagedSessionId caller); - /** The calling session's annotations, unfiltered. */ + /** + * Every finding of every review scope the caller may address: the scopes + * bound to its session, plus any the human granted it. Unfiltered. + */ List annotations(ManagedSessionId caller); /** - * Atomically re-reads the annotation with {@code id}, applies {@code + * Atomically re-reads the finding under {@code key}, applies {@code * transform} and stores the result, then flushes so the human's view sees - * it. Empty when no annotation has that id. + * it. Empty when nothing is stored under that key. * *

A transform rather than a plain "replace this value": the human's * Review tab writes the same threads from the FX thread, so a caller that @@ -58,7 +65,51 @@ public interface McpSessionContext { * so a decision made inside it (including refusing by throwing) is made * against what is actually there.

*/ - Optional mutateAnnotation(String id, UnaryOperator transform); + Optional mutateAnnotation(ReviewAnnotation.Key key, + UnaryOperator transform); + + // ---- review scopes (Review MCP schema) ---------------------------------- + + /** + * The review scope {@code scopeId} names, if the caller may address it: + * its own session's scopes plus any the human granted with "Run review". + * Empty for an unknown scope and for one the caller may not + * touch -- the two are deliberately indistinguishable, so probing scope + * ids tells an agent nothing. + */ + Optional reviewScope(String scopeId, ManagedSessionId caller); + + /** + * The display name of the agent behind {@code caller}, for attributing + * what it writes. A finding says who found it, and a Codex session's + * findings must not be signed "Claude". + */ + String reviewerName(ManagedSessionId caller); + + /** The diff of a scope, already parsed. Runs git, so never on the FX thread. */ + UnifiedDiff reviewDiff(ReviewScope scope) throws McpToolException; + + /** Replaces a scope's intent grouping ({@code review_intents}). */ + void putIntents(String scopeId, List intents); + + /** Upserts findings on {@code finding.id}, so a re-run keeps existing threads. */ + void upsertFindings(List findings); + + /** Every finding of one scope, whatever its state. */ + List findingsOf(String scopeId); + + /** The verdicts recorded on one scope's intents. */ + List verdictsOf(String scopeId); + + /** Whether the human has submitted this scope's review. */ + boolean reviewSubmitted(String scopeId); + + /** The agent's own turns in the bound session, for the step timeline. */ + List promptHistory(ReviewScope scope); + + /** One turn of the bound session's transcript ({@code review_scope.promptHistory}). */ + record PromptStep(int step, java.time.Instant at, String prompt, String tool, List files) { + } /** * Reads {@code line} of {@code file} in the caller's worktree, with up to diff --git a/app/src/main/java/app/drydock/mcp/McpToolRouter.java b/app/src/main/java/app/drydock/mcp/McpToolRouter.java index d00c48b8..48ab41f9 100644 --- a/app/src/main/java/app/drydock/mcp/McpToolRouter.java +++ b/app/src/main/java/app/drydock/mcp/McpToolRouter.java @@ -3,8 +3,12 @@ import app.drydock.domain.ManagedSessionId; import app.drydock.git.DiffScope; import app.drydock.mcp.AnnotationLines.LineRef; +import app.drydock.git.UnifiedDiff; import app.drydock.review.AnnotationStatus; import app.drydock.review.ReviewAnnotation; +import app.drydock.review.ReviewIntent; +import app.drydock.review.ReviewScope; +import app.drydock.review.Severity; import app.drydock.state.json.JsonValue; import app.drydock.state.json.JsonValue.JsonArray; import app.drydock.state.json.JsonValue.JsonBoolean; @@ -60,23 +64,72 @@ public McpToolRouter(McpSessionContext context, McpSessionRegistry registry) { public List toolDescriptors() { return List.of( descriptor("review_comments", - "Lists this session's open (OPEN or SENT) review-annotation threads, with the " - + "base branch, decoded line number, and a working-tree excerpt so an " - + "agent can re-locate each comment as its own edits shift line numbers.", + "Lists the open (OPEN or SENT) review threads of every review scope this session " + + "may address, with the base branch, decoded line number, and a " + + "working-tree excerpt so an agent can re-locate each comment as its own " + + "edits shift line numbers.", JsonObject.empty() - .put("scope", schemaString("Diff scope to filter by: WORKING_TREE, " - + "UPSTREAM, or BASE. Omit for every scope."))), + .put("scopeId", schemaString("Review scope handle to filter by. Omit for " + + "every scope this session may address."))), descriptor("review_reply", - "Appends a Claude-authored note to a review-annotation thread. Pass " - + "addressed: true to claim the annotation as ADDRESSED; the human still " - + "confirms with RESOLVED. Refused outright for threads already RESOLVED " - + "or FIXED.", + "Appends a Claude-authored note to a review thread. Pass addressed: true to claim " + + "the thread as ADDRESSED; the human still confirms with RESOLVED. " + + "Refused outright for threads already RESOLVED or FIXED.", JsonObject.empty() - .put("id", schemaString("Id of the annotation to reply to.")) + .put("id", schemaString("Id of the finding to reply to.")) + .put("scopeId", schemaString("Review scope handle the finding belongs to. " + + "Required only when the same id exists in more than one scope " + + "this session may address.")) .put("note", schemaString("Reply text to append to the thread.")) - .put("addressed", schemaBoolean("Whether to mark the annotation ADDRESSED. " + .put("addressed", schemaBoolean("Whether to mark the thread ADDRESSED. " + "Defaults to false.")), "id", "note"), + descriptor("review_scope", + "Reads a review scope: its identity, the changed files, and the diff hunks with " + + "stable line keys. Paged -- pass the returned cursor to continue. Every " + + "anchor in review_finding is one of these line keys.", + JsonObject.empty() + .put("scopeId", schemaString("Review scope handle to read.")) + .put("cursor", schemaString("Resume token from a previous page. Omit to start.")) + .put("maxBytes", schemaString("Byte budget for this page; default " + + DEFAULT_SCOPE_BYTES + ".")), + "scopeId"), + descriptor("review_intents", + "Replaces a scope's intent grouping: what the change is trying to do, at what risk, " + + "and which hunks belong to each intent. Optional -- with no call the UI " + + "groups by file.", + JsonObject.empty() + .put("scopeId", schemaString("Review scope handle.")) + .put("intents", schemaString("Array of {id, title, kind, risk, rationale, " + + "hunkIds, collapse?, autoApprove?}.")), + "scopeId", "intents"), + descriptor("review_finding", + "Records findings against a scope. Idempotent on finding id: a re-run upserts, so " + + "existing threads, human severity overrides and resolutions survive. A " + + "patch is a PROPOSAL -- drydock never applies one; the human clicks Apply.", + JsonObject.empty() + .put("scopeId", schemaString("Review scope handle.")) + .put("findings", schemaString("Array of {id, intentId?, anchor{file, " + + "startKey, endKey?}, severity, confidence, title?, body, " + + "evidence?, patch?, deviatesFrom?, asks?}.")), + "scopeId", "findings"), + descriptor("review_answer", + "Answers a human message in a finding's thread. proposeSeverity and proposeResolve " + + "are SUGGESTIONS: the store changes only when the human accepts.", + JsonObject.empty() + .put("scopeId", schemaString("Review scope handle.")) + .put("findingId", schemaString("Finding whose thread to answer.")) + .put("body", schemaString("The answer.")) + .put("proposeSeverity", schemaString("Severity to suggest: blocking, " + + "question, deviation or nit.")) + .put("proposeResolve", schemaBoolean("Suggest that the human resolve it.")), + "scopeId", "findingId", "body"), + descriptor("review_state", + "What the human has done so far on a scope: per-intent verdicts, per-finding " + + "severity/resolution/threads, and whether the review was submitted. Read " + + "this before a re-run so settled findings are not re-flagged.", + JsonObject.empty().put("scopeId", schemaString("Review scope handle.")), + "scopeId"), descriptor("worktree_create", "Creates a new worktree for a branch in the caller's repository.", JsonObject.empty() @@ -105,6 +158,11 @@ public JsonValue call(ManagedSessionId caller, String tool, JsonValue arguments) return switch (tool) { case "review_comments" -> reviewComments(caller, arguments); case "review_reply" -> reviewReply(caller, arguments); + case "review_scope" -> reviewScope(caller, arguments); + case "review_intents" -> reviewIntents(caller, arguments); + case "review_finding" -> reviewFinding(caller, arguments); + case "review_answer" -> reviewAnswer(caller, arguments); + case "review_state" -> reviewState(caller, arguments); case "worktree_create" -> worktreeCreate(caller, arguments); case "session_start" -> sessionStart(caller, arguments); case "repos_list" -> reposList(caller); @@ -113,18 +171,185 @@ public JsonValue call(ManagedSessionId caller, String tool, JsonValue arguments) }; } + /** + * A numeric argument that may arrive as a JSON number or, from a client + * that stringifies everything, as a numeric string. Anything else falls + * back rather than failing the call: a malformed budget is not worth + * refusing a whole page over. + */ + private static int optionalIntArg(JsonObject args, String key, int fallback) { + if (args.get(key) instanceof JsonNumber number) { + return number.asInt(); + } + if (args.get(key) instanceof JsonString text) { + try { + return Integer.parseInt(text.value().strip()); + } catch (NumberFormatException e) { + return fallback; + } + } + return fallback; + } + + /** Default byte budget for one {@code review_scope} page (schema §1). */ + static final int DEFAULT_SCOPE_BYTES = 24_000; + + /** Ceiling on a caller-supplied budget, so one call cannot ask for the whole diff at once. */ + private static final int MAX_SCOPE_BYTES = 256_000; + + // ---- the review surface (Review MCP schema) ------------------------- + + /** + * Resolves the scope a call names, or refuses. Unknown and forbidden are + * one message on purpose: an agent must not be able to discover that a + * scope exists by probing handles. + */ + private ReviewScope requireScope(ManagedSessionId caller, JsonObject args) throws McpToolException { + String scopeId = requiredStringArg(args, "scopeId"); + return context.reviewScope(scopeId, caller) + .orElseThrow(() -> new McpToolException( + "No review scope '" + scopeId + "' is addressable by this session. A scope is " + + "addressable when it is bound to this session, or when the human granted " + + "it with \"Run review\".")); + } + + private JsonValue reviewScope(ManagedSessionId caller, JsonValue arguments) throws McpToolException { + requireLiveSession(caller); + JsonObject args = asObject(arguments); + ReviewScope scope = requireScope(caller, args); + + int maxBytes = Math.clamp(optionalIntArg(args, "maxBytes", DEFAULT_SCOPE_BYTES), + 1_000, MAX_SCOPE_BYTES); + UnifiedDiff diff = context.reviewDiff(scope); + ReviewToolCodec.ScopePage page = ReviewToolCodec.pageHunks(diff, + optionalStringArg(args, "cursor"), maxBytes); + + JsonObject result = JsonObject.empty() + .put("scope", ReviewToolCodec.scopeToJson(scope)) + .put("files", ReviewToolCodec.filesToJson(diff)) + .put("hunks", new JsonArray(page.hunks())) + .put("cursor", page.cursor() + .map(JsonString::new) + .orElse(JsonNull.INSTANCE)); + if (page.truncatedHunk()) { + result.put("truncated", new JsonBoolean(true)); + } + // priorThreads lets a re-run recognize its own earlier findings + // instead of duplicating them. + result.put("priorThreads", new JsonArray(context.findingsOf(scope.id()).stream() + .map(ReviewToolCodec::findingStateToJson) + .toList())); + return result; + } + + private JsonValue reviewIntents(ManagedSessionId caller, JsonValue arguments) throws McpToolException { + requireLiveSession(caller); + JsonObject args = asObject(arguments); + ReviewScope scope = requireScope(caller, args); + + List intents = ReviewToolCodec.intentsFromJson(args.get("intents")); + context.putIntents(scope.id(), intents); + return JsonObject.empty() + .put("scopeId", new JsonString(scope.id())) + .put("intents", JsonNumber.of(intents.size())); + } + + private JsonValue reviewFinding(ManagedSessionId caller, JsonValue arguments) throws McpToolException { + requireLiveSession(caller); + JsonObject args = asObject(arguments); + ReviewScope scope = requireScope(caller, args); + + if (!(args.get("findings") instanceof JsonArray array)) { + throw new McpToolException("findings must be an array"); + } + String author = context.reviewerName(caller); + List decoded = new java.util.ArrayList<>(); + for (JsonValue element : array.elements()) { + if (!(element instanceof JsonObject obj)) { + throw new McpToolException("each finding must be an object"); + } + String id = ReviewToolCodec.requireString(obj, "id"); + Optional existing = context.findingsOf(scope.id()).stream() + .filter(finding -> finding.id().equals(id)) + .findFirst(); + decoded.add(ReviewToolCodec.findingFromJson(scope.id(), obj, author, existing)); + } + // Decoded in full before anything is stored: a batch with one bad + // entry writes nothing, rather than half a review. + context.upsertFindings(decoded); + return JsonObject.empty() + .put("scopeId", new JsonString(scope.id())) + .put("findings", JsonNumber.of(decoded.size())); + } + + /** + * {@code review_answer}: appends the agent's reply to a thread. The + * {@code propose*} fields are suggestions -- they are recorded in the + * thread text and never applied, because the store changes when the human + * accepts, not when the agent asks (schema §4). + */ + private JsonValue reviewAnswer(ManagedSessionId caller, JsonValue arguments) throws McpToolException { + requireLiveSession(caller); + JsonObject args = asObject(arguments); + ReviewScope scope = requireScope(caller, args); + String findingId = requiredStringArg(args, "findingId"); + String body = PromptSafety.checkInboundText(requiredStringArg(args, "body"), "body"); + + Optional proposeSeverity = optionalStringArg(args, "proposeSeverity") + .flatMap(Severity::fromWire); + boolean proposeResolve = optionalBooleanArg(args, "proposeResolve", false); + StringBuilder text = new StringBuilder(body); + proposeSeverity.ifPresent(severity -> + text.append("\n\n[proposes severity: ").append(severity.wireName()).append("]")); + if (proposeResolve) { + text.append("\n\n[proposes resolving this finding]"); + } + + String author = context.reviewerName(caller); + ReviewAnnotation.Key key = new ReviewAnnotation.Key(scope.id(), findingId); + ReviewAnnotation updated = context.mutateAnnotation(key, current -> + current.withReply(new ReviewAnnotation.Message(author, Instant.now(), + text.toString()))) + .orElseThrow(() -> new McpToolException("No finding '" + findingId + + "' in scope '" + scope.id() + "'.")); + return JsonObject.empty() + .put("id", new JsonString(updated.id())) + .put("scopeId", new JsonString(updated.scopeId())) + .put("messages", JsonNumber.of(updated.thread().size())); + } + + private JsonValue reviewState(ManagedSessionId caller, JsonValue arguments) throws McpToolException { + requireLiveSession(caller); + JsonObject args = asObject(arguments); + ReviewScope scope = requireScope(caller, args); + + List intents = context.verdictsOf(scope.id()).stream() + .map(verdict -> (JsonValue) JsonObject.empty() + .put("id", new JsonString(verdict.intentId())) + .put("verdict", new JsonString(verdict.decision().wireName())) + .put("note", verdict.note() + .map(JsonString::new).orElse(JsonNull.INSTANCE))) + .toList(); + return JsonObject.empty() + .put("intents", new JsonArray(intents)) + .put("findings", new JsonArray(context.findingsOf(scope.id()).stream() + .map(ReviewToolCodec::findingStateToJson) + .toList())) + .put("submitted", new JsonBoolean(context.reviewSubmitted(scope.id()))); + } + // ---- review_comments ----------------------------------------------- private JsonValue reviewComments(ManagedSessionId caller, JsonValue arguments) throws McpToolException { requireLiveSession(caller); JsonObject args = asObject(arguments); - Optional scopeFilter = optionalScope(args); + Optional scopeFilter = optionalStringArg(args, "scopeId"); JsonArray comments = new JsonArray(context.annotations(caller).stream() .filter(annotation -> annotation.status() == AnnotationStatus.OPEN || annotation.status() == AnnotationStatus.SENT) - .filter(annotation -> scopeFilter.isEmpty() || annotation.scope() == scopeFilter.get()) + .filter(annotation -> scopeFilter.isEmpty() || annotation.scopeId().equals(scopeFilter.get())) .map(annotation -> toComment(caller, annotation)) .flatMap(Optional::stream) .toList()); @@ -134,19 +359,6 @@ private JsonValue reviewComments(ManagedSessionId caller, JsonValue arguments) t .put("comments", comments); } - private Optional optionalScope(JsonObject args) throws McpToolException { - Optional raw = optionalStringArg(args, "scope"); - if (raw.isEmpty()) { - return Optional.empty(); - } - try { - return Optional.of(DiffScope.valueOf(raw.get())); - } catch (IllegalArgumentException e) { - throw new McpToolException("Unknown scope '" + raw.get() - + "'; must be one of WORKING_TREE, UPSTREAM, BASE."); - } - } - private Optional toComment(ManagedSessionId caller, ReviewAnnotation annotation) { LineRef ref; try { @@ -181,7 +393,8 @@ private Optional toComment(ManagedSessionId caller, ReviewAnnotation .put("line", JsonNumber.of(ref.line())) .put("deleted_line", new JsonBoolean(ref.deleted())) .put("status", new JsonString(annotation.status().name())) - .put("scope", new JsonString(annotation.scope().name())) + .put("scopeId", new JsonString(annotation.scopeId())) + .put("severity", new JsonString(annotation.effectiveSeverity().wireName())) .put("excerpt", excerpt) .put("hint", hint) .put("thread", thread)); @@ -195,19 +408,20 @@ private JsonValue reviewReply(ManagedSessionId caller, JsonValue arguments) thro String id = requiredStringArg(args, "id"); String note = requiredStringArg(args, "note"); boolean addressed = optionalBooleanArg(args, "addressed", false); + Optional scopeId = optionalStringArg(args, "scopeId"); - // Ownership only. Which session owns an annotation cannot change, so - // unlike the status this read cannot go stale; the store's own - // by-id lookup below is what decides on current values. - context.annotations(caller).stream() - .filter(candidate -> candidate.id().equals(id)) - .findFirst() - .orElseThrow(() -> new McpToolException("No such annotation '" + id + "'.")); + // Resolves WHICH finding, not its current value. Finding ids are + // agent-chosen and repeat across scopes, so an id alone can name two + // different findings; that ambiguity is refused rather than guessed, + // because guessing is precisely the bug (scoped) keying exists to + // prevent. The store's own keyed lookup below decides on current + // values. + ReviewAnnotation.Key key = resolveFindingKey(caller, id, scopeId); ReviewAnnotation.Message reply = new ReviewAnnotation.Message("Claude", Instant.now(), note); Optional result; try { - result = context.mutateAnnotation(id, current -> { + result = context.mutateAnnotation(key, current -> { // Checked INSIDE the transform, against the stored value: the // human may have clicked Resolve between the ownership read // above and this write, and a refusal decided outside would @@ -226,12 +440,38 @@ private JsonValue reviewReply(ManagedSessionId caller, JsonValue arguments) thro } ReviewAnnotation updated = result.orElseThrow(() -> - new McpToolException("No such annotation '" + id + "'.")); + new McpToolException("No such finding '" + id + "'.")); return JsonObject.empty() .put("id", new JsonString(updated.id())) + .put("scopeId", new JsonString(updated.scopeId())) .put("status", new JsonString(updated.status().name())); } + /** + * Finds the one finding {@code id} names among the caller's addressable + * scopes. An id present in two scopes is ambiguous and is refused with + * the candidates listed, so the agent re-calls with {@code scopeId} + * rather than having drydock pick one. + */ + private ReviewAnnotation.Key resolveFindingKey(ManagedSessionId caller, String id, + Optional scopeId) throws McpToolException { + List matches = context.annotations(caller).stream() + .filter(candidate -> candidate.id().equals(id)) + .filter(candidate -> scopeId.isEmpty() || candidate.scopeId().equals(scopeId.get())) + .toList(); + if (matches.isEmpty()) { + throw new McpToolException("No such finding '" + id + "'" + + scopeId.map(scope -> " in scope '" + scope + "'").orElse("") + "."); + } + if (matches.size() > 1) { + throw new McpToolException("Finding '" + id + "' exists in more than one review scope (" + + matches.stream().map(ReviewAnnotation::scopeId).distinct().sorted() + .collect(java.util.stream.Collectors.joining(", ")) + + "); pass scopeId to say which."); + } + return matches.get(0).key(); + } + /** * Carries an {@link McpToolException} out of an annotation transform, which * cannot declare a checked exception. Never escapes {@link #reviewReply}, diff --git a/app/src/main/java/app/drydock/mcp/PromptSafety.java b/app/src/main/java/app/drydock/mcp/PromptSafety.java index e18081a2..66f964aa 100644 --- a/app/src/main/java/app/drydock/mcp/PromptSafety.java +++ b/app/src/main/java/app/drydock/mcp/PromptSafety.java @@ -20,6 +20,62 @@ public final class PromptSafety { private PromptSafety() { } + /** + * Longest text a single inbound finding field may carry. Not a security + * boundary on its own -- the store would hold more -- but a reviewer + * pasting a whole file into a title is a bug, and a bounded field keeps + * one such call from making the margin unusable. + */ + private static final int MAX_INBOUND_TEXT = 8000; + + /** + * Validates text arriving from an agent -- a finding title, body + * or evidence block (Review MCP schema, "Injection"). + * + *

The diff an agent reads is untrusted input, and a finding may quote + * it verbatim, so this text is adversarial by construction. Two distinct + * hazards, and only one of them is about rendering:

+ * + *
    + *
  • The margin renders findings as {@code Label} text, never as + * markup, so nothing here has to strip markup -- there is no + * renderer to confuse.
    • + *
  • The text can nevertheless reach a terminal later: "Ask the agent + * to fix it" and "Apply patch" type a finding's own words into a + * live session. Control characters are therefore refused here, at + * the boundary, rather than at that much later hand-off.
    • + *
+ * + *

Unlike {@link #validate}, a leading {@code !}, {@code /} or + * {@code #} is fine: those rules are about what the {@code claude} TUI + * does with a line it is typed, and a finding body is not typed as a + * line. Newlines and tabs are legitimate in a body and are allowed.

+ * + * @return {@code text} unchanged, so callers can use this inline + * @throws McpToolException if {@code text} is over-long or carries a + * control character other than newline, carriage return or tab + */ + public static String checkInboundText(String text, String field) throws McpToolException { + if (text == null) { + return null; + } + if (text.length() > MAX_INBOUND_TEXT) { + throw new McpToolException(field + " is " + text.length() + " characters; the limit is " + + MAX_INBOUND_TEXT); + } + for (int i = 0; i < text.length(); i++) { + char c = text.charAt(i); + if (c == '\n' || c == '\r' || c == '\t') { + continue; + } + if (Character.isISOControl(c)) { + throw new McpToolException(field + " must not contain control characters (found one at index " + + i + "); a finding is rendered as text and may later be typed into a terminal"); + } + } + return text; + } + /** * @throws McpToolException if {@code prompt} is blank, contains an ASCII * control character, or -- after leading whitespace is stripped -- diff --git a/app/src/main/java/app/drydock/mcp/ReviewToolCodec.java b/app/src/main/java/app/drydock/mcp/ReviewToolCodec.java new file mode 100644 index 00000000..d08b5afd --- /dev/null +++ b/app/src/main/java/app/drydock/mcp/ReviewToolCodec.java @@ -0,0 +1,422 @@ +package app.drydock.mcp; + +import app.drydock.git.UnifiedDiff; +import app.drydock.review.AnnotationStatus; +import app.drydock.review.Confidence; +import app.drydock.review.ReviewAnnotation; +import app.drydock.review.ReviewIntent; +import app.drydock.review.ReviewScope; +import app.drydock.review.Severity; +import app.drydock.state.json.JsonValue; +import app.drydock.state.json.JsonValue.JsonArray; +import app.drydock.state.json.JsonValue.JsonBoolean; +import app.drydock.state.json.JsonValue.JsonNumber; +import app.drydock.state.json.JsonValue.JsonObject; +import app.drydock.state.json.JsonValue.JsonString; + +import java.time.Instant; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; + +/** + * Encodes and decodes the Review MCP payloads (schema §§1-4), keeping the + * shape of the wire format out of {@link McpToolRouter}'s dispatch logic. + * + *

Every inbound text field goes through + * {@link PromptSafety#checkInboundText}: the diff an agent read is untrusted + * input, a finding may quote it verbatim, and that text can later be typed + * into a live terminal.

+ */ +final class ReviewToolCodec { + + private ReviewToolCodec() { + } + + // ---- review_scope (drydock -> agent) ------------------------------------ + + /** One page of {@code review_scope}: the rows that fit, and where to resume. */ + record ScopePage(List hunks, Optional cursor, boolean truncatedHunk) { + } + + static JsonValue scopeToJson(ReviewScope scope) { + JsonObject obj = JsonObject.empty(); + obj.put("id", new JsonString(scope.id())); + obj.put("kind", new JsonString(scope.kind().name().toLowerCase(java.util.Locale.ROOT))); + obj.put("repoRoot", new JsonString(scope.repoRoot().toString())); + obj.put("worktree", scope.worktree() + .map(path -> new JsonString(path.toString())) + .orElse(JsonValue.JsonNull.INSTANCE)); + obj.put("base", new JsonString(scope.base())); + obj.put("head", new JsonString(scope.head())); + obj.put("pr", scope.pr().map(pr -> { + JsonObject prObj = JsonObject.empty(); + prObj.put("number", JsonNumber.of(pr.number())); + pr.url().ifPresent(url -> prObj.put("url", new JsonString(url))); + return prObj; + }).orElse(JsonValue.JsonNull.INSTANCE)); + obj.put("sessionId", scope.sessionId() + .map(id -> new JsonString(id.toString())) + .orElse(JsonValue.JsonNull.INSTANCE)); + return obj; + } + + static JsonValue filesToJson(UnifiedDiff diff) { + List files = new ArrayList<>(); + for (UnifiedDiff.FileDiff file : diff.files()) { + JsonObject obj = JsonObject.empty(); + obj.put("path", new JsonString(file.path())); + obj.put("status", new JsonString(file.kind())); + obj.put("insertions", JsonNumber.of(file.insertions())); + obj.put("deletions", JsonNumber.of(file.deletions())); + files.add(obj); + } + return new JsonArray(files); + } + + /** + * Pages the diff's hunks under {@code maxBytes}, resuming after + * {@code cursor}. + * + *

A single hunk larger than the whole budget is returned + * truncated with {@code truncated: true}, never dropped and + * never allowed to fail the call: one generated file must not make a + * scope unreadable (schema, "Budget").

+ */ + static ScopePage pageHunks(UnifiedDiff diff, Optional cursor, int maxBytes) { + List refs = flatten(diff); + int start = cursor.map(value -> indexOf(refs, value)).orElse(0); + List page = new ArrayList<>(); + int budget = maxBytes; + boolean truncatedHunk = false; + + int i = start; + for (; i < refs.size(); i++) { + HunkRef ref = refs.get(i); + JsonObject encoded = hunkToJson(ref, budget); + int size = approximateBytes(encoded); + boolean truncated = encoded.get("truncated") instanceof JsonBoolean marker && marker.value(); + if (truncated && !page.isEmpty()) { + break; + } + page.add(encoded); + budget -= size; + truncatedHunk |= truncated; + if (truncated) { + i++; + break; + } + } + Optional next = i < refs.size() ? Optional.of(refs.get(i).id()) : Optional.empty(); + return new ScopePage(page, next, truncatedHunk); + } + + /** A hunk plus the file it came from and the stable id the cursor uses. */ + private record HunkRef(String file, int index, UnifiedDiff.Hunk hunk) { + String id() { + return "h_" + file + "_" + index; + } + } + + private static List flatten(UnifiedDiff diff) { + List refs = new ArrayList<>(); + for (UnifiedDiff.FileDiff file : diff.files()) { + int index = 0; + for (UnifiedDiff.Hunk hunk : file.hunks()) { + refs.add(new HunkRef(file.path(), index++, hunk)); + } + } + return refs; + } + + private static int indexOf(List refs, String cursor) { + for (int i = 0; i < refs.size(); i++) { + if (refs.get(i).id().equals(cursor)) { + return i; + } + } + return 0; + } + + private static JsonObject hunkToJson(HunkRef ref, int maxBytes) { + JsonObject obj = JsonObject.empty(); + obj.put("id", new JsonString(ref.id())); + obj.put("file", new JsonString(ref.file())); + List lines = ref.hunk().lines(); + obj.put("oldStart", JsonNumber.of(firstOld(lines))); + obj.put("oldCount", JsonNumber.of((int) lines.stream() + .filter(line -> line.oldLine().isPresent()).count())); + obj.put("newStart", JsonNumber.of(firstNew(lines))); + obj.put("newCount", JsonNumber.of((int) lines.stream() + .filter(line -> line.newLine().isPresent()).count())); + + List encoded = new ArrayList<>(); + int contentBudget = Math.max(0, maxBytes - 20); // reserve ,"truncated":true + for (UnifiedDiff.Line line : lines) { + JsonObject lineObj = lineToJson(line, truncateUtf8(line.text(), contentBudget)); + encoded.add(lineObj); + obj.put("lines", new JsonArray(encoded)); + if (approximateBytes(obj) > contentBudget) { + encoded.remove(encoded.size() - 1); + obj.put("lines", new JsonArray(encoded)); + obj.put("truncated", new JsonBoolean(true)); + return obj; + } + if (!line.text().equals(((JsonString) lineObj.get("text")).value())) { + obj.put("truncated", new JsonBoolean(true)); + return obj; + } + } + obj.put("lines", new JsonArray(encoded)); + return obj; + } + + private static JsonObject lineToJson(UnifiedDiff.Line line, String text) { + JsonObject lineObj = JsonObject.empty(); + lineObj.put("key", new JsonString(line.lineKey())); + lineObj.put("sign", new JsonString(switch (line.kind()) { + case ADD -> "+"; + case DEL -> "-"; + case CONTEXT -> " "; + })); + lineObj.put("old", line.oldLine().isPresent() + ? JsonNumber.of(line.oldLine().getAsInt()) : JsonValue.JsonNull.INSTANCE); + lineObj.put("new", line.newLine().isPresent() + ? JsonNumber.of(line.newLine().getAsInt()) : JsonValue.JsonNull.INSTANCE); + lineObj.put("text", new JsonString(text)); + return lineObj; + } + + /** Returns a UTF-8-bounded prefix without ever serializing a huge source line. */ + private static String truncateUtf8(String text, int maxBytes) { + if (maxBytes <= 0) { + return ""; + } + int used = 0; + int end = 0; + while (end < text.length()) { + int codePoint = text.codePointAt(end); + int bytes = codePoint <= 0x7F ? 1 : codePoint <= 0x7FF ? 2 : codePoint <= 0xFFFF ? 3 : 4; + if (used + bytes > maxBytes) { + return text.substring(0, end) + "…"; + } + used += bytes; + end += Character.charCount(codePoint); + } + return text; + } + + private static int firstOld(List lines) { + return lines.stream().filter(line -> line.oldLine().isPresent()) + .mapToInt(line -> line.oldLine().getAsInt()).min().orElse(0); + } + + private static int firstNew(List lines) { + return lines.stream().filter(line -> line.newLine().isPresent()) + .mapToInt(line -> line.newLine().getAsInt()).min().orElse(0); + } + + /** + * Byte cost of an encoded hunk, measured on its own serialization rather + * than estimated: the budget exists to keep a response under a hard limit, + * and an estimate that drifts would either waste the budget or blow it. + */ + private static int approximateBytes(JsonValue value) { + return app.drydock.state.json.JsonWriter.write(value).getBytes(java.nio.charset.StandardCharsets.UTF_8).length; + } + + // ---- review_intents (agent -> drydock) ---------------------------------- + + static List intentsFromJson(JsonValue value) throws McpToolException { + if (!(value instanceof JsonArray array)) { + throw new McpToolException("intents must be an array"); + } + List intents = new ArrayList<>(); + int number = 1; + for (JsonValue element : array.elements()) { + if (!(element instanceof JsonObject obj)) { + throw new McpToolException("each intent must be an object"); + } + String id = requireString(obj, "id"); + intents.add(new ReviewIntent(id, number++, + PromptSafety.checkInboundText(requireString(obj, "title"), "intent.title"), + optionalString(obj, "kind").flatMap(ReviewIntent.Kind::fromWire) + .orElse(ReviewIntent.Kind.CHANGE), + optionalString(obj, "risk").flatMap(ReviewIntent.Risk::fromWire) + .orElse(ReviewIntent.Risk.NONE), + PromptSafety.checkInboundText(optionalString(obj, "rationale").orElse(""), + "intent.rationale"), + stringList(obj, "hunkIds"), + collapseFromJson(obj), + obj.get("autoApprove") instanceof JsonBoolean auto && auto.value())); + } + return List.copyOf(intents); + } + + private static Optional collapseFromJson(JsonObject obj) + throws McpToolException { + if (!(obj.get("collapse") instanceof JsonObject collapse)) { + return Optional.empty(); + } + return Optional.of(new ReviewIntent.Collapse( + optionalString(collapse, "reason").orElse("generated"), + PromptSafety.checkInboundText(optionalString(collapse, "evidence").orElse(""), + "collapse.evidence"), + collapse.get("hunkCount") instanceof JsonNumber count ? count.asInt() : 0, + collapse.get("fileCount") instanceof JsonNumber count ? count.asInt() : 0)); + } + + // ---- review_finding (agent -> drydock) ---------------------------------- + + /** + * Decodes one finding. {@code existing} is the value already stored under + * the same key, if any: a re-run upserts, and the human's thread, severity + * override and resolution are theirs -- an agent re-stating its finding + * must not quietly undo them. + */ + static ReviewAnnotation findingFromJson(String scopeId, JsonObject obj, String author, + Optional existing) + throws McpToolException { + String id = requireString(obj, "id"); + if (!(obj.get("anchor") instanceof JsonObject anchor)) { + throw new McpToolException("finding '" + id + "' has no anchor"); + } + String file = requireString(anchor, "file"); + String startKey = requireString(anchor, "startKey"); + String endKey = optionalString(anchor, "endKey").orElse(startKey); + + List thread = existing.map(ReviewAnnotation::thread) + .filter(messages -> !messages.isEmpty()) + .orElse(null); + String body = PromptSafety.checkInboundText(requireString(obj, "body"), "finding.body"); + if (thread == null) { + thread = List.of(new ReviewAnnotation.Message(author, Instant.now(), body)); + } else { + // Keep the conversation, refresh the reviewer's opening statement. + List updated = new ArrayList<>(thread); + updated.set(0, new ReviewAnnotation.Message(author, updated.get(0).at(), body)); + thread = List.copyOf(updated); + } + + return new ReviewAnnotation(scopeId, id, + optionalString(obj, "intentId"), + file, startKey, endKey, + optionalString(obj, "severity").flatMap(Severity::fromWire).orElse(Severity.QUESTION), + optionalString(obj, "confidence").flatMap(Confidence::fromWire).orElse(Confidence.MEDIUM), + Optional.ofNullable(PromptSafety.checkInboundText( + optionalString(obj, "title").orElse(null), "finding.title")), + author, + existing.map(ReviewAnnotation::at).orElseGet(Instant::now), + evidenceFromJson(obj), + patchFromJson(obj), + deviatesFromJson(obj), + asksFromJson(obj), + thread, + // The human's override and status survive a re-run: they are + // the human's, not the agent's, to restate. + existing.flatMap(ReviewAnnotation::severityOverride), + existing.map(ReviewAnnotation::status).orElse(AnnotationStatus.OPEN)); + } + + private static List evidenceFromJson(JsonObject obj) + throws McpToolException { + if (!(obj.get("evidence") instanceof JsonArray array)) { + return List.of(); + } + List evidence = new ArrayList<>(); + for (JsonValue element : array.elements()) { + if (element instanceof JsonObject entry) { + evidence.add(new ReviewAnnotation.Evidence( + PromptSafety.checkInboundText(optionalString(entry, "label").orElse(""), + "evidence.label"), + PromptSafety.checkInboundText(requireString(entry, "code"), "evidence.code"), + optionalString(entry, "language").orElse("text"))); + } + } + return List.copyOf(evidence); + } + + static Optional patchFromJson(JsonObject obj) throws McpToolException { + if (!(obj.get("patch") instanceof JsonObject patch)) { + return Optional.empty(); + } + return Optional.of(new ReviewAnnotation.Patch( + PromptSafety.checkInboundText(requireString(patch, "unified"), "patch.unified"), + PromptSafety.checkInboundText(optionalString(patch, "summary").orElse(""), + "patch.summary"))); + } + + private static Optional deviatesFromJson(JsonObject obj) + throws McpToolException { + if (!(obj.get("deviatesFrom") instanceof JsonObject deviation)) { + return Optional.empty(); + } + return Optional.of(new ReviewAnnotation.DeviatesFrom( + PromptSafety.checkInboundText(requireString(deviation, "prompt"), "deviatesFrom.prompt"), + deviation.get("step") instanceof JsonNumber step + ? Optional.of(step.asInt()) : Optional.empty())); + } + + private static List asksFromJson(JsonObject obj) throws McpToolException { + if (!(obj.get("asks") instanceof JsonArray array)) { + return List.of(); + } + List asks = new ArrayList<>(); + for (JsonValue element : array.elements()) { + if (element instanceof JsonObject entry) { + asks.add(new ReviewAnnotation.Ask( + PromptSafety.checkInboundText(optionalString(entry, "label").orElse("Ask"), + "ask.label"), + PromptSafety.checkInboundText(requireString(entry, "question"), + "ask.question"))); + } + } + return List.copyOf(asks); + } + + // ---- review_state (drydock -> agent) ------------------------------------ + + static JsonValue findingStateToJson(ReviewAnnotation finding) { + JsonObject obj = JsonObject.empty(); + obj.put("id", new JsonString(finding.id())); + obj.put("severity", new JsonString(finding.effectiveSeverity().wireName())); + obj.put("resolved", new JsonBoolean(finding.resolved())); + obj.put("status", new JsonString(finding.status().name())); + List messages = new ArrayList<>(); + for (ReviewAnnotation.Message message : finding.thread()) { + JsonObject messageObj = JsonObject.empty(); + messageObj.put("actor", new JsonString(message.author())); + messageObj.put("at", new JsonString(message.at().toString())); + messageObj.put("body", new JsonString(message.text())); + messages.add(messageObj); + } + obj.put("messages", new JsonArray(messages)); + return obj; + } + + // ---- shared helpers ----------------------------------------------------- + + static String requireString(JsonObject obj, String key) throws McpToolException { + if (obj.get(key) instanceof JsonString value && !value.value().isBlank()) { + return value.value(); + } + throw new McpToolException("missing or blank string field '" + key + "'"); + } + + static Optional optionalString(JsonObject obj, String key) { + return obj.get(key) instanceof JsonString value ? Optional.of(value.value()) : Optional.empty(); + } + + private static List stringList(JsonObject obj, String key) { + if (!(obj.get(key) instanceof JsonArray array)) { + return List.of(); + } + List values = new ArrayList<>(); + for (JsonValue element : array.elements()) { + if (element instanceof JsonString value) { + values.add(value.value()); + } + } + return List.copyOf(values); + } +} diff --git a/app/src/main/java/app/drydock/mcp/WorkspaceMcpSessionContext.java b/app/src/main/java/app/drydock/mcp/WorkspaceMcpSessionContext.java index fbfb0f06..1b14e39e 100644 --- a/app/src/main/java/app/drydock/mcp/WorkspaceMcpSessionContext.java +++ b/app/src/main/java/app/drydock/mcp/WorkspaceMcpSessionContext.java @@ -17,6 +17,16 @@ import app.drydock.git.WorktreeService; import app.drydock.git.WorktreeService.Worktree; import app.drydock.review.AnnotationStore; +import app.drydock.git.DiffScope; +import app.drydock.git.DiffService; +import app.drydock.git.UnifiedDiff; +import app.drydock.agent.api.AgentRegistry; +import app.drydock.review.IntentGrouping; +import app.drydock.ui.AgentLabels; +import app.drydock.review.ReviewIntent; +import app.drydock.review.ReviewScope; +import app.drydock.review.ReviewVerdict; +import app.drydock.review.ReviewScopeRegistry; import app.drydock.review.ReviewAnnotation; import java.io.IOException; @@ -82,6 +92,10 @@ public final class WorkspaceMcpSessionContext implements McpSessionContext { private final Supplier> sessionCatalog; private final Supplier> repositoryCatalog; private final AnnotationStore annotationStore; + private final ReviewScopeRegistry reviewScopeRegistry; + private final AgentRegistry agentRegistry; + private final IntentGrouping intentGrouping; + private final DiffService diffService; private final GitStatusService gitStatusService; private final WorktreeService worktreeService; private final Supplier userConfig; @@ -97,6 +111,10 @@ public final class WorkspaceMcpSessionContext implements McpSessionContext { public WorkspaceMcpSessionContext(Supplier> sessionCatalog, Supplier> repositoryCatalog, AnnotationStore annotationStore, + ReviewScopeRegistry reviewScopeRegistry, + AgentRegistry agentRegistry, + IntentGrouping intentGrouping, + DiffService diffService, GitStatusService gitStatusService, WorktreeService worktreeService, Supplier userConfig, @@ -105,6 +123,10 @@ public WorkspaceMcpSessionContext(Supplier> sessionCat this.sessionCatalog = Objects.requireNonNull(sessionCatalog, "sessionCatalog"); this.repositoryCatalog = Objects.requireNonNull(repositoryCatalog, "repositoryCatalog"); this.annotationStore = Objects.requireNonNull(annotationStore, "annotationStore"); + this.reviewScopeRegistry = Objects.requireNonNull(reviewScopeRegistry, "reviewScopeRegistry"); + this.agentRegistry = Objects.requireNonNull(agentRegistry, "agentRegistry"); + this.intentGrouping = Objects.requireNonNull(intentGrouping, "intentGrouping"); + this.diffService = Objects.requireNonNull(diffService, "diffService"); this.gitStatusService = Objects.requireNonNull(gitStatusService, "gitStatusService"); this.worktreeService = Objects.requireNonNull(worktreeService, "worktreeService"); this.userConfig = Objects.requireNonNull(userConfig, "userConfig"); @@ -166,14 +188,102 @@ public Optional baseBranch(ManagedSessionId caller) { // ---- annotations -------------------------------------------------------- + /** + * Findings are keyed by scope handle, not by session, so this resolves + * the caller's addressable scopes first (its own, plus any granted) and + * collects their findings. A caller with no scope sees nothing rather + * than everything -- the registry is the authorization boundary. + */ @Override public List annotations(ManagedSessionId caller) { - return annotationStore.forSession(caller); + List findings = new ArrayList<>(); + for (ReviewScope scope : reviewScopeRegistry.scopes()) { + if (reviewScopeRegistry.isAddressableBy(scope.id(), caller)) { + findings.addAll(annotationStore.forScope(scope.id())); + } + } + return List.copyOf(findings); + } + + // ---- review scopes ------------------------------------------------------ + + @Override + public Optional reviewScope(String scopeId, ManagedSessionId caller) { + // Unknown and forbidden are one answer on purpose: an agent must not + // be able to learn that a scope exists by probing ids. + if (!reviewScopeRegistry.isAddressableBy(scopeId, caller)) { + return Optional.empty(); + } + return reviewScopeRegistry.byId(scopeId); + } + + @Override + public String reviewerName(ManagedSessionId caller) { + return sessionOf(caller) + .map(session -> AgentLabels.displayName(agentRegistry, session.agentKind())) + .orElse("Agent"); + } + + @Override + public UnifiedDiff reviewDiff(ReviewScope scope) throws McpToolException { + DiffScope diffScope = scope.kind() == ReviewScope.Kind.WORKING_TREE + ? DiffScope.WORKING_TREE + : DiffScope.BASE; + try { + return diffService.diff(scope.diffRoot(), diffScope, scope.base(), + DiffService.REVIEW_CONTEXT_LINES).get(JOIN_TIMEOUT_SECONDS, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new McpToolException("Interrupted while diffing " + scope.diffRoot()); + } catch (ExecutionException | CompletionException e) { + throw new McpToolException("Could not diff " + scope.diffRoot() + ": " + + (e.getCause() == null ? e.getMessage() : e.getCause().getMessage())); + } catch (TimeoutException e) { + throw new McpToolException("Timed out diffing " + scope.diffRoot()); + } + } + + @Override + public void putIntents(String scopeId, List intents) { + intentGrouping.set(scopeId, intents); + } + + @Override + public void upsertFindings(List findings) { + findings.forEach(annotationStore::upsert); + annotationStore.flushPendingSaves(); + } + + @Override + public List findingsOf(String scopeId) { + return annotationStore.forScope(scopeId); + } + + @Override + public List verdictsOf(String scopeId) { + return annotationStore.verdictsFor(scopeId); + } + + @Override + public boolean reviewSubmitted(String scopeId) { + return annotationStore.isSubmitted(scopeId); + } + + /** + * The bound session's own turns. Empty until the transcript reader is + * wired to it -- {@code review_scope} treats {@code promptHistory} as an + * optional include, so an empty list is a valid answer rather than a + * failure. + */ + @Override + public List promptHistory(ReviewScope scope) { + return List.of(); } @Override - public Optional mutateAnnotation(String id, UnaryOperator transform) { - Optional updated = annotationStore.mutate(id, transform); + public Optional mutateAnnotation(ReviewAnnotation.Key key, + UnaryOperator transform) { + Optional updated = annotationStore.mutate(key, transform); // The human's Review card refreshes off the store's change listener; // the flush is so the note survives a crash before the next autosave. updated.ifPresent(annotation -> annotationStore.flushPendingSaves()); diff --git a/app/src/main/java/app/drydock/review/AnnotationStore.java b/app/src/main/java/app/drydock/review/AnnotationStore.java index 1aa6ee8d..01da1117 100644 --- a/app/src/main/java/app/drydock/review/AnnotationStore.java +++ b/app/src/main/java/app/drydock/review/AnnotationStore.java @@ -17,11 +17,15 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardCopyOption; +import java.security.SecureRandom; import java.time.DateTimeException; import java.time.Instant; import java.util.ArrayList; +import java.util.Base64; +import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.concurrent.CopyOnWriteArrayList; @@ -35,10 +39,17 @@ import java.util.function.UnaryOperator; /** - * The per-session store of Review annotations (design handoff section C): - * in-memory, mutated on the FX thread by the Review tab, persisted as a - * JSON file alongside the application state file - * ({@code annotations.json} in the same directory as {@code state.json}). + * The store of Review findings and verdicts: in-memory, mutated on the FX + * thread by the Review destination and on its own executor by the MCP tool + * router, persisted as a JSON file alongside the application state + * file ({@code annotations.json} beside {@code state.json}). + * + *

Everything is keyed by {@code (scopeId, id)}. Finding + * and intent ids are agent-chosen and repeat across scopes -- a reviewer + * re-run against two worktrees produces {@code f_leak_1} in both -- so + * keying on the id alone made resolving a finding in one worktree resolve it + * in another. That was a real bug; {@link ReviewAnnotation.Key} is the fix, + * and {@code AnnotationStoreTest} keeps it fixed.

* *

Persisting to a sibling file -- rather than adding a member to * {@code state.json} -- keeps this store the single writer of its file: @@ -46,39 +57,68 @@ * wholesale, so a second writer there would race it. Saves run on a * single background thread (writes are serialized; the newest snapshot * wins) with the same temp-file-and-atomic-rename pattern as the state - * repository. Rapid mutations coalesce: only the newest snapshot is - * written, not one full-file rewrite per mutation. Loading is lenient: a - * missing or malformed file yields an empty store.

+ * repository. Rapid mutations coalesce. Loading is lenient: a missing or + * malformed file yields an empty store, and one malformed entry never + * discards the rest.

*/ public final class AnnotationStore implements AutoCloseable { private static final Logger LOG = System.getLogger(AnnotationStore.class.getName()); - private static final int SCHEMA_VERSION = 1; + /** + * 1 keyed findings by {@code (sessionId, DiffScope)}; 2 keys them by + * scope handle; 3 adds the secret used to derive restart-stable scope + * handles. A v1 file is migrated on read rather than dropped -- see + * {@link #legacyScopeId}. + */ + private static final int SCHEMA_VERSION = 3; + private static final SecureRandom RANDOM = new SecureRandom(); + + /** + * The scope id a pre-scope-handle finding is parked under. Scope handles + * are minted per process, so a v1 entry cannot name the handle it now + * belongs to; it keeps a deterministic placeholder instead, and {@link + * #adoptLegacy} moves it across the first time the matching scope is + * minted. The alternative -- dropping v1 entries -- would silently throw + * away real review comments on upgrade. + */ + static String legacyScopeId(ManagedSessionId sessionId, DiffScope scope) { + return "legacy:" + sessionId.value() + ":" + scope.name(); + } private final Path file; private final ExecutorService saveExecutor = Executors.newSingleThreadExecutor(runnable -> Thread.ofVirtual().unstarted(runnable)); - private final List annotations = new ArrayList<>(); - /** - * Newest-wins pending snapshot: mutations replace it, and at most one - * writer task is queued to consume it, so a burst of edits produces a - * single file write of the latest state. - */ - private final AtomicReference> pendingSnapshot = new AtomicReference<>(); + /** Findings by their composite key, in insertion order (the margin renders in this order). */ + private final Map findings = new LinkedHashMap<>(); + + /** Verdicts by {@code (scopeId, intentId)}. */ + private final Map verdicts = new LinkedHashMap<>(); + + /** Scopes whose review has been submitted. */ + private final List submitted = new ArrayList<>(); + + /** Persisted per-profile secret used to derive opaque, restart-stable scope ids. */ + private String scopeIdSecret = newScopeIdSecret(); + + private final AtomicReference pendingSnapshot = new AtomicReference<>(); /** - * Change listeners, notified after every mutation with the affected - * annotation's id (or {@code null} for a bulk change). + * Change listeners, notified after every mutation with the affected key + * (or {@code null} for a bulk change). * - *

Exists because this store now has more than one writer: the Review - * tab on the FX thread, and the MCP tool router on its own executor. A - * view that caches annotation values must be told to re-read them, or a + *

Exists because this store has more than one writer: the Review + * destination on the FX thread, and the MCP tool router on its own + * executor. A view that caches a finding must be told to re-read it, or a * later read-modify-write from the stale value silently discards the * other writer's change (AGENTS.md, "One writer for persistent state").

*/ - private final List> changeListeners = new CopyOnWriteArrayList<>(); + private final List> changeListeners = new CopyOnWriteArrayList<>(); + + private record Snapshot(List findings, List verdicts, + List submitted, String scopeIdSecret) { + } public AnnotationStore(Path file) { this.file = file.toAbsolutePath().normalize(); @@ -90,26 +130,66 @@ public static Path siblingOf(Path stateFile) { return stateFile.toAbsolutePath().normalize().resolveSibling("annotations.json"); } - // ---- queries ---- + /** A defensive copy of the profile secret used by {@link ReviewScopeRegistry}. */ + public synchronized byte[] scopeIdSecret() { + return Base64.getUrlDecoder().decode(scopeIdSecret); + } + + // ---- queries ------------------------------------------------------------ - public synchronized List forSession(ManagedSessionId sessionId) { - return annotations.stream().filter(a -> a.sessionId().equals(sessionId)).toList(); + /** Every finding of one scope, in the order they were added. */ + public synchronized List forScope(String scopeId) { + return findings.values().stream().filter(f -> f.scopeId().equals(scopeId)).toList(); } - public synchronized List forScope(ManagedSessionId sessionId, DiffScope scope) { - return annotations.stream() - .filter(a -> a.sessionId().equals(sessionId) && a.scope() == scope) + /** Every finding of one intent within one scope. */ + public synchronized List forIntent(String scopeId, String intentId) { + return findings.values().stream() + .filter(f -> f.scopeId().equals(scopeId)) + .filter(f -> f.intentId().filter(intentId::equals).isPresent()) .toList(); } - public synchronized Optional byId(String id) { - return annotations.stream().filter(a -> a.id().equals(id)).findFirst(); + public synchronized Optional byKey(ReviewAnnotation.Key key) { + return Optional.ofNullable(findings.get(key)); } - // ---- change notification ---- + public Optional byId(String scopeId, String id) { + return byKey(new ReviewAnnotation.Key(scopeId, id)); + } + + /** Open (unresolved) findings of a scope -- what the queue badge and the margin count. */ + public synchronized long openCount(String scopeId) { + return findings.values().stream() + .filter(f -> f.scopeId().equals(scopeId)) + .filter(f -> !f.resolved()) + .count(); + } + + /** Whether any unresolved finding of {@code intentId} refuses approval. */ + public synchronized boolean hasOpenBlockingFinding(String scopeId, String intentId) { + return findings.values().stream() + .filter(f -> f.scopeId().equals(scopeId)) + .filter(f -> intentId == null || f.intentId().filter(intentId::equals).isPresent()) + .anyMatch(ReviewAnnotation::blocksApproval); + } + + public synchronized Optional verdict(String scopeId, String intentId) { + return Optional.ofNullable(verdicts.get(new ReviewVerdict.Key(scopeId, intentId))); + } + + public synchronized List verdictsFor(String scopeId) { + return verdicts.values().stream().filter(v -> v.scopeId().equals(scopeId)).toList(); + } + + public synchronized boolean isSubmitted(String scopeId) { + return submitted.contains(scopeId); + } + + // ---- change notification ------------------------------------------------ /** Registers a listener; returns a handle that unregisters it. */ - public Runnable addChangeListener(Consumer listener) { + public Runnable addChangeListener(Consumer listener) { Objects.requireNonNull(listener, "listener"); changeListeners.add(listener); return () -> changeListeners.remove(listener); @@ -120,102 +200,184 @@ public Runnable addChangeListener(Consumer listener) { * logged and skipped: notification is cosmetic next to the write that just * succeeded, and one bad subscriber must not fail the others. */ - private void fireChanged(String annotationId) { - for (Consumer listener : changeListeners) { + private void fireChanged(ReviewAnnotation.Key key) { + for (Consumer listener : changeListeners) { try { - listener.accept(annotationId); + listener.accept(key); } catch (RuntimeException e) { LOG.log(Level.WARNING, "Annotation change listener failed", e); } } } - // ---- mutations (each persists asynchronously) ---- + // ---- mutations (each persists asynchronously) --------------------------- - public void add(ReviewAnnotation annotation) { - addInternal(annotation); - fireChanged(annotation.id()); + /** Adds a finding, or replaces the one already stored under its key (idempotent upsert). */ + public void upsert(ReviewAnnotation finding) { + upsertInternal(finding); + fireChanged(finding.key()); } - private synchronized void addInternal(ReviewAnnotation annotation) { - annotations.add(annotation); + private synchronized void upsertInternal(ReviewAnnotation finding) { + findings.put(finding.key(), finding); persistAsync(); } /** - * The atomic read-modify-write: re-reads the annotation with {@code id} - * under this store's monitor, applies {@code transform} to it, stores the - * result and returns it. Empty when no annotation has that id. + * The atomic read-modify-write: re-reads the finding under this store's + * monitor, applies {@code transform}, stores the result and returns it. + * Empty when no finding has that key. * - *

The only way to change a stored annotation, deliberately: a - * replace-by-id mutator alongside it would let a caller read, decide, and - * then write, giving the other writer -- the FX Review tab or the MCP tool - * router -- a window in which to land a change that the write then - * overwrites (AGENTS.md: "One writer for persistent state" names this - * read-modify-write pattern as a past data-loss bug).

+ *

The only way to change a stored finding, deliberately: a + * replace-by-key mutator alongside it would let a caller read, decide, and + * then write, giving the other writer -- the FX Review destination or the + * MCP tool router -- a window in which to land a change that the write then + * overwrites (AGENTS.md names this read-modify-write pattern as a past + * data-loss bug).

* *

{@code transform} runs while the monitor is held, so it must be short * and must not call back into this store. It may throw: an exception - * propagates to the caller with nothing written and no listener fired, - * which is how a caller refuses the mutation based on the value it was - * handed (see {@code McpToolRouter}'s {@code review_reply}). Listeners fire - * after the monitor is released, never while it is held.

+ * propagates with nothing written and no listener fired, which is how a + * caller refuses a mutation based on the value it was handed. Listeners + * fire after the monitor is released, never while it is held.

*/ - public Optional mutate(String id, UnaryOperator transform) { - Optional updated = mutateInternal(id, transform); - updated.ifPresent(annotation -> fireChanged(annotation.id())); + public Optional mutate(ReviewAnnotation.Key key, + UnaryOperator transform) { + Optional updated = mutateInternal(key, transform); + updated.ifPresent(finding -> fireChanged(finding.key())); return updated; } - private synchronized Optional mutateInternal(String id, - UnaryOperator transform) { - Objects.requireNonNull(id, "id"); + private synchronized Optional mutateInternal( + ReviewAnnotation.Key key, UnaryOperator transform) { + Objects.requireNonNull(key, "key"); Objects.requireNonNull(transform, "transform"); - for (int i = 0; i < annotations.size(); i++) { - if (annotations.get(i).id().equals(id)) { - ReviewAnnotation updated = Objects.requireNonNull(transform.apply(annotations.get(i)), - "transform must not return null"); - if (!updated.id().equals(id)) { - throw new IllegalArgumentException("transform must not change the annotation id"); - } - annotations.set(i, updated); - persistAsync(); - return Optional.of(updated); - } + ReviewAnnotation current = findings.get(key); + if (current == null) { + return Optional.empty(); } - return Optional.empty(); + ReviewAnnotation updated = Objects.requireNonNull(transform.apply(current), + "transform must not return null"); + if (!updated.key().equals(key)) { + throw new IllegalArgumentException("transform must not change the (scopeId, id) key"); + } + findings.put(key, updated); + persistAsync(); + return Optional.of(updated); } - public void remove(String id) { - if (removeInternal(id)) { - fireChanged(id); + public void remove(ReviewAnnotation.Key key) { + if (removeInternal(key)) { + fireChanged(key); } } - private synchronized boolean removeInternal(String id) { - if (annotations.removeIf(a -> a.id().equals(id))) { + private synchronized boolean removeInternal(ReviewAnnotation.Key key) { + if (findings.remove(key) != null) { persistAsync(); return true; } return false; } - /** Drops every annotation of a deleted session. */ - public void removeSession(ManagedSessionId sessionId) { - if (removeSessionInternal(sessionId)) { + /** Drops every finding and verdict of a scope that has left the queue for good. */ + public void removeScope(String scopeId) { + if (removeScopeInternal(scopeId)) { + fireChanged(null); + } + } + + private synchronized boolean removeScopeInternal(String scopeId) { + boolean changed = findings.keySet().removeIf(key -> key.scopeId().equals(scopeId)); + changed |= verdicts.keySet().removeIf(key -> key.scopeId().equals(scopeId)); + changed |= submitted.remove(scopeId); + if (changed) { + persistAsync(); + } + return changed; + } + + /** Records a per-intent verdict, replacing any previous one. */ + public void putVerdict(ReviewVerdict verdict) { + putVerdictInternal(verdict); + fireChanged(null); + } + + private synchronized void putVerdictInternal(ReviewVerdict verdict) { + verdicts.put(verdict.key(), verdict); + persistAsync(); + } + + /** {@code u}: undoes the verdict on one intent. */ + public void clearVerdict(String scopeId, String intentId) { + if (clearVerdictInternal(scopeId, intentId)) { fireChanged(null); } } - private synchronized boolean removeSessionInternal(ManagedSessionId sessionId) { - if (annotations.removeIf(a -> a.sessionId().equals(sessionId))) { + private synchronized boolean clearVerdictInternal(String scopeId, String intentId) { + if (verdicts.remove(new ReviewVerdict.Key(scopeId, intentId)) != null) { persistAsync(); return true; } return false; } - // ---- persistence ---- + /** Marks a scope's review as submitted. */ + public void markSubmitted(String scopeId) { + if (markSubmittedInternal(scopeId)) { + fireChanged(null); + } + } + + private synchronized boolean markSubmittedInternal(String scopeId) { + if (submitted.contains(scopeId)) { + return false; + } + submitted.add(scopeId); + persistAsync(); + return true; + } + + /** + * Moves findings parked under a pre-scope-handle key onto {@code scopeId} + * (see {@link #legacyScopeId}). Called when a scope is minted for a + * checkout whose session used to own those annotations, so a user's + * comments from before scope handles existed reappear against the right + * review rather than being lost. + * + * @return how many findings were adopted + */ + public int adoptLegacy(ManagedSessionId sessionId, DiffScope diffScope, String scopeId) { + int adopted = adoptLegacyInternal(legacyScopeId(sessionId, diffScope), scopeId); + if (adopted > 0) { + fireChanged(null); + } + return adopted; + } + + private synchronized int adoptLegacyInternal(String from, String to) { + List moving = findings.values().stream() + .filter(f -> f.scopeId().equals(from)) + .toList(); + if (moving.isEmpty()) { + return 0; + } + for (ReviewAnnotation finding : moving) { + findings.remove(finding.key()); + } + for (ReviewAnnotation finding : moving) { + ReviewAnnotation moved = finding.withScopeId(to); + // An id that already exists on the target scope keeps the target's + // value: the live review is the truth, and a stale legacy copy + // must never overwrite it. + findings.putIfAbsent(moved.key(), moved); + } + persistAsync(); + return moving.size(); + } + + // ---- persistence -------------------------------------------------------- /** * Blocks until every save queued so far has finished writing. For @@ -248,12 +410,13 @@ public void close() { } private void persistAsync() { - List snapshot = List.copyOf(annotations); + Snapshot snapshot = new Snapshot(List.copyOf(findings.values()), + List.copyOf(verdicts.values()), List.copyOf(submitted), scopeIdSecret); // Queue a writer task only when there is no snapshot already // pending; otherwise the queued task picks up this newer one. if (pendingSnapshot.getAndSet(snapshot) == null) { saveExecutor.execute(() -> { - List latest = pendingSnapshot.getAndSet(null); + Snapshot latest = pendingSnapshot.getAndSet(null); if (latest != null) { saveSnapshot(latest); } @@ -261,11 +424,12 @@ private void persistAsync() { } } - private void saveSnapshot(List snapshot) { + private void saveSnapshot(Snapshot snapshot) { try { Path directory = file.getParent(); Files.createDirectories(directory); - String text = JsonWriter.write(toJson(snapshot)); + String text = JsonWriter.write(toJson(snapshot.findings(), snapshot.verdicts(), + snapshot.submitted(), snapshot.scopeIdSecret())); Path tempFile = Files.createTempFile(directory, file.getFileName().toString() + ".", ".tmp"); try { Files.writeString(tempFile, text, StandardCharsets.UTF_8); @@ -284,42 +448,141 @@ private void loadFromDisk() { } try { String text = Files.readString(file, StandardCharsets.UTF_8); - annotations.addAll(fromJson(JsonParser.parse(text))); + JsonValue parsed = JsonParser.parse(text); + scopeIdSecretFromJson(parsed).ifPresent(secret -> scopeIdSecret = secret); + for (ReviewAnnotation finding : fromJson(parsed)) { + findings.put(finding.key(), finding); + } + for (ReviewVerdict verdict : verdictsFromJson(parsed)) { + verdicts.put(verdict.key(), verdict); + } + submitted.addAll(submittedFromJson(parsed)); } catch (IOException | RuntimeException e) { LOG.log(Level.WARNING, "Annotations file " + file + " is malformed; starting empty", e); } } - // ---- codec (package-private for tests) ---- + // ---- codec (package-private for tests) ---------------------------------- - static JsonValue toJson(List annotations) { + static JsonValue toJson(List findings, List verdicts, + List submitted) { + return toJson(findings, verdicts, submitted, newScopeIdSecret()); + } + + private static JsonValue toJson(List findings, List verdicts, + List submitted, String scopeIdSecret) { JsonObject root = JsonObject.empty(); root.put("schemaVersion", JsonNumber.of(SCHEMA_VERSION)); + root.put("scopeIdSecret", new JsonString(scopeIdSecret)); + List entries = new ArrayList<>(); - for (ReviewAnnotation annotation : annotations) { - JsonObject obj = JsonObject.empty(); - obj.put("id", new JsonString(annotation.id())); - obj.put("sessionId", new JsonString(annotation.sessionId().value().toString())); - obj.put("scope", new JsonString(annotation.scope().name())); - obj.put("file", new JsonString(annotation.file())); - obj.put("startKey", new JsonString(annotation.startKey())); - obj.put("endKey", new JsonString(annotation.endKey())); - obj.put("status", new JsonString(annotation.status().name())); - List thread = new ArrayList<>(); - for (ReviewAnnotation.Message message : annotation.thread()) { - JsonObject messageObj = JsonObject.empty(); - messageObj.put("author", new JsonString(message.author())); - messageObj.put("at", new JsonString(message.at().toString())); - messageObj.put("text", new JsonString(message.text())); - thread.add(messageObj); - } - obj.put("thread", new JsonArray(thread)); - entries.add(obj); + for (ReviewAnnotation finding : findings) { + entries.add(findingToJson(finding)); } root.put("annotations", new JsonArray(entries)); + + List verdictEntries = new ArrayList<>(); + for (ReviewVerdict verdict : verdicts) { + JsonObject obj = JsonObject.empty(); + obj.put("scopeId", new JsonString(verdict.scopeId())); + obj.put("intentId", new JsonString(verdict.intentId())); + obj.put("verdict", new JsonString(verdict.decision().wireName())); + verdict.note().ifPresent(note -> obj.put("note", new JsonString(note))); + obj.put("at", new JsonString(verdict.at().toString())); + verdictEntries.add(obj); + } + root.put("verdicts", new JsonArray(verdictEntries)); + + List submittedEntries = new ArrayList<>(); + for (String scopeId : submitted) { + submittedEntries.add(new JsonString(scopeId)); + } + root.put("submitted", new JsonArray(submittedEntries)); return root; } + private static Optional scopeIdSecretFromJson(JsonValue value) { + if (!(value instanceof JsonObject root) || !(root.get("scopeIdSecret") instanceof JsonString secret)) { + return Optional.empty(); + } + try { + byte[] decoded = Base64.getUrlDecoder().decode(secret.value()); + return decoded.length >= 16 ? Optional.of(secret.value()) : Optional.empty(); + } catch (IllegalArgumentException ignored) { + return Optional.empty(); + } + } + + private static String newScopeIdSecret() { + byte[] secret = new byte[32]; + RANDOM.nextBytes(secret); + return Base64.getUrlEncoder().withoutPadding().encodeToString(secret); + } + + private static JsonObject findingToJson(ReviewAnnotation finding) { + JsonObject obj = JsonObject.empty(); + obj.put("scopeId", new JsonString(finding.scopeId())); + obj.put("id", new JsonString(finding.id())); + finding.intentId().ifPresent(value -> obj.put("intentId", new JsonString(value))); + obj.put("file", new JsonString(finding.file())); + obj.put("startKey", new JsonString(finding.startKey())); + obj.put("endKey", new JsonString(finding.endKey())); + obj.put("severity", new JsonString(finding.severity().wireName())); + obj.put("confidence", new JsonString(finding.confidence().wireName())); + finding.title().ifPresent(value -> obj.put("title", new JsonString(value))); + obj.put("author", new JsonString(finding.author())); + obj.put("at", new JsonString(finding.at().toString())); + obj.put("status", new JsonString(finding.status().name())); + finding.severityOverride().ifPresent(value -> + obj.put("severityOverride", new JsonString(value.wireName()))); + + if (!finding.evidence().isEmpty()) { + obj.put("evidence", new JsonArray(finding.evidence().stream() + .map(evidence -> (JsonValue) evidenceToJson(evidence)).toList())); + } + finding.patch().ifPresent(patch -> { + JsonObject patchObj = JsonObject.empty(); + patchObj.put("unified", new JsonString(patch.unified())); + patchObj.put("summary", new JsonString(patch.summary())); + obj.put("patch", patchObj); + }); + finding.deviatesFrom().ifPresent(deviation -> { + JsonObject deviationObj = JsonObject.empty(); + deviationObj.put("prompt", new JsonString(deviation.prompt())); + deviation.step().ifPresent(step -> deviationObj.put("step", JsonNumber.of(step))); + obj.put("deviatesFrom", deviationObj); + }); + if (!finding.asks().isEmpty()) { + List asks = new ArrayList<>(); + for (ReviewAnnotation.Ask ask : finding.asks()) { + JsonObject askObj = JsonObject.empty(); + askObj.put("label", new JsonString(ask.label())); + askObj.put("question", new JsonString(ask.question())); + asks.add(askObj); + } + obj.put("asks", new JsonArray(asks)); + } + List thread = new ArrayList<>(); + for (ReviewAnnotation.Message message : finding.thread()) { + JsonObject messageObj = JsonObject.empty(); + messageObj.put("author", new JsonString(message.author())); + messageObj.put("at", new JsonString(message.at().toString())); + messageObj.put("text", new JsonString(message.text())); + message.code().ifPresent(code -> messageObj.put("code", evidenceToJson(code))); + thread.add(messageObj); + } + obj.put("thread", new JsonArray(thread)); + return obj; + } + + private static JsonObject evidenceToJson(ReviewAnnotation.Evidence evidence) { + JsonObject obj = JsonObject.empty(); + obj.put("label", new JsonString(evidence.label())); + obj.put("code", new JsonString(evidence.code())); + obj.put("language", new JsonString(evidence.language())); + return obj; + } + static List fromJson(JsonValue value) { if (!(value instanceof JsonObject root) || !(root.get("annotations") instanceof JsonArray entries)) { return List.of(); @@ -330,26 +593,7 @@ static List fromJson(JsonValue value) { continue; } try { - List thread = new ArrayList<>(); - if (obj.get("thread") instanceof JsonArray messages) { - for (JsonValue messageValue : messages.elements()) { - if (messageValue instanceof JsonObject messageObj) { - thread.add(new ReviewAnnotation.Message( - requireString(messageObj, "author"), - Instant.parse(requireString(messageObj, "at")), - requireString(messageObj, "text"))); - } - } - } - result.add(new ReviewAnnotation( - requireString(obj, "id"), - ManagedSessionId.of(requireString(obj, "sessionId")), - DiffScope.valueOf(requireString(obj, "scope").toUpperCase(Locale.ROOT)), - requireString(obj, "file"), - requireString(obj, "startKey"), - requireString(obj, "endKey"), - AnnotationStatus.fromPersisted(requireString(obj, "status")), - thread)); + result.add(findingFromJson(obj)); } catch (IllegalArgumentException | DateTimeException e) { // One malformed entry never discards the rest. LOG.log(Level.WARNING, "Skipping malformed annotation entry: " + e.getMessage()); @@ -358,10 +602,162 @@ static List fromJson(JsonValue value) { return result; } + private static ReviewAnnotation findingFromJson(JsonObject obj) { + List thread = new ArrayList<>(); + if (obj.get("thread") instanceof JsonArray messages) { + for (JsonValue messageValue : messages.elements()) { + if (messageValue instanceof JsonObject messageObj) { + thread.add(new ReviewAnnotation.Message( + requireString(messageObj, "author"), + Instant.parse(requireString(messageObj, "at")), + requireString(messageObj, "text"), + messageObj.get("code") instanceof JsonObject code + ? Optional.of(evidenceFromJson(code)) + : Optional.empty())); + } + } + } + Instant at = obj.get("at") instanceof JsonString value + ? Instant.parse(value.value()) + : thread.stream().map(ReviewAnnotation.Message::at).findFirst().orElse(Instant.EPOCH); + + return new ReviewAnnotation( + scopeIdFromJson(obj), + requireString(obj, "id"), + optionalString(obj, "intentId"), + requireString(obj, "file"), + requireString(obj, "startKey"), + requireString(obj, "endKey"), + optionalString(obj, "severity").flatMap(Severity::fromWire).orElse(Severity.QUESTION), + optionalString(obj, "confidence").flatMap(Confidence::fromWire).orElse(Confidence.HIGH), + optionalString(obj, "title"), + optionalString(obj, "author").orElseGet(() -> + thread.isEmpty() ? "You" : thread.get(0).author()), + at, + evidenceListFromJson(obj), + patchFromJson(obj), + deviatesFromJson(obj), + asksFromJson(obj), + thread, + optionalString(obj, "severityOverride").flatMap(Severity::fromWire), + AnnotationStatus.fromPersisted(requireString(obj, "status"))); + } + + /** + * A v2 entry names its scope handle. A v1 entry predates handles and + * names a session and a diff scope instead; it is parked under a + * deterministic placeholder rather than discarded (see {@link + * #legacyScopeId}). + */ + private static String scopeIdFromJson(JsonObject obj) { + Optional scopeId = optionalString(obj, "scopeId"); + if (scopeId.isPresent()) { + return scopeId.get(); + } + String sessionId = requireString(obj, "sessionId"); + DiffScope diffScope = DiffScope.valueOf(requireString(obj, "scope").toUpperCase(Locale.ROOT)); + return legacyScopeId(ManagedSessionId.of(sessionId), diffScope); + } + + private static List evidenceListFromJson(JsonObject obj) { + if (!(obj.get("evidence") instanceof JsonArray array)) { + return List.of(); + } + List evidence = new ArrayList<>(); + for (JsonValue element : array.elements()) { + if (element instanceof JsonObject entry) { + evidence.add(evidenceFromJson(entry)); + } + } + return evidence; + } + + private static ReviewAnnotation.Evidence evidenceFromJson(JsonObject obj) { + return new ReviewAnnotation.Evidence( + optionalString(obj, "label").orElse(""), + requireString(obj, "code"), + optionalString(obj, "language").orElse("text")); + } + + private static Optional patchFromJson(JsonObject obj) { + if (!(obj.get("patch") instanceof JsonObject patch)) { + return Optional.empty(); + } + return Optional.of(new ReviewAnnotation.Patch( + requireString(patch, "unified"), + optionalString(patch, "summary").orElse(""))); + } + + private static Optional deviatesFromJson(JsonObject obj) { + if (!(obj.get("deviatesFrom") instanceof JsonObject deviation)) { + return Optional.empty(); + } + Optional step = deviation.get("step") instanceof JsonNumber number + ? Optional.of(number.asInt()) + : Optional.empty(); + return Optional.of(new ReviewAnnotation.DeviatesFrom(requireString(deviation, "prompt"), step)); + } + + private static List asksFromJson(JsonObject obj) { + if (!(obj.get("asks") instanceof JsonArray array)) { + return List.of(); + } + List asks = new ArrayList<>(); + for (JsonValue element : array.elements()) { + if (element instanceof JsonObject entry) { + asks.add(new ReviewAnnotation.Ask( + optionalString(entry, "label").orElse("Ask"), + requireString(entry, "question"))); + } + } + return asks; + } + + static List verdictsFromJson(JsonValue value) { + if (!(value instanceof JsonObject root) || !(root.get("verdicts") instanceof JsonArray entries)) { + return List.of(); + } + List result = new ArrayList<>(); + for (JsonValue entryValue : entries.elements()) { + if (!(entryValue instanceof JsonObject obj)) { + continue; + } + try { + result.add(new ReviewVerdict( + requireString(obj, "scopeId"), + requireString(obj, "intentId"), + ReviewVerdict.Decision.fromWire(requireString(obj, "verdict")) + .orElseThrow(() -> new IllegalArgumentException("unknown verdict")), + optionalString(obj, "note"), + Instant.parse(requireString(obj, "at")))); + } catch (IllegalArgumentException | DateTimeException e) { + LOG.log(Level.WARNING, "Skipping malformed verdict entry: " + e.getMessage()); + } + } + return result; + } + + static List submittedFromJson(JsonValue value) { + if (!(value instanceof JsonObject root) || !(root.get("submitted") instanceof JsonArray entries)) { + return List.of(); + } + List result = new ArrayList<>(); + for (JsonValue element : entries.elements()) { + if (element instanceof JsonString scopeId) { + result.add(scopeId.value()); + } + } + return result; + } + private static String requireString(JsonObject obj, String key) { if (obj.get(key) instanceof JsonString s) { return s.value(); } throw new IllegalArgumentException("Missing or non-string field: " + key); } + + private static Optional optionalString(JsonObject obj, String key) { + return obj.get(key) instanceof JsonString s ? Optional.of(s.value()) : Optional.empty(); + } } diff --git a/app/src/main/java/app/drydock/review/Confidence.java b/app/src/main/java/app/drydock/review/Confidence.java new file mode 100644 index 00000000..9501522e --- /dev/null +++ b/app/src/main/java/app/drydock/review/Confidence.java @@ -0,0 +1,40 @@ +package app.drydock.review; + +import java.util.Locale; +import java.util.Optional; + +/** + * How sure the reviewer is of a finding (Review MCP schema §3). Rendered + * beside the anchor so a reader can weigh a confident blocking finding + * differently from an unsure one. + */ +public enum Confidence { + + HIGH("high"), + MEDIUM("medium"), + UNSURE("unsure"); + + private final String wireName; + + Confidence(String wireName) { + this.wireName = wireName; + } + + public String wireName() { + return wireName; + } + + /** Empty rather than throwing: an unknown value from an agent is a value, not a crash. */ + public static Optional fromWire(String raw) { + if (raw == null) { + return Optional.empty(); + } + String normalized = raw.strip().toLowerCase(Locale.ROOT); + for (Confidence confidence : values()) { + if (confidence.wireName.equals(normalized)) { + return Optional.of(confidence); + } + } + return Optional.empty(); + } +} diff --git a/app/src/main/java/app/drydock/review/IntentGrouping.java b/app/src/main/java/app/drydock/review/IntentGrouping.java new file mode 100644 index 00000000..e729123b --- /dev/null +++ b/app/src/main/java/app/drydock/review/IntentGrouping.java @@ -0,0 +1,104 @@ +package app.drydock.review; + +import app.drydock.git.UnifiedDiff; + +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.function.Consumer; + +/** + * Holds each scope's intent grouping: whatever {@code review_intents} + * supplied, or the by-file fallback when nothing has (Review MCP schema §2). + * + *

The fallback is what keeps Review usable with no reviewer configured: + * there is always something to settle, so the verdict bar and the submit + * flow work on a plain diff exactly as they do on a reviewed one.

+ * + *

Thread-safe: the MCP router writes on its own executor, the UI reads on + * the FX thread.

+ */ +public final class IntentGrouping { + + private final Map> byScope = new ConcurrentHashMap<>(); + private final List> listeners = new CopyOnWriteArrayList<>(); + + /** + * Replaces {@code scopeId}'s grouping with what a reviewer supplied. + * Numbering is assigned here rather than trusted from the caller, so the + * rail's {@code 1..N} is always dense and in order. + */ + public void set(String scopeId, List intents) { + Objects.requireNonNull(scopeId, "scopeId"); + List numbered = new ArrayList<>(); + int number = 1; + for (ReviewIntent intent : intents) { + numbered.add(new ReviewIntent(intent.id(), number++, intent.title(), intent.kind(), + intent.risk(), intent.rationale(), intent.hunkIds(), intent.collapse(), + intent.autoApprove())); + } + byScope.put(scopeId, List.copyOf(numbered)); + notifyChanged(scopeId); + } + + /** Drops a scope's grouping (the scope left the queue). */ + public void clear(String scopeId) { + if (byScope.remove(scopeId) != null) { + notifyChanged(scopeId); + } + } + + /** Whether a reviewer has supplied a grouping for this scope. */ + public boolean hasReviewerGrouping(String scopeId) { + return byScope.containsKey(scopeId); + } + + /** + * {@code scopeId}'s intents: the reviewer's grouping when there is one, + * otherwise one intent per changed file, derived from {@code diff}. + */ + public List intentsFor(String scopeId, UnifiedDiff diff) { + List supplied = byScope.get(scopeId); + if (supplied != null) { + return supplied; + } + return byFile(diff); + } + + /** The fallback grouping: one intent per changed file, in diff order. */ + static List byFile(UnifiedDiff diff) { + Map intents = new LinkedHashMap<>(); + int number = 1; + for (UnifiedDiff.FileDiff file : diff.files()) { + if (!intents.containsKey(file.path())) { + intents.put(file.path(), ReviewIntent.forFile(number++, file.path())); + } + } + return List.copyOf(intents.values()); + } + + /** The intent a given file belongs to, for anchoring a finding that names no intent. */ + public Optional intentForFile(String scopeId, UnifiedDiff diff, String file) { + return intentsFor(scopeId, diff).stream() + .filter(intent -> intent.id().equals("file:" + file)) + .findFirst(); + } + + /** Subscribes to grouping changes; the returned runnable unsubscribes. */ + public Runnable addChangeListener(Consumer listener) { + Objects.requireNonNull(listener, "listener"); + listeners.add(listener); + return () -> listeners.remove(listener); + } + + private void notifyChanged(String scopeId) { + for (Consumer listener : listeners) { + listener.accept(scopeId); + } + } +} diff --git a/app/src/main/java/app/drydock/review/ReviewAnnotation.java b/app/src/main/java/app/drydock/review/ReviewAnnotation.java index a344d350..9bfb2ba4 100644 --- a/app/src/main/java/app/drydock/review/ReviewAnnotation.java +++ b/app/src/main/java/app/drydock/review/ReviewAnnotation.java @@ -1,67 +1,199 @@ package app.drydock.review; -import app.drydock.domain.ManagedSessionId; -import app.drydock.git.DiffScope; - import java.time.Instant; import java.util.ArrayList; import java.util.List; import java.util.Objects; +import java.util.Optional; import java.util.UUID; /** - * One gutter annotation of the Diff Review tab (design handoff section C): - * a line range of one file in one diff scope, with a message thread and a - * status. The range is stored as stable line keys -- - * {@code n} for lines that exist in the post-image, - * {@code o} for deleted lines (see - * {@link app.drydock.git.UnifiedDiff.Line#lineKey()}) -- so annotations - * survive re-diffs of the same scope. + * One finding against a review scope: a human's annotation or a reviewer's + * finding, which are the same thing in this model and differ only by {@link + * #author()} (Review MCP schema §3). + * + *

Identity is {@code (scopeId, id)}, never {@code id} alone. + * Finding ids are agent-chosen and stable across re-runs, so the same + * id genuinely appears in two worktrees at once -- a re-run of the same + * reviewer against two branches produces {@code f_leak_1} in both. Keying on + * the id alone caused a real bug where resolving a finding in one worktree + * resolved it in another.

+ * + *

The anchor is stored as stable line keys -- {@code n} + * for lines that exist in the post-image, {@code o} for deleted + * lines (see {@link app.drydock.git.UnifiedDiff.Line#lineKey()}) -- so + * findings survive a re-diff of the same scope.

*/ public record ReviewAnnotation( + String scopeId, String id, - ManagedSessionId sessionId, - DiffScope scope, + Optional intentId, String file, String startKey, String endKey, - AnnotationStatus status, - List thread + Severity severity, + Confidence confidence, + Optional title, + String author, + Instant at, + List evidence, + Optional patch, + Optional deviatesFrom, + List asks, + List thread, + Optional severityOverride, + AnnotationStatus status ) { - /** One message of the thread; {@code author} is "You" or "Claude" (design: You/Claude avatars). */ - public record Message(String author, Instant at, String text) { + /** One message of the thread; {@code author} is "You" or the reviewer's name. */ + public record Message(String author, Instant at, String text, Optional code) { public Message { Objects.requireNonNull(author, "author"); Objects.requireNonNull(at, "at"); Objects.requireNonNull(text, "text"); + Objects.requireNonNull(code, "code"); + } + + public Message(String author, Instant at, String text) { + this(author, at, text, Optional.empty()); + } + } + + /** A labelled code excerpt supporting a finding or a reply. */ + public record Evidence(String label, String code, String language) { + public Evidence { + Objects.requireNonNull(label, "label"); + Objects.requireNonNull(code, "code"); + Objects.requireNonNull(language, "language"); + } + } + + /** + * A proposed patch. drydock never applies one: {@code Apply + * patch} is a human click. An agent that writes to the tree behind the + * review is a bug, not a feature (schema §3). + */ + public record Patch(String unified, String summary) { + public Patch { + Objects.requireNonNull(unified, "unified"); + Objects.requireNonNull(summary, "summary"); + } + } + + /** What the change departs from: the human's own instruction, and where. */ + public record DeviatesFrom(String prompt, Optional step) { + public DeviatesFrom { + Objects.requireNonNull(prompt, "prompt"); + Objects.requireNonNull(step, "step"); + } + } + + /** A prebaked follow-up the human can fire with one click (the ASK chips). */ + public record Ask(String label, String question) { + public Ask { + Objects.requireNonNull(label, "label"); + Objects.requireNonNull(question, "question"); } } public ReviewAnnotation { + Objects.requireNonNull(scopeId, "scopeId"); Objects.requireNonNull(id, "id"); - Objects.requireNonNull(sessionId, "sessionId"); - Objects.requireNonNull(scope, "scope"); + Objects.requireNonNull(intentId, "intentId"); Objects.requireNonNull(file, "file"); Objects.requireNonNull(startKey, "startKey"); Objects.requireNonNull(endKey, "endKey"); + Objects.requireNonNull(severity, "severity"); + Objects.requireNonNull(confidence, "confidence"); + Objects.requireNonNull(title, "title"); + Objects.requireNonNull(author, "author"); + Objects.requireNonNull(at, "at"); + Objects.requireNonNull(patch, "patch"); + Objects.requireNonNull(deviatesFrom, "deviatesFrom"); + Objects.requireNonNull(severityOverride, "severityOverride"); Objects.requireNonNull(status, "status"); + if (scopeId.isBlank() || id.isBlank()) { + throw new IllegalArgumentException("a finding is keyed by (scopeId, id); neither may be blank"); + } + evidence = List.copyOf(Objects.requireNonNull(evidence, "evidence")); + asks = List.copyOf(Objects.requireNonNull(asks, "asks")); thread = List.copyOf(Objects.requireNonNull(thread, "thread")); } - public static ReviewAnnotation create(ManagedSessionId sessionId, DiffScope scope, String file, - String startKey, String endKey, Message firstMessage) { - return new ReviewAnnotation(UUID.randomUUID().toString(), sessionId, scope, file, - startKey, endKey, AnnotationStatus.OPEN, List.of(firstMessage)); + /** The composite key everything is stored and looked up under. */ + public Key key() { + return new Key(scopeId, id); + } + + /** {@code (scopeId, id)} -- see the class note on why the id alone will not do. */ + public record Key(String scopeId, String id) { + public Key { + Objects.requireNonNull(scopeId, "scopeId"); + Objects.requireNonNull(id, "id"); + } + } + + /** A human annotation typed into the margin: a question at high confidence, authored by "You". */ + public static ReviewAnnotation human(String scopeId, String file, String startKey, String endKey, + Message firstMessage) { + return new ReviewAnnotation(scopeId, UUID.randomUUID().toString(), Optional.empty(), + file, startKey, endKey, Severity.QUESTION, Confidence.HIGH, Optional.empty(), + firstMessage.author(), firstMessage.at(), List.of(), Optional.empty(), Optional.empty(), + List.of(), List.of(firstMessage), Optional.empty(), AnnotationStatus.OPEN); + } + + /** The severity actually in force: the human's override when there is one, else the reviewer's. */ + public Severity effectiveSeverity() { + return severityOverride.orElse(severity); + } + + /** Whether this finding refuses approval of its intent right now. */ + public boolean blocksApproval() { + return !resolved() && effectiveSeverity().blocksApproval(); + } + + public boolean resolved() { + return status == AnnotationStatus.RESOLVED || status == AnnotationStatus.FIXED; + } + + /** The one-line label the margin card and the pin tooltip show. */ + public String displayTitle() { + return title.filter(text -> !text.isBlank()).orElseGet(() -> firstLineOfBody()); + } + + private String firstLineOfBody() { + String body = thread.isEmpty() ? "" : thread.get(0).text(); + int newline = body.indexOf('\n'); + String line = newline < 0 ? body : body.substring(0, newline); + return line.length() <= 80 ? line : line.substring(0, 79) + "…"; } public ReviewAnnotation withStatus(AnnotationStatus newStatus) { - return new ReviewAnnotation(id, sessionId, scope, file, startKey, endKey, newStatus, thread); + return new ReviewAnnotation(scopeId, id, intentId, file, startKey, endKey, severity, confidence, + title, author, at, evidence, patch, deviatesFrom, asks, thread, severityOverride, newStatus); } public ReviewAnnotation withReply(Message reply) { List extended = new ArrayList<>(thread); extended.add(reply); - return new ReviewAnnotation(id, sessionId, scope, file, startKey, endKey, status, extended); + return new ReviewAnnotation(scopeId, id, intentId, file, startKey, endKey, severity, confidence, + title, author, at, evidence, patch, deviatesFrom, asks, extended, severityOverride, status); + } + + /** + * Records a human's severity override. The reviewer's original {@link + * #severity()} is deliberately left intact: an agent re-reading the state + * must be able to see both what it said and what the human decided. + */ + public ReviewAnnotation withSeverityOverride(Severity override) { + return new ReviewAnnotation(scopeId, id, intentId, file, startKey, endKey, severity, confidence, + title, author, at, evidence, patch, deviatesFrom, asks, thread, + Optional.ofNullable(override), status); + } + + /** Re-keys this finding onto another scope (see {@code AnnotationStore.adoptLegacy}). */ + public ReviewAnnotation withScopeId(String newScopeId) { + return new ReviewAnnotation(newScopeId, id, intentId, file, startKey, endKey, severity, confidence, + title, author, at, evidence, patch, deviatesFrom, asks, thread, severityOverride, status); } } diff --git a/app/src/main/java/app/drydock/review/ReviewIntent.java b/app/src/main/java/app/drydock/review/ReviewIntent.java new file mode 100644 index 00000000..6264bb4e --- /dev/null +++ b/app/src/main/java/app/drydock/review/ReviewIntent.java @@ -0,0 +1,128 @@ +package app.drydock.review; + +import java.util.List; +import java.util.Locale; +import java.util.Objects; +import java.util.Optional; + +/** + * One intent: a group of hunks the reviewer says belong together, with its + * risk and rationale (Review MCP schema §2). + * + *

Intents are what the human settles, so there is always a set of them -- + * with no {@code review_intents} call the UI falls back to one intent per + * file (schema §2), which is what keeps the verdict bar meaningful with no + * reviewer configured.

+ */ +public record ReviewIntent( + String id, + int number, + String title, + Kind kind, + Risk risk, + String rationale, + List hunkIds, + Optional collapse, + boolean autoApprove) { + + /** What kind of change this intent is; drives the tag beside its title. */ + public enum Kind { + CHANGE("change"), REFACTOR("refactor"), MOVE("move"), + TESTS("tests"), GENERATED("generated"), CONFIG("config"); + + private final String wireName; + + Kind(String wireName) { + this.wireName = wireName; + } + + public String wireName() { + return wireName; + } + + public static Optional fromWire(String raw) { + return lookup(values(), Kind::wireName, raw); + } + } + + /** The intent's risk, which drives its heat bar. */ + public enum Risk { + HIGH("HIGH"), MED("MED"), LOW("LOW"), NONE("NONE"); + + private final String wireName; + + Risk(String wireName) { + this.wireName = wireName; + } + + public String wireName() { + return wireName; + } + + /** The {@code app.css} modifier class for this risk's heat bar. */ + public String styleClass() { + return "risk-" + wireName.toLowerCase(Locale.ROOT); + } + + public static Optional fromWire(String raw) { + return lookup(values(), Risk::wireName, raw); + } + } + + /** + * The agent's assertion that a large hunk count is structurally + * equivalent -- a pure rename, a pure move, or generated output -- plus + * how it checked. drydock renders the assertion and keeps the hunks one + * click away; it never verifies the claim itself. + */ + public record Collapse(String reason, String evidence, int hunkCount, int fileCount) { + public Collapse { + Objects.requireNonNull(reason, "reason"); + Objects.requireNonNull(evidence, "evidence"); + } + } + + public ReviewIntent { + Objects.requireNonNull(id, "id"); + Objects.requireNonNull(title, "title"); + Objects.requireNonNull(kind, "kind"); + Objects.requireNonNull(risk, "risk"); + Objects.requireNonNull(rationale, "rationale"); + Objects.requireNonNull(collapse, "collapse"); + if (id.isBlank()) { + throw new IllegalArgumentException("intent id must not be blank"); + } + hunkIds = List.copyOf(Objects.requireNonNull(hunkIds, "hunkIds")); + } + + /** + * Collapsed intents (pure renames, moves, generated output) do not count + * toward review progress -- the point of the collapse is that there is + * nothing to read, so requiring a verdict on it would be busywork. + */ + public boolean countsTowardProgress() { + return collapse.isEmpty(); + } + + /** The by-file fallback used when no reviewer has supplied a grouping. */ + public static ReviewIntent forFile(int number, String path) { + return new ReviewIntent("file:" + path, number, path, Kind.CHANGE, Risk.NONE, + "Grouped by file — no reviewer has proposed intents for this scope.", + List.of(), Optional.empty(), false); + } + + private static > Optional lookup(E[] values, + java.util.function.Function wire, + String raw) { + if (raw == null) { + return Optional.empty(); + } + String normalized = raw.strip(); + for (E value : values) { + if (wire.apply(value).equalsIgnoreCase(normalized)) { + return Optional.of(value); + } + } + return Optional.empty(); + } +} diff --git a/app/src/main/java/app/drydock/review/ReviewItem.java b/app/src/main/java/app/drydock/review/ReviewItem.java new file mode 100644 index 00000000..5353bd51 --- /dev/null +++ b/app/src/main/java/app/drydock/review/ReviewItem.java @@ -0,0 +1,69 @@ +package app.drydock.review; + +import java.util.Objects; +import java.util.Optional; + +/** + * One row of the Review queue: a {@link ReviewScope} plus what the rail + * renders for it (spec §4.1). + * + *

The open-finding count is deliberately not a field. It is + * derived from the findings that actually exist for the scope, so an item + * whose reviewer has never run shows no count at all -- rather than a + * confident zero, which would read as "reviewed, nothing found".

+ */ +public record ReviewItem(ReviewScope scope, Group group, String title, String subtitle) { + + /** The queue's four sections, in rail order. */ + public enum Group { + /** The human's own uncommitted work and local branches. */ + MINE("MINE"), + /** Worktrees an agent authored. */ + AGENTS("AGENTS"), + /** PRs where a review was requested from this user. */ + REQUESTED("REQUESTED"), + /** Dependent PR stacks. */ + STACK("STACK"); + + private final String label; + + Group(String label) { + this.label = label; + } + + public String label() { + return label; + } + } + + public ReviewItem { + Objects.requireNonNull(scope, "scope"); + Objects.requireNonNull(group, "group"); + Objects.requireNonNull(title, "title"); + Objects.requireNonNull(subtitle, "subtitle"); + } + + /** The rail's icon column glyph, keyed to the scope kind (spec §4.1). */ + public String icon() { + return switch (scope.kind()) { + case WORKING_TREE -> "❯_"; + case BRANCH -> "⎇"; + case WORKTREE -> "◫"; + case PR -> "◧"; + case STACK -> "⛁"; + }; + } + + /** The collapsed rail's tooltip: the full description a 44px column cannot show. */ + public String tooltip() { + String session = scope.sessionId() + .map(id -> " · session bound") + .orElse(" · no session yet"); + return title + " — " + subtitle + session; + } + + /** The PR number, when this item is one (used by the checkout gate). */ + public Optional prNumber() { + return scope.pr().map(ReviewScope.PullRequestRef::number); + } +} diff --git a/app/src/main/java/app/drydock/review/ReviewQueueService.java b/app/src/main/java/app/drydock/review/ReviewQueueService.java new file mode 100644 index 00000000..3800cbe8 --- /dev/null +++ b/app/src/main/java/app/drydock/review/ReviewQueueService.java @@ -0,0 +1,300 @@ +package app.drydock.review; + +import app.drydock.domain.ManagedSessionId; +import app.drydock.git.GhCliService; +import app.drydock.git.GitBranchState; +import app.drydock.git.GitStatus; +import app.drydock.git.GitStatusService; +import app.drydock.git.WorktreeService; + +import java.lang.System.Logger; +import java.lang.System.Logger.Level; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.CompletableFuture; +import java.util.function.Function; +import java.util.stream.Collectors; + +/** + * Assembles the Review queue (spec §4.1) from what drydock already knows: + * {@code git worktree list}, {@code git status}, and {@code gh pr list + * --search "review-requested:@me"}. + * + *

Every scope it produces is minted through {@link ReviewScopeRegistry}, + * so the same worktree keeps the same handle across rescans and everything + * keyed by {@code (scopeId, …)} survives a background refresh.

+ * + *

Nothing here blocks and nothing here fails the queue. + * A repository whose git commands fail, or a missing {@code gh}, contributes + * no items and is logged; the rest of the queue still assembles. Review + * degrades, never blocks (spec §6).

+ */ +public final class ReviewQueueService { + + private static final Logger LOG = System.getLogger(ReviewQueueService.class.getName()); + + /** A repository to scan: its main checkout and the name the queue shows. */ + public record RepositoryTarget(Path root, String displayName) { + public RepositoryTarget { + Objects.requireNonNull(root, "root"); + Objects.requireNonNull(displayName, "displayName"); + } + } + + /** + * Resolves the managed session running in a checkout, if any. Supplied + * by the workspace rather than read here, so this service depends on no + * session bookkeeping -- and so tests can drive the MINE/AGENTS split + * without a {@code SessionManager}. + */ + @FunctionalInterface + public interface SessionLookup { + Optional sessionAt(Path checkoutRoot); + } + + /** + * Where the REQUESTED group comes from -- in the app, {@code + * GhCliService::listReviewRequests}. A narrow function rather than the + * service itself, because "{@code gh} is absent or failing" is a state + * the queue must survive and therefore a state tests have to be able to + * produce, and {@code GhCliService} discovers its executable from + * {@code PATH} with no seam to take it away. + */ + @FunctionalInterface + public interface ReviewRequestSource { + CompletableFuture> forRepository(Path repositoryRoot); + } + + private final WorktreeService worktreeService; + private final GitStatusService gitStatusService; + private final ReviewRequestSource reviewRequests; + private final ReviewScopeRegistry scopeRegistry; + + public ReviewQueueService(WorktreeService worktreeService, GitStatusService gitStatusService, + ReviewRequestSource reviewRequests, ReviewScopeRegistry scopeRegistry) { + this.worktreeService = Objects.requireNonNull(worktreeService, "worktreeService"); + this.gitStatusService = Objects.requireNonNull(gitStatusService, "gitStatusService"); + this.reviewRequests = Objects.requireNonNull(reviewRequests, "reviewRequests"); + this.scopeRegistry = Objects.requireNonNull(scopeRegistry, "scopeRegistry"); + } + + /** + * Assembles the whole queue, grouped MINE · AGENTS · REQUESTED · STACK. + * Repositories are scanned concurrently; the future completes when all + * of them have either produced items or failed. + * + *

Handles for scopes confirmed no longer in the queue are revoked, so + * a worktree that was removed stops being addressable over MCP. A failed + * scan is not evidence that an existing scope departed.

+ */ + public CompletableFuture> assemble(List repositories, + SessionLookup sessions) { + Objects.requireNonNull(sessions, "sessions"); + List> perRepository = repositories.stream() + .map(repository -> assembleRepository(repository, sessions)) + .toList(); + return CompletableFuture.allOf(perRepository.toArray(CompletableFuture[]::new)) + .thenApply(ignored -> { + List scans = perRepository.stream().map(CompletableFuture::join).toList(); + List items = scans.stream() + .flatMap(scan -> scan.items().stream()) + .sorted(Comparator.comparingInt(item -> item.group().ordinal())) + .toList(); + revokeDepartedScopes(scans); + return items; + }); + } + + /** + * One repository's items. Worktrees, the main checkout's status and the + * review-requested PRs are fetched concurrently, then combined -- the + * PR list is the slow one (a network call), and it must not serialize + * behind two local git commands. + */ + private CompletableFuture assembleRepository(RepositoryTarget repository, + SessionLookup sessions) { + CompletableFuture>> worktrees = + worktreeService.list(repository.root()).handle((value, failure) -> { + if (failure == null) { + return new Fetch<>(value, true); + } + LOG.log(Level.DEBUG, "Could not list worktrees of " + repository.root(), failure); + return new Fetch<>(List.of(), false); + }); + CompletableFuture>> status = + gitStatusService.getStatus(repository.root()) + .>thenApply(Optional::of) + .handle((value, failure) -> { + if (failure == null) { + return new Fetch<>(value, true); + } + LOG.log(Level.DEBUG, "Could not read status of " + repository.root(), failure); + return new Fetch<>(Optional.empty(), false); + }); + // The base every review diffs against: the repository's own default + // branch, NOT whatever the main checkout happens to be on. Deriving + // it from the current branch made a `git switch` in another terminal + // silently recompute every queue item's diff. + CompletableFuture>> defaultBranch = + gitStatusService.defaultBranch(repository.root()).handle((value, failure) -> { + if (failure == null) { + return new Fetch<>(value, true); + } + LOG.log(Level.DEBUG, "Could not resolve the default branch of " + + repository.root(), failure); + return new Fetch<>(Optional.empty(), false); + }); + CompletableFuture>> requests = + reviewRequests.forRepository(repository.root()).handle((value, failure) -> { + if (failure == null) { + return new Fetch<>(value, true); + } + LOG.log(Level.DEBUG, "Could not list review requests for " + repository.root(), failure); + return new Fetch<>(List.of(), false); + }); + + return worktrees.thenCombine(status, (trees, mainStatus) -> + new PartialScan(trees.value(), mainStatus.value(), Optional.empty(), + trees.complete() && mainStatus.complete())) + .thenCombine(defaultBranch, (scan, branch) -> + new PartialScan(scan.worktrees(), scan.status(), branch.value(), + scan.localComplete() && branch.complete())) + .thenCombine(requests, (scan, prs) -> new RepositoryScan(repository, + build(repository, sessions, scan, prs.value()), scan.localComplete(), prs.complete())); + } + + private record Fetch(T value, boolean complete) { } + + private record PartialScan(List worktrees, Optional status, + Optional defaultBranch, boolean localComplete) { } + + private record RepositoryScan(RepositoryTarget repository, List items, + boolean localComplete, boolean requestsComplete) { } + + private List build(RepositoryTarget repository, SessionLookup sessions, + PartialScan scan, List requests) { + String base = baseBranchOf(scan.defaultBranch(), scan.status()); + List items = new ArrayList<>(); + + // MINE -- the main checkout's uncommitted work, when there is any. + if (scan.status().map(GitStatus::dirty).orElse(false)) { + ReviewScope scope = scopeRegistry.mint(ReviewScopeRegistry.spec( + ReviewScope.Kind.WORKING_TREE, repository.root(), Optional.of(repository.root()), + base, base, Optional.empty(), sessions.sessionAt(repository.root()))); + items.add(new ReviewItem(scope, ReviewItem.Group.MINE, "Working tree", + repository.displayName() + " · uncommitted changes")); + } + + // MINE / AGENTS -- one item per non-main worktree. A worktree with a + // bound session is an agent's; one without is the human's own. + for (WorktreeService.Worktree worktree : scan.worktrees()) { + if (worktree.mainCheckout() || worktree.prunable()) { + continue; + } + String head = worktree.branch().orElse(worktree.detached() ? "(detached)" : "(no branch)"); + Optional session = sessions.sessionAt(worktree.path()); + ReviewScope scope = scopeRegistry.mint(ReviewScopeRegistry.spec( + ReviewScope.Kind.WORKTREE, repository.root(), Optional.of(worktree.path()), + base, head, Optional.empty(), session)); + items.add(new ReviewItem(scope, + session.isPresent() ? ReviewItem.Group.AGENTS : ReviewItem.Group.MINE, + head, repository.displayName() + " · vs " + base)); + } + + // REQUESTED -- PRs asking this user for a review. A PR whose head + // branch is already checked out in a worktree is that worktree's + // item; listing it twice would split its findings across two scopes. + Set checkedOutBranches = scan.worktrees().stream() + .map(WorktreeService.Worktree::branch) + .flatMap(Optional::stream) + .collect(Collectors.toUnmodifiableSet()); + for (GhCliService.ReviewRequest request : requests) { + if (checkedOutBranches.contains(request.headRefName())) { + continue; + } + ReviewScope scope = scopeRegistry.mint(ReviewScopeRegistry.spec( + ReviewScope.Kind.PR, repository.root(), Optional.empty(), + request.baseRefName(), request.headRefName(), + Optional.of(new ReviewScope.PullRequestRef(request.number(), request.url())), + Optional.empty())); + items.add(new ReviewItem(scope, ReviewItem.Group.REQUESTED, + "PR #" + request.number() + " " + request.headRefName(), + prSubtitle(repository, request))); + } + + return List.copyOf(items); + } + + private static String prSubtitle(RepositoryTarget repository, GhCliService.ReviewRequest request) { + StringBuilder subtitle = new StringBuilder(repository.displayName()); + request.author().ifPresent(author -> subtitle.append(" · @").append(author)); + if (request.changedFiles() > 0) { + subtitle.append(" · ").append(request.changedFiles()) + .append(request.changedFiles() == 1 ? " file" : " files"); + } + if (request.draft()) { + subtitle.append(" · draft"); + } + return subtitle.append(" · not checked out").toString(); + } + + /** + * The branch every worktree in this repository is reviewed against: the + * repository's default branch. + * + *

It used to be the main checkout's current branch. That made the base + * -- and so every queue item's diff -- follow whatever the user happened + * to have checked out, so switching branches in another terminal silently + * recomputed every review against a different thing, leaving findings + * anchored by line key pointing at unrelated code.

+ * + *

The current branch is still the last resort, for a repository with + * no {@code origin/HEAD} and none of the conventional default names; a + * detached checkout with neither leaves {@code HEAD}.

+ */ + private static String baseBranchOf(Optional defaultBranch, Optional status) { + return defaultBranch + .or(() -> status.map(GitStatus::branch) + .filter(GitBranchState.OnBranch.class::isInstance) + .map(branch -> ((GitBranchState.OnBranch) branch).name())) + .orElse("HEAD"); + } + + /** + * Revokes handles only when the source authoritative for their kind + * completed successfully and no longer lists them. Without this a pruned + * worktree's handle would stay addressable over MCP; treating a failed + * command as an empty result would instead revoke live scopes. + */ + private void revokeDepartedScopes(List scans) { + for (ReviewScope scope : scopeRegistry.scopes()) { + Optional scan = scans.stream() + .filter(candidate -> candidate.repository().root().toAbsolutePath().normalize() + .equals(scope.repoRoot().toAbsolutePath().normalize())) + .findFirst(); + if (scan.isEmpty()) { + continue; + } + boolean sourceCompleted = scope.kind() == ReviewScope.Kind.PR + ? scan.get().requestsComplete() + : scan.get().localComplete(); + boolean stillLive = scan.get().items().stream() + .anyMatch(item -> item.scope().id().equals(scope.id())); + if (sourceCompleted && !stillLive) { + scopeRegistry.revoke(scope.id()); + } + } + } + + /** Exposed for the sidebar's per-worktree {@code ◨n} badge lookup. */ + public Function> scopeByWorktree() { + return path -> scopeRegistry.scopes().stream() + .filter(scope -> scope.worktree().filter(path::equals).isPresent()) + .findFirst(); + } +} diff --git a/app/src/main/java/app/drydock/review/ReviewScope.java b/app/src/main/java/app/drydock/review/ReviewScope.java new file mode 100644 index 00000000..9a204428 --- /dev/null +++ b/app/src/main/java/app/drydock/review/ReviewScope.java @@ -0,0 +1,96 @@ +package app.drydock.review; + +import app.drydock.domain.ManagedSessionId; + +import java.nio.file.Path; +import java.util.Objects; +import java.util.Optional; + +/** + * One reviewable thing, addressed by an opaque handle rather than by a + * session id (Review MCP schema §0). + * + *

The Review destination is cross-repo, but {@code McpSessionRegistry} + * binds an MCP caller to exactly one session. A scope handle is + * therefore the unit an agent addresses: {@link ReviewScopeRegistry} mints + * it, the human grants it to a session, and every review MCP call carries + * its {@link #id()}.

+ * + *

{@link #worktree()} is empty only for a PR that has not been checked + * out. That is the whole reason the checkout gate exists: no worktree means + * no session, which means no agent caller and therefore no reviewer -- the + * diff can still be read, but nothing can be asked of an agent.

+ */ +public record ReviewScope( + String id, + Kind kind, + Path repoRoot, + Optional worktree, + String base, + String head, + Optional pr, + Optional sessionId) { + + /** What is being reviewed; drives the queue icon and how the diff is resolved. */ + public enum Kind { + /** An agent-authored (or human) checkout under {@code git worktree}. */ + WORKTREE, + /** Uncommitted changes in a checkout ({@code ❯_}). */ + WORKING_TREE, + /** A branch against its base, in the main checkout. */ + BRANCH, + /** A GitHub pull request, checked out or not. */ + PR, + /** A stack of dependent PRs. */ + STACK + } + + /** The GitHub side of a {@link Kind#PR} scope. */ + public record PullRequestRef(int number, Optional url) { + public PullRequestRef { + if (number <= 0) { + throw new IllegalArgumentException("PR number must be positive: " + number); + } + Objects.requireNonNull(url, "url"); + } + } + + public ReviewScope { + Objects.requireNonNull(id, "id"); + Objects.requireNonNull(kind, "kind"); + Objects.requireNonNull(repoRoot, "repoRoot"); + Objects.requireNonNull(worktree, "worktree"); + Objects.requireNonNull(base, "base"); + Objects.requireNonNull(head, "head"); + Objects.requireNonNull(pr, "pr"); + Objects.requireNonNull(sessionId, "sessionId"); + if (id.isBlank()) { + throw new IllegalArgumentException("scope id must not be blank"); + } + if (kind == Kind.PR && pr.isEmpty()) { + throw new IllegalArgumentException("a PR scope must carry a PullRequestRef"); + } + } + + /** + * The checkout a diff is read from: the worktree when there is one, + * otherwise the repository's main checkout. A not-checked-out PR + * therefore still resolves to a directory git can be run in -- what it + * does not resolve to is a directory containing the PR's code. + */ + public Path diffRoot() { + return worktree.orElse(repoRoot); + } + + /** This scope with {@code session} bound (the checkout gate's result, M5). */ + public ReviewScope withSession(ManagedSessionId session) { + return new ReviewScope(id, kind, repoRoot, worktree, base, head, pr, + Optional.ofNullable(session)); + } + + /** This scope with {@code newWorktree} as its checkout. */ + public ReviewScope withWorktree(Path newWorktree) { + return new ReviewScope(id, kind, repoRoot, Optional.ofNullable(newWorktree), base, head, pr, + sessionId); + } +} diff --git a/app/src/main/java/app/drydock/review/ReviewScopeRegistry.java b/app/src/main/java/app/drydock/review/ReviewScopeRegistry.java new file mode 100644 index 00000000..f6777f26 --- /dev/null +++ b/app/src/main/java/app/drydock/review/ReviewScopeRegistry.java @@ -0,0 +1,274 @@ +package app.drydock.review; + +import app.drydock.domain.ManagedSessionId; + +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.security.SecureRandom; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.function.Consumer; + +/** + * Mints, resolves and revokes {@link ReviewScope} handles, and records which + * sessions may address which scopes (Review MCP schema §0). + * + *

{@code McpSessionRegistry} binds an MCP caller to one session; the + * Review destination spans repositories. This registry is the bridge: an + * agent may touch the scope its own session is bound to, plus any + * scope a human explicitly granted it by pressing "Run review". That grant + * is what lets a session's agent review a worktree that is not its own.

+ * + *

Minting is idempotent on identity -- kind, repository + * root, worktree, base, head and PR number. The queue is reassembled every + * time repositories or worktrees change, and a fresh id per assembly would + * break everything keyed by {@code (scopeId, id)}: findings, threads, + * drafts and verdicts would all be orphaned by a background rescan. The id + * a scope was first minted with therefore survives until it is explicitly + * revoked (the item left the queue).

+ * + *

Thread-safe: the queue service assembles off the FX thread, the MCP + * router resolves on its own executor, and the UI reads on the FX + * thread.

+ */ +public final class ReviewScopeRegistry { + + /** Crockford base32 keeps schema-compatible opaque, case-insensitive ids. */ + private static final char[] BASE32 = "0123456789ABCDEFGHJKMNPQRSTVWXYZ".toCharArray(); + private static final SecureRandom RANDOM = new SecureRandom(); + + /** + * Identity of a scope: a place, plus which pull request it is -- + * the kind, the repository, the worktree, and the PR number. Nothing + * else. + * + *

Base and head are deliberately excluded. They are + * properties of a review, not what makes it that review, and both move + * for reasons that have nothing to do with the review: the base is + * derived from the repository, and a worktree's head changes whenever + * someone commits or switches branches inside it. + * + *

Including them was a real bug. Findings and verdicts are keyed by + * {@code (scopeId, …)}, so anything that changed the base -- checking out + * a pull request, or simply {@code git switch} in the main checkout -- + * re-derived every worktree's handle and made that worktree's findings + * and verdicts vanish from the UI while sitting untouched in the store + * under the old handle. Silently: the queue showed the new handle with + * nothing against it, and the reviewer had no way to tell their review + * had been orphaned rather than never made.

+ */ + private record Identity(ReviewScope.Kind kind, Path repoRoot, Optional worktree, + Optional pr) { + static Identity of(ReviewScope scope) { + return new Identity(scope.kind(), normalized(scope.repoRoot()), + scope.worktree().map(Identity::normalized), + scope.pr().map(ReviewScope.PullRequestRef::number)); + } + + private static Path normalized(Path path) { + return path.toAbsolutePath().normalize(); + } + } + + private final byte[] scopeIdSecret; + private final Map byId = new ConcurrentHashMap<>(); + private final Map idByIdentity = new ConcurrentHashMap<>(); + private final Map> grants = new ConcurrentHashMap<>(); + private final List> listeners = new CopyOnWriteArrayList<>(); + + /** Creates a registry with a fresh secret for callers that do not persist review data. */ + public ReviewScopeRegistry() { + this(newSecret()); + } + + /** + * Creates a registry whose scope handles are deterministic for this + * secret. Application startup obtains the secret from {@link + * AnnotationStore}, so a persisted annotation remains addressable after + * a restart without exposing repository paths in the handle. + */ + public ReviewScopeRegistry(byte[] scopeIdSecret) { + Objects.requireNonNull(scopeIdSecret, "scopeIdSecret"); + if (scopeIdSecret.length < 16) { + throw new IllegalArgumentException("scopeIdSecret must contain at least 128 bits"); + } + this.scopeIdSecret = scopeIdSecret.clone(); + } + + /** + * Mints a handle for {@code spec}, or returns the existing handle when a + * scope with the same identity is already registered. The {@code id} + * carried by {@code spec} is ignored -- callers describe what they want + * reviewed and this registry owns identity (see the class note on + * idempotence). + * + *

An existing handle is updated in place from {@code spec}: + * a rescan that discovers the scope now has a bound session must not + * leave the registry serving the sessionless value it minted first.

+ */ + public ReviewScope mint(ReviewScope spec) { + Objects.requireNonNull(spec, "spec"); + Identity identity = Identity.of(spec); + String id = idByIdentity.computeIfAbsent(identity, this::newId); + ReviewScope scope = new ReviewScope(id, spec.kind(), spec.repoRoot(), spec.worktree(), + spec.base(), spec.head(), spec.pr(), spec.sessionId()); + ReviewScope previous = byId.put(id, scope); + if (!scope.equals(previous)) { + notifyChanged(id); + } + return scope; + } + + /** + * Builds a spec with a placeholder id. {@link #mint} replaces it, so no + * caller has to invent one. + */ + public static ReviewScope spec(ReviewScope.Kind kind, Path repoRoot, Optional worktree, + String base, String head, Optional pr, + Optional sessionId) { + return new ReviewScope("rs_pending", kind, repoRoot, worktree, base, head, pr, sessionId); + } + + public Optional byId(String id) { + return id == null ? Optional.empty() : Optional.ofNullable(byId.get(id)); + } + + /** Every live scope, in mint order. */ + public List scopes() { + List live = new ArrayList<>(); + for (Map.Entry entry : new LinkedHashMap<>(idByIdentity).entrySet()) { + ReviewScope scope = byId.get(entry.getValue()); + if (scope != null) { + live.add(scope); + } + } + return List.copyOf(live); + } + + /** + * Drops the handle and every grant against it -- the item left the + * queue. Findings keyed by this id are not touched here: the store owns + * their lifetime, and revoking a handle for an item that later comes + * back remains addressable under its stable identity-derived id. The + * store owns the lifetime of its review data, while this registry owns + * whether an agent may currently address it. + */ + public void revoke(String id) { + if (id == null) { + return; + } + ReviewScope removed = byId.remove(id); + grants.remove(id); + idByIdentity.values().removeIf(id::equals); + if (removed != null) { + notifyChanged(id); + } + } + + /** Lets {@code sessionId}'s agent address {@code scopeId} ("Run review" on someone else's worktree). */ + public void grant(String scopeId, ManagedSessionId sessionId) { + Objects.requireNonNull(sessionId, "sessionId"); + if (!byId.containsKey(scopeId)) { + throw new IllegalArgumentException("No such review scope: " + scopeId); + } + grants.computeIfAbsent(scopeId, key -> ConcurrentHashMap.newKeySet()).add(sessionId); + notifyChanged(scopeId); + } + + /** Withdraws a grant made by {@link #grant}; the scope's own bound session is unaffected. */ + public void revokeGrant(String scopeId, ManagedSessionId sessionId) { + Set granted = grants.get(scopeId); + if (granted != null && granted.remove(sessionId)) { + notifyChanged(scopeId); + } + } + + /** + * Whether {@code sessionId} may address {@code scopeId}: either the + * scope is bound to that session, or a human granted it. An unknown + * scope is never addressable -- the MCP router turns that into a tool + * error rather than a silent empty result. + */ + public boolean isAddressableBy(String scopeId, ManagedSessionId sessionId) { + ReviewScope scope = byId.get(scopeId); + if (scope == null || sessionId == null) { + return false; + } + if (scope.sessionId().filter(sessionId::equals).isPresent()) { + return true; + } + return grants.getOrDefault(scopeId, Set.of()).contains(sessionId); + } + + /** Sessions explicitly granted {@code scopeId} (excluding its own bound session). */ + public Set grantsFor(String scopeId) { + return Set.copyOf(grants.getOrDefault(scopeId, Set.of())); + } + + /** + * Subscribes to mint/update/revoke/grant notifications; the returned + * runnable unsubscribes. Listeners may be called from any thread. + */ + public Runnable addChangeListener(Consumer listener) { + Objects.requireNonNull(listener, "listener"); + listeners.add(listener); + return () -> listeners.remove(listener); + } + + private void notifyChanged(String scopeId) { + for (Consumer listener : listeners) { + listener.accept(scopeId); + } + } + + private String newId(Identity identity) { + try { + Mac mac = Mac.getInstance("HmacSHA256"); + mac.init(new SecretKeySpec(scopeIdSecret, "HmacSHA256")); + update(mac, identity.kind().name()); + update(mac, identity.repoRoot().toString()); + update(mac, identity.worktree().map(Path::toString).orElse("")); + update(mac, identity.pr().map(Object::toString).orElse("")); + return opaqueId(mac.doFinal()); + } catch (Exception e) { + throw new IllegalStateException("Unable to create a review scope id", e); + } + } + + private static void update(Mac mac, String value) { + byte[] bytes = value.getBytes(StandardCharsets.UTF_8); + mac.update((byte) (bytes.length >>> 24)); + mac.update((byte) (bytes.length >>> 16)); + mac.update((byte) (bytes.length >>> 8)); + mac.update((byte) bytes.length); + mac.update(bytes); + } + + private static String opaqueId(byte[] digest) { + StringBuilder out = new StringBuilder("rs_"); + int bit = 0; + for (int group = 0; group < 18; group++) { + int value = 0; + for (int offset = 0; offset < 5; offset++, bit++) { + value = (value << 1) | ((digest[bit / 8] >>> (7 - (bit % 8))) & 1); + } + out.append(BASE32[value]); + } + return out.toString(); + } + + private static byte[] newSecret() { + byte[] secret = new byte[32]; + RANDOM.nextBytes(secret); + return secret; + } +} diff --git a/app/src/main/java/app/drydock/review/ReviewVerdict.java b/app/src/main/java/app/drydock/review/ReviewVerdict.java new file mode 100644 index 00000000..0afdb0f5 --- /dev/null +++ b/app/src/main/java/app/drydock/review/ReviewVerdict.java @@ -0,0 +1,77 @@ +package app.drydock.review; + +import java.time.Instant; +import java.util.Locale; +import java.util.Objects; +import java.util.Optional; + +/** + * The human's decision on one intent (Review handoff §7): keyed by + * {@code (scopeId, intentId)}, because intent ids repeat across scopes for + * the same reason finding ids do. + */ +public record ReviewVerdict(String scopeId, String intentId, Decision decision, + Optional note, Instant at) { + + /** What was decided. {@code AUTO_APPROVED} is the agent's own assertion, not the human's. */ + public enum Decision { + APPROVED("approved"), + CHANGES("changes"), + AUTO_APPROVED("auto-approved"); + + private final String wireName; + + Decision(String wireName) { + this.wireName = wireName; + } + + public String wireName() { + return wireName; + } + + /** The label the intent rail and the verdict bar show once settled. */ + public String label() { + return switch (this) { + case APPROVED -> "✓ approved"; + case CHANGES -> "↺ changes requested"; + case AUTO_APPROVED -> "✓ auto-approved"; + }; + } + + public static Optional fromWire(String raw) { + if (raw == null) { + return Optional.empty(); + } + String normalized = raw.strip().toLowerCase(Locale.ROOT); + for (Decision decision : values()) { + if (decision.wireName.equals(normalized)) { + return Optional.of(decision); + } + } + return Optional.empty(); + } + } + + public ReviewVerdict { + Objects.requireNonNull(scopeId, "scopeId"); + Objects.requireNonNull(intentId, "intentId"); + Objects.requireNonNull(decision, "decision"); + Objects.requireNonNull(note, "note"); + Objects.requireNonNull(at, "at"); + if (scopeId.isBlank() || intentId.isBlank()) { + throw new IllegalArgumentException("a verdict is keyed by (scopeId, intentId); neither may be blank"); + } + } + + public Key key() { + return new Key(scopeId, intentId); + } + + /** {@code (scopeId, intentId)} -- intent ids repeat across scopes. */ + public record Key(String scopeId, String intentId) { + public Key { + Objects.requireNonNull(scopeId, "scopeId"); + Objects.requireNonNull(intentId, "intentId"); + } + } +} diff --git a/app/src/main/java/app/drydock/review/Severity.java b/app/src/main/java/app/drydock/review/Severity.java new file mode 100644 index 00000000..5ca4afb5 --- /dev/null +++ b/app/src/main/java/app/drydock/review/Severity.java @@ -0,0 +1,60 @@ +package app.drydock.review; + +import java.util.Locale; +import java.util.Optional; + +/** + * How serious a finding is, in the agent's opinion (Review MCP schema §3). + * The human can override it, and so can the outcome of an {@code asks} + * exchange -- both are recorded with an actor, and neither rewrites what the + * reviewer originally said. + * + *

Ordered most to least severe, so a margin or a queue badge can colour + * itself by the worst finding it holds.

+ */ +public enum Severity { + + /** Blocks approval of its intent until it is resolved or discussed. */ + BLOCKING("blocking"), + /** A question rather than a defect. */ + QUESTION("question"), + /** The change deviates from what the human asked for. */ + DEVIATION("deviation"), + /** A nit: worth saying, never worth blocking on. */ + NIT("nit"); + + private final String wireName; + + Severity(String wireName) { + this.wireName = wireName; + } + + /** The name this severity travels under, over MCP and in the store. Never rename one. */ + public String wireName() { + return wireName; + } + + /** The {@code app.css} modifier class for this severity's pill and pin. */ + public String styleClass() { + return "severity-" + wireName; + } + + /** Empty rather than throwing: an unknown severity from an agent is a value, not a crash. */ + public static Optional fromWire(String raw) { + if (raw == null) { + return Optional.empty(); + } + String normalized = raw.strip().toLowerCase(Locale.ROOT); + for (Severity severity : values()) { + if (severity.wireName.equals(normalized)) { + return Optional.of(severity); + } + } + return Optional.empty(); + } + + /** Whether a finding of this severity refuses approval of its intent while open. */ + public boolean blocksApproval() { + return this == BLOCKING; + } +} diff --git a/app/src/main/java/app/drydock/ui/AgentLabels.java b/app/src/main/java/app/drydock/ui/AgentLabels.java index 7b7c670a..9ce602bd 100644 --- a/app/src/main/java/app/drydock/ui/AgentLabels.java +++ b/app/src/main/java/app/drydock/ui/AgentLabels.java @@ -12,7 +12,7 @@ * wrong -- so every such label is derived from the session's {@link * AgentKind} here instead of being written out at a call site. */ -final class AgentLabels { +public final class AgentLabels { /** Sub-tab mark for the agent surface, matching the ❯_/▤/◨ of its neighbours. */ private static final String AGENT_GLYPH = "✳"; @@ -27,7 +27,7 @@ private AgentLabels() { } * name when no provider for {@code kind} is registered -- a session * persisted with an agent this build didn't discover still has to name it. */ - static String displayName(AgentRegistry registry, AgentKind kind) { + public static String displayName(AgentRegistry registry, AgentKind kind) { return registry.provider(kind) .map(AgentProvider::displayName) .orElseGet(() -> titleCase(kind.persistedName())); diff --git a/app/src/main/java/app/drydock/ui/MainWorkspace.java b/app/src/main/java/app/drydock/ui/MainWorkspace.java index 038d6e20..81efd62e 100644 --- a/app/src/main/java/app/drydock/ui/MainWorkspace.java +++ b/app/src/main/java/app/drydock/ui/MainWorkspace.java @@ -1,5 +1,6 @@ package app.drydock.ui; +import app.drydock.agent.api.Agent; import app.drydock.agent.api.AgentKind; import app.drydock.agent.api.AgentRegistry; import app.drydock.app.RepositoryManager; @@ -17,20 +18,36 @@ import app.drydock.domain.UiTheme; import app.drydock.domain.WorkspaceUiState; import app.drydock.git.ChangedLineService; +import app.drydock.git.DiffScope; import app.drydock.git.DiffService; import app.drydock.git.GhCliService; +import app.drydock.git.PrCheckoutService; +import app.drydock.git.UnifiedDiff; +import app.drydock.git.WorktreeNaming; import app.drydock.git.GitBranchState; import app.drydock.git.GitStatusService; import app.drydock.git.GitTarget; import app.drydock.git.WorktreeService; +import app.drydock.mcp.McpActivityLog; import app.drydock.mcp.McpSessionRegistry.Spawn; +import app.drydock.config.UserConfig; import app.drydock.mcp.WorkspaceMcpSessionContext; import app.drydock.process.SshCommandBuilder; import app.drydock.review.AnnotationStore; +import app.drydock.review.IntentGrouping; +import app.drydock.review.ReviewItem; +import app.drydock.review.ReviewAnnotation; +import app.drydock.review.ReviewIntent; +import app.drydock.review.ReviewVerdict; +import app.drydock.review.Severity; +import app.drydock.review.AnnotationStatus; +import app.drydock.review.ReviewQueueService; +import app.drydock.review.ReviewScope; +import app.drydock.review.ReviewScopeRegistry; import app.drydock.search.SessionSearchService; import app.drydock.ui.explorer.DiffOverlay; import app.drydock.ui.explorer.SessionExplorerView; -import app.drydock.ui.review.ReviewView; +import app.drydock.ui.review.ReviewDestinationView; import app.drydock.ui.model.WorkspaceViewModel; import app.drydock.terminal.TerminalFactory; import app.drydock.terminal.api.TerminalHostView; @@ -70,12 +87,14 @@ import java.lang.System.Logger; import java.lang.System.Logger.Level; import java.nio.file.Path; +import java.time.Instant; import java.util.ArrayList; import java.util.HashSet; import java.util.HashMap; import java.util.List; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.concurrent.CompletableFuture; @@ -83,6 +102,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.function.DoubleSupplier; +import java.util.function.Consumer; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -149,6 +169,43 @@ public final class MainWorkspace extends BorderPane implements WorkspaceNavigato private final StackPane centerStack; private final MenuButton newTabButton = new MenuButton("＋"); + /** + * The Review destination (Review handoff §1). A scene-graph view like + * the Explorer, so showing it hides every native terminal -- see {@link + * #setReviewShowing}. Built once and kept in {@link #centerStack}; + * rebuilding it per visit would drop the queue selection and the + * remembered rail-collapse state. + */ + private final ReviewDestinationView reviewDestination; + private final PrCheckoutService prCheckoutService = new PrCheckoutService(); + private final ReviewScopeRegistry reviewScopeRegistry; + private final ReviewQueueService reviewQueueService; + private final IntentGrouping intentGrouping = new IntentGrouping(); + + /** + * True while the Review destination owns the centre. Tracked separately + * from {@link #terminalsObscured}: a modal opening and closing over + * Review must not un-hide the terminals underneath it. + */ + private boolean reviewShowing; + + /** Fires when the Review queue changes, so the sidebar can restyle its badge. */ + private Runnable onReviewQueueChanged = () -> { }; + + /** Shows the shared shortcuts overlay (wired by DrydockApplication, which owns the modal layer). */ + private Runnable onShowShortcuts = () -> { }; + + /** + * A checkout whose queue item should be selected once the in-flight + * refresh lands. {@code ⌘4} arrives before the queue exists on a cold + * start, and dropping the request there would land the user on whatever + * item happened to sort first. + */ + private Path pendingReviewSelection; + + /** Which agent "Run review" would use; null until the human picks one. */ + private String selectedReviewer; + /** * The per-worktree empty pane (worktree handoff: "No session in this * worktree yet"), shown while an UNOPENED worktree is selected in the @@ -246,6 +303,7 @@ public MainWorkspace(SessionManager sessionManager, AgentRegistry agentRegistry, GitStatusService gitStatusService, SessionSearchService searchService, GhCliService ghCliService, WorktreeService worktreeService, DiffService diffService, ChangedLineService changedLineService, AnnotationStore annotationStore, + ReviewScopeRegistry reviewScopeRegistry, McpActivityLog activityLog, WorkspaceViewModel viewModel, Stage stage) { this.sessionManager = sessionManager; this.agentRegistry = agentRegistry; @@ -257,6 +315,9 @@ public MainWorkspace(SessionManager sessionManager, AgentRegistry agentRegistry, this.diffService = diffService; this.changedLineService = changedLineService; this.annotationStore = annotationStore; + this.reviewScopeRegistry = reviewScopeRegistry; + this.reviewQueueService = new ReviewQueueService(worktreeService, gitStatusService, + ghCliService::listReviewRequests, reviewScopeRegistry); this.viewModel = viewModel; this.stage = stage; this.worktreeLifecycle = new WorktreeLifecycleController(sessionManager, gitStatusService, @@ -286,7 +347,11 @@ public MainWorkspace(SessionManager sessionManager, AgentRegistry agentRegistry, } }); - centerStack = new StackPane(tabPane, emptyState, newTabButton); + reviewDestination = new ReviewDestinationView(new ReviewHost(), diffService, activityLog); + reviewDestination.setVisible(false); + reviewDestination.setManaged(false); + + centerStack = new StackPane(tabPane, emptyState, newTabButton, reviewDestination); StackPane.setAlignment(newTabButton, Pos.TOP_RIGHT); StackPane.setMargin(newTabButton, new Insets(10, 10, 0, 0)); setCenter(centerStack); @@ -295,9 +360,7 @@ public MainWorkspace(SessionManager sessionManager, AgentRegistry agentRegistry, if (newTab != null) { clearUnopenedWorktreeState(); } - for (OpenSessionTab open : openTabs.values()) { - open.setVisible(!terminalsObscured && open.tab == newTab); - } + updateTerminalVisibility(); updatePickerVisibility(); // Tab selection only moves the active-row highlight; the model // turns this into activeSessionChanged, never a tree rebuild. @@ -330,10 +393,33 @@ public void structureChanged() { } }); + // Finding counts drive the queue's per-item badge and the sidebar's + // ◨n badges, and the MCP tool router writes to the store from its own + // executor -- so the counts have to be told, not polled. No + // unsubscribe: this workspace and the store share the application's + // lifetime. + annotationStore.addChangeListener(key -> Platform.runLater(this::refreshReviewCounts)); + exitWatcher.setCycleCount(Animation.INDEFINITE); exitWatcher.play(); } + /** + * Releases the background executors this workspace owns. Lifecycle + * symmetry (AGENTS.md): anything with an executor gets a close that + * shutdown actually calls. + */ + public void closeReviewServices() { + prCheckoutService.close(); + } + + /** Re-reads the finding counts the Review queue and the sidebar badges render. */ + private void refreshReviewCounts() { + reviewDestination.refreshCounts(); + reviewDestination.refreshReviewState(); + onReviewQueueChanged.run(); + } + /** Pushes the manager's current session snapshot into the view model (FX thread; no-op if unchanged). */ private void publishSessions() { viewModel.setSessions(sessionManager.sessions()); @@ -393,7 +479,9 @@ private void populateNewTabMenu() { * tabs at all it fills the pane. */ private void updatePickerVisibility() { - boolean show = tabPane.getSelectionModel().getSelectedItem() == null; + // Review owns the whole centre while it is showing; the no-session + // placeholder underneath must not paint through the gaps. + boolean show = !reviewShowing && tabPane.getSelectionModel().getSelectedItem() == null; boolean hasTabs = !tabPane.getTabs().isEmpty(); StackPane.setMargin(emptyState, new Insets(hasTabs ? TAB_STRIP_HEIGHT : 0, 0, 0, 0)); boolean unopenedShowing = show && unopenedWorktreeState != null; @@ -579,9 +667,574 @@ public void showExplorerSubTab() { currentlySelected().ifPresent(open -> open.showSubTab(OpenSessionTab.SubTab.EXPLORER)); } - /** ⌘4: switches the selected session tab to its Review sub-tab. */ - public void showReviewSubTab() { - currentlySelected().ifPresent(open -> open.showSubTab(OpenSessionTab.SubTab.REVIEW)); + /** + * {@code ⌘4} from anywhere: navigates to the Review destination, scoped + * to the selected session's checkout when there is one (Review handoff + * §2). A navigation command, not a view switch -- Review spans + * repositories, so it cannot live inside one session's tab. + */ + public void showReviewForCurrentSession() { + Optional checkout = currentlySelected() + .map(OpenSessionTab::sessionId) + .flatMap(id -> sessionManager.sessions().stream() + .filter(session -> session.id().equals(id)) + .findFirst()) + .map(session -> session.worktreeRoot().orElseGet(() -> + repositoryFor(session).map(Repository::root).orElse(null))); + checkout.filter(Objects::nonNull).ifPresentOrElse(this::showReviewForCheckout, + this::showReview); + } + + // ---- Review destination (Review handoff sections 1 & 2) ----------------- + + /** + * Shows the Review destination, keeping whatever the queue already had + * selected. Review is a scene-graph view stacked over the tab pane, so + * this must hide every native terminal exactly as the Explorer swap does + * -- the ghostty surfaces paint above the whole JavaFX scene and would + * otherwise sit on top of it. + */ + @Override + public void showReview() { + setReviewShowing(true); + } + + /** + * {@code ⌘4} and the sidebar's {@code ◨n} badge: shows Review with the + * item for {@code checkoutRoot} selected. The queue is reassembled + * asynchronously, so the selection is applied both now (for a scope that + * is already minted) and again when the refresh lands. + */ + @Override + public void showReviewForCheckout(Path checkoutRoot) { + // Recorded before the refresh is kicked off, so the completion + // handler cannot land on a null request. + pendingReviewSelection = checkoutRoot; + setReviewShowing(true); + selectReviewScopeFor(checkoutRoot); + } + + /** Whether Review currently owns the centre (the Esc unwind order asks). */ + public boolean isReviewShowing() { + return reviewShowing; + } + + /** + * Closes the topmost thing Review has open -- the symbol lens, then the + * MCP panel -- and reports whether it closed anything. False means Esc + * should move on and leave Review altogether. + */ + public boolean unwindReviewOverlay() { + return reviewShowing && reviewDestination.unwindOne(); + } + + /** Leaves Review, restoring the tab pane and the selected tab's terminal. */ + public void hideReview() { + setReviewShowing(false); + } + + /** + * The one writer of {@link #reviewShowing}. Restoring the terminals goes + * through {@link #updateTerminalVisibility} and then re-runs geometry on + * the next pulse: the centre swap only invalidates the placeholder's + * bounds at the next layout pass, so the native frame would otherwise + * track stale bounds (the same ordering {@code OpenSessionTab.showSubTab} + * relies on). + */ + private void setReviewShowing(boolean showing) { + if (reviewShowing == showing) { + if (showing) { + reviewDestination.onShown(); + } + return; + } + reviewShowing = showing; + reviewDestination.setVisible(showing); + reviewDestination.setManaged(showing); + tabPane.setVisible(!showing); + newTabButton.setVisible(!showing); + newTabButton.setManaged(!showing); + updatePickerVisibility(); + updateTerminalVisibility(); + if (showing) { + reviewDestination.onShown(); + } else { + Platform.runLater(() -> currentlySelected().ifPresent(OpenSessionTab::updateGeometryNow)); + } + } + + /** + * Reassembles the queue off the FX thread and pushes it into the view. + * Remote repositories are skipped: they have no local checkout for git + * to run in, and their root is a virtual placeholder that must never be + * resolved against the filesystem. + */ + private void refreshReviewQueue() { + List local = repositoryManager.repositories().stream() + .filter(repository -> !repository.isRemote()) + .toList(); + List targets = local.stream() + .map(repository -> new ReviewQueueService.RepositoryTarget( + repository.root(), repository.displayName())) + .toList(); + reviewQueueService.assemble(targets, this::sessionAtCheckout) + .whenComplete((items, failure) -> Platform.runLater(() -> { + if (failure != null) { + LOG.log(Level.WARNING, "Could not assemble the Review queue", failure); + return; + } + adoptLegacyAnnotations(items); + reviewDestination.setItems(items, local.size()); + if (pendingReviewSelection != null) { + selectReviewScopeFor(pendingReviewSelection); + pendingReviewSelection = null; + } + onReviewQueueChanged.run(); + })); + } + + /** + * Moves annotations written before scope handles existed onto the scope + * they now belong to (see {@code AnnotationStore.adoptLegacy}). Runs on + * every queue assembly because a session may only bind to its worktree + * later; adoption is idempotent, so repeating it costs nothing once the + * legacy entries are gone. + */ + private void adoptLegacyAnnotations(List items) { + for (ReviewItem item : items) { + ReviewScope scope = item.scope(); + Optional session = scope.sessionId(); + if (session.isEmpty()) { + continue; + } + DiffScope diffScope = scope.kind() == ReviewScope.Kind.WORKING_TREE + ? DiffScope.WORKING_TREE + : DiffScope.BASE; + int adopted = annotationStore.adoptLegacy(session.get(), diffScope, scope.id()); + if (adopted > 0) { + LOG.log(Level.INFO, "Adopted " + adopted + " pre-scope-handle annotation(s) into " + + scope.id()); + } + } + } + + /** Selects the queue item whose scope is checked out at {@code checkoutRoot}, if it exists yet. */ + private void selectReviewScopeFor(Path checkoutRoot) { + reviewScopeRegistry.scopes().stream() + .filter(scope -> scope.worktree().filter(checkoutRoot::equals).isPresent()) + .findFirst() + .ifPresent(scope -> reviewDestination.selectScope(scope.id())); + } + + /** The managed session running in {@code checkoutRoot}, if any (the queue's MINE/AGENTS split). */ + private Optional sessionAtCheckout(Path checkoutRoot) { + return sessionManager.sessions().stream() + .filter(session -> session.worktreeRoot().filter(checkoutRoot::equals).isPresent()) + .map(ManagedAgentSession::id) + .findFirst(); + } + + /** + * Open findings for {@code scope} -- empty when no reviewer has run + * against it (spec §4.1), which is not the same as zero. A scope with no + * findings at all has never been reviewed; one whose findings are all + * resolved genuinely has none open, and says so. + */ + private Optional openFindingsFor(ReviewScope scope) { + if (annotationStore.forScope(scope.id()).isEmpty()) { + return Optional.empty(); + } + return Optional.of((int) annotationStore.openCount(scope.id())); + } + + /** {@code running} / {@code idle} for a scope's bound session; empty when none is bound. */ + private Optional reviewSessionState(ReviewScope scope) { + return scope.sessionId() + .flatMap(id -> sessionManager.sessions().stream() + .filter(session -> session.id().equals(id)) + .findFirst()) + .map(session -> session.status() == SessionStatus.RUNNING ? "running" : "idle"); + } + + /** How many Review queue items exist right now (the sidebar destination's badge). */ + public int reviewQueueSize() { + return reviewDestination.diagItems().size(); + } + + /** + * The intent grouping the MCP router writes and the Review view reads. + * Owned here (rather than by the router) because the view renders from it + * and the router only supplies it -- one holder, two readers. + */ + public IntentGrouping intentGrouping() { + return intentGrouping; + } + + /** Notified after every queue reassembly, so the sidebar can re-render its badge. */ + public void setOnReviewQueueChanged(Runnable handler) { + this.onReviewQueueChanged = handler == null ? () -> { } : handler; + } + + /** Shows the shared shortcuts overlay (Review's {@code ?} button); the app shell owns the modal layer. */ + public void setOnShowShortcuts(Runnable handler) { + this.onShowShortcuts = handler == null ? () -> { } : handler; + } + + /** Open findings for the worktree at {@code checkoutRoot} (the sidebar's per-worktree ◨n badge). */ + public Optional openFindingsAt(Path checkoutRoot) { + return reviewScopeRegistry.scopes().stream() + .filter(scope -> scope.worktree().filter(checkoutRoot::equals).isPresent()) + .findFirst() + .flatMap(this::openFindingsFor) + .filter(count -> count > 0); + } + + /** The Review view's window onto the workspace (see {@link ReviewDestinationView.Host}). */ + private final class ReviewHost implements ReviewDestinationView.Host { + + @Override + public void refreshQueue() { + refreshReviewQueue(); + } + + @Override + public void openSession(ManagedSessionId sessionId) { + OpenSessionTab open = openTabs.get(sessionId); + if (open == null) { + // Not open: resume it, which opens a tab and leaves Review. + sessionManager.sessions().stream() + .filter(session -> session.id().equals(sessionId)) + .findFirst() + .ifPresent(MainWorkspace.this::resumeSession); + hideReview(); + return; + } + hideReview(); + tabPane.getSelectionModel().select(open.tab); + } + + @Override + public Optional bodyFor(ReviewScope scope) { + // M2 returns the diff column here; until then the view renders + // its own placeholder, which is what the empty Optional means. + return Optional.empty(); + } + + @Override + public Optional openFindings(ReviewScope scope) { + return openFindingsFor(scope); + } + + @Override + public Optional sessionState(ReviewScope scope) { + return reviewSessionState(scope); + } + + @Override + public void showShortcuts() { + onShowShortcuts.run(); + } + + /** + * The Explorer lives inside a session's tab, so this can only work + * for a scope whose session is open. Reports that rather than + * pretending, which is what lets the diff column disable the button + * with an explanation instead of silently doing nothing. + */ + @Override + public boolean openInExplorer(ReviewScope scope, Path file, int line) { + OpenSessionTab open = scope.sessionId().map(openTabs::get).orElse(null); + if (open == null) { + return false; + } + hideReview(); + tabPane.getSelectionModel().select(open.tab); + open.openExplorerAt(file, line); + return true; + } + + @Override + public List findings(ReviewScope scope) { + return annotationStore.forScope(scope.id()); + } + + @Override + public List intents(ReviewScope scope) { + return intentGrouping.intentsFor(scope.id(), reviewDestination.currentDiff()); + } + + @Override + public Optional verdict(ReviewScope scope, ReviewIntent intent) { + return annotationStore.verdict(scope.id(), intent.id()); + } + + @Override + public void setVerdict(ReviewScope scope, ReviewIntent intent, + Optional decision) { + if (decision.isEmpty()) { + annotationStore.clearVerdict(scope.id(), intent.id()); + return; + } + // Approval is refused, not merely discouraged, while a blocking + // finding of this intent is open (spec §4.6). Checked here as well + // as in the bar so the keyboard path cannot slip past the button's + // refusal. + if (decision.get() == ReviewVerdict.Decision.APPROVED + && blockingFindingOpen(scope, intent)) { + return; + } + annotationStore.putVerdict(new ReviewVerdict(scope.id(), intent.id(), decision.get(), + Optional.empty(), Instant.now())); + } + + @Override + public void setResolved(ReviewScope scope, ReviewAnnotation finding, boolean resolved) { + annotationStore.mutate(finding.key(), current -> current.withStatus( + resolved ? AnnotationStatus.RESOLVED : AnnotationStatus.OPEN)); + } + + @Override + public void postMessage(ReviewScope scope, ReviewAnnotation finding, String body) { + annotationStore.mutate(finding.key(), current -> current.withReply( + new ReviewAnnotation.Message("You", Instant.now(), body))); + } + + /** + * {@code Apply patch}. drydock does not apply the patch itself: it + * hands the proposal to the scope's live session, exactly as every + * other worktree action hands work to the agent. What it records is + * the hand-off, never a fabricated outcome. + */ + @Override + public void applyPatch(ReviewScope scope, ReviewAnnotation finding) { + finding.patch().ifPresent(patch -> { + boolean handedOff = sendToBoundSession(scope, + "Apply this proposed patch from the review of " + finding.file() + + " (" + patch.summary() + "), then summarize what changed:\n" + + patch.unified()); + if (handedOff) { + annotationStore.mutate(finding.key(), + current -> current.withStatus(AnnotationStatus.SENT)); + } + }); + } + + @Override + public void overrideSeverity(ReviewScope scope, ReviewAnnotation finding, Severity severity) { + annotationStore.mutate(finding.key(), current -> current.withSeverityOverride(severity)); + } + + @Override + public void askAgentToFix(ReviewScope scope, ReviewIntent intent, + List findings) { + if (findings.isEmpty()) { + return; + } + StringBuilder prompt = new StringBuilder("Address these review findings on \"") + .append(intent.title()).append("\", then summarize what you changed: "); + int n = 1; + for (ReviewAnnotation finding : findings) { + prompt.append('[').append(n++).append("] ").append(finding.file()).append(' ') + .append(finding.startKey()).append(": ") + .append(finding.displayTitle().replaceAll("\\s+", " ")).append(". "); + } + if (sendToBoundSession(scope, prompt.toString().strip())) { + for (ReviewAnnotation finding : findings) { + annotationStore.mutate(finding.key(), + current -> current.withStatus(AnnotationStatus.SENT)); + } + } + } + + /** + * Records the submission and hands the worktree to the Finish flow -- + * the review is over, and merging, opening a PR or deleting the + * worktree is exactly what that flow already does. A scope with no + * bound session has nothing to finish, and simply records the + * submission. + */ + @Override + public void submit(ReviewScope scope) { + annotationStore.markSubmitted(scope.id()); + Optional session = scope.sessionId(); + Optional worktree = scope.worktree(); + if (session.isPresent() && worktree.isPresent()) { + hideReview(); + worktreeLifecycle.finishAfterReview(session.get(), worktree.get()); + } + } + + /** + * The agents that can act as a reviewer: every provider drydock can + * launch. Empty leaves Review a plain diff, which it must always be + * able to be. + */ + @Override + public List reviewers() { + return agentRegistry.agents().stream() + .filter(Agent::isAvailable) + .map(Agent::displayName) + .toList(); + } + + @Override + public Optional selectedReviewer() { + return Optional.ofNullable(selectedReviewer); + } + + @Override + public void selectReviewer(String reviewer) { + selectedReviewer = reviewer; + } + + /** + * Grants the scope to its bound session and asks that session's agent + * to review it. The grant is what the schema calls the human pressing + * "Run review": it is the only way an agent may address a scope that + * is not its own, and it is always a human action. + */ + /** + * The checkout gate's primary action, end to end: a worktree, the PR + * checked out into it, a session started on it, and the scope handle + * granted to that session so its agent may review it. + * + *

Every step runs off the FX thread and reports failure back to + * the gate, which is already showing progress -- a network fetch of a + * whole branch is not something to do behind a frozen window.

+ */ + @Override + public void startSessionAndReview(ReviewScope scope, Consumer onCheckoutFailed) { + Optional pr = scope.pr().map(ReviewScope.PullRequestRef::number); + if (pr.isEmpty()) { + onCheckoutFailed.accept("This scope is not a pull request."); + return; + } + Optional repository = repositoryManager.repositories().stream() + .filter(candidate -> candidate.root().equals(scope.repoRoot())) + .findFirst(); + if (repository.isEmpty()) { + onCheckoutFailed.accept("The repository this pull request belongs to is no longer registered."); + return; + } + Path worktree = WorktreeNaming.defaultDirectory(Path.of(System.getProperty("user.home")), + UserConfig.load().worktreesDirectory(), repository.get().displayName(), + PrCheckoutService.localBranchFor(pr.get())); + + prCheckoutService.checkout(scope.repoRoot(), worktree, pr.get()) + .whenComplete((created, failure) -> Platform.runLater(() -> { + if (failure != null) { + LOG.log(Level.WARNING, "PR checkout failed for #" + pr.get(), failure); + onCheckoutFailed.accept(UiErrors.unwrap(failure).getMessage()); + return; + } + openCheckedOutPr(repository.get(), scope, created, pr.get(), onCheckoutFailed); + })); + } + + @Override + public void readPatchOnly(ReviewScope scope, Consumer> onComplete) { + Optional pr = scope.pr().map(ReviewScope.PullRequestRef::number); + if (pr.isEmpty()) { + onComplete.accept(Optional.empty()); + return; + } + ghCliService.prDiff(scope.repoRoot(), pr.get()) + .thenApply(diff -> diff.map(DiffService::parseUnified)) + .whenComplete((patch, failure) -> Platform.runLater(() -> { + if (failure != null) { + LOG.log(Level.WARNING, "Could not read the patch for PR #" + pr.get(), failure); + onComplete.accept(Optional.empty()); + return; + } + onComplete.accept(patch); + })); + } + + /** + * Diagnostic-only: runs the scope's real diff and reports what came + * back, so the visual pass can prove every queue item resolves its + * base rather than only the selected one. + */ + @Override + public String diagDiffSummary(ReviewScope scope) { + DiffScope diffScope = scope.kind() == ReviewScope.Kind.WORKING_TREE + ? DiffScope.WORKING_TREE + : DiffScope.BASE; + try { + UnifiedDiff result = diffService.diff(scope.diffRoot(), diffScope, scope.base(), + DiffService.REVIEW_CONTEXT_LINES).get(30, TimeUnit.SECONDS); + return result.files().size() + " files"; + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return "INTERRUPTED"; + } catch (ExecutionException | TimeoutException e) { + return "FAILED: " + UiErrors.unwrap(e).getMessage(); + } + } + + @Override + public boolean runReview(ReviewScope scope) { + Optional session = scope.sessionId(); + if (session.isEmpty()) { + return false; + } + reviewScopeRegistry.grant(scope.id(), session.get()); + return sendToBoundSession(scope, + "Review the changes in this worktree with the drydock review tools. " + + "Read review_scope for handle " + scope.id() + + ", then post review_intents and review_finding against it. " + + "Call review_state first so already-settled findings are not re-flagged."); + } + } + + /** + * Starts a session on the freshly checked-out worktree and grants it the + * scope, so its agent may review the PR. The grant is the human action + * the MCP schema requires; it happens here because this whole flow began + * with a human clicking "Start session & review". + */ + private void openCheckedOutPr(Repository repository, ReviewScope scope, Path worktree, + int prNumber, Consumer onFailure) { + try { + ManagedSessionId session = openWorktreeSession(repository, + PrCheckoutService.localBranchFor(prNumber), worktree, Optional.empty(), + false, AgentKind.CLAUDE, Spawn.FORBIDDEN); + // Re-mint so the scope now names its worktree and its session; + // minting is idempotent on identity, but the identity changed -- + // it has a worktree now -- so this is a new handle, and the old + // PR-without-checkout handle leaves the queue on the next scan. + ReviewScope bound = reviewScopeRegistry.mint(ReviewScopeRegistry.spec( + ReviewScope.Kind.PR, scope.repoRoot(), Optional.of(worktree), + scope.base(), scope.head(), scope.pr(), Optional.of(session))); + reviewScopeRegistry.grant(bound.id(), session); + pendingReviewSelection = worktree; + refreshReviewQueue(); + } catch (RuntimeException e) { + LOG.log(Level.WARNING, "Could not start a session on the checked-out PR #" + prNumber, e); + onFailure.accept("The pull request was checked out, but the session could not start: " + + UiErrors.unwrap(e).getMessage()); + } + } + + private boolean blockingFindingOpen(ReviewScope scope, ReviewIntent intent) { + return annotationStore.forScope(scope.id()).stream() + .filter(finding -> finding.intentId() + .map(id -> id.equals(intent.id())).orElse(true)) + .anyMatch(ReviewAnnotation::blocksApproval); + } + + /** + * Sends {@code prompt} to the scope's bound session's live terminal. + * False when there is no session or its tab is not open -- the caller + * then records nothing, because the hand-off did not happen. + */ + private boolean sendToBoundSession(ReviewScope scope, String prompt) { + OpenSessionTab open = scope.sessionId().map(openTabs::get).orElse(null); + if (open == null) { + return false; + } + open.sendPrompt(prompt); + return true; } /** ⌘⇧]: selects the next session tab (wraps around). */ @@ -616,9 +1269,21 @@ public void setOnToggleSidebar(Runnable handler) { /** Hides/restores every native terminal view while a modal is showing (see {@link #terminalsObscured}). */ public void setTerminalsObscured(boolean obscured) { this.terminalsObscured = obscured; + updateTerminalVisibility(); + } + + /** + * The single rule for whether a tab's native terminal paints: it must be + * the selected tab, no modal may be up, and Review must not own the + * centre. Three independent conditions with one writer -- the bug this + * prevents is a modal closing over Review and un-hiding the terminal + * through it, because the native view overlays the whole scene. + */ + private void updateTerminalVisibility() { Tab selected = tabPane.getSelectionModel().getSelectedItem(); + boolean allowed = !terminalsObscured && !reviewShowing; for (OpenSessionTab open : openTabs.values()) { - open.setVisible(!obscured && open.tab == selected); + open.setVisible(allowed && open.tab == selected); } } @@ -1143,7 +1808,7 @@ private void attachOpenedSession(OpenSessionTab placeholderTab, SessionOpenResul placeholderTab.setDisplayName(opened.session().displayName()); placeholderTab.setStatus(opened.session().status()); openTabs.put(opened.session().id(), placeholderTab); - placeholderTab.setVisible(!terminalsObscured + placeholderTab.setVisible(!terminalsObscured && !reviewShowing && tabPane.getSelectionModel().getSelectedItem() == placeholderTab.tab); opened.session().worktreeRoot().ifPresent(root -> worktreeLifecycle.setupWorktreeHeader(placeholderTab, opened.session().id(), root)); @@ -1284,7 +1949,13 @@ public void flushExplorerEdits() { /** Called by the sidebar after {@link SessionManager#deleteSession} so any open tab disappears too. */ @Override public void noteSessionDeleted(ManagedSessionId sessionId) { - annotationStore.removeSession(sessionId); + // Findings are keyed by scope handle now, so a deleted session's + // review data is reached through the scopes that were bound to it. + for (ReviewScope scope : reviewScopeRegistry.scopes()) { + if (scope.sessionId().filter(sessionId::equals).isPresent()) { + annotationStore.removeScope(scope.id()); + } + } // A deleted session is never coming back, so its activity file would // otherwise linger until the next startup purge. forgetActivity(sessionId); @@ -1347,12 +2018,13 @@ public void diagTypeInExplorer(String text) { } /** - * Diagnostic-only: switches the selected tab to the Review sub-tab and - * returns its view, so the automated visual pass can screenshot a - * populated Review pane -- the FX layer has no headless harness. + * Diagnostic-only: shows the Review destination and returns it, so the + * automated visual pass can screenshot a populated queue -- the FX layer + * has no headless harness. */ - public ReviewView diagShowReview() { - return currentlySelected().map(OpenSessionTab::diagShowReview).orElse(null); + public ReviewDestinationView diagShowReview() { + showReview(); + return reviewDestination; } // ---- Exit watcher -------------------------------------------------------- @@ -1581,13 +2253,14 @@ private OpenSessionTab createOpenSessionTab(ManagedSessionId sessionId, String d openTab.setOnPreviousSessionTab(this::selectPreviousSessionTab); openTab.setOnNextSessionTab(this::selectNextSessionTab); openTab.setOnToggleSidebar(() -> onToggleSidebar.run()); + openTab.setOnShowReview(this::showReviewForCurrentSession); if (repository.map(Repository::isRemote).orElse(false)) { - // Explorer (local file search) and Review (local diffs) have no - // local checkout to operate on -- spec: Feature gating. Leaving - // both factories unset (OpenSessionTab disables their toggle - // buttons for a remote tab; see its constructor) instead of - // wiring anything against the remote's placeholder root. + // The Explorer's file search has no local checkout to operate on + // -- spec: Feature gating. Leaving its factory unset + // (OpenSessionTab disables the toggle button for a remote tab; + // see its constructor) instead of wiring anything against the + // remote's placeholder root. repository.ifPresent(repo -> gitStatusService.getStatus(GitTarget.of(repo)) .whenComplete((status, failure) -> Platform.runLater(() -> { if (failure == null && status.branch() instanceof GitBranchState.OnBranch onBranch) { @@ -1603,24 +2276,6 @@ private OpenSessionTab createOpenSessionTab(ManagedSessionId sessionId, String d openExplorers.put(openTab, explorer); return explorer; }); - // openTab.sessionId() rather than the constructor parameter: the - // factory runs lazily (first Review open), by which time a created - // session's tab has adopted the real id -- annotations must key on it. - repository.ifPresent(repo -> openTab.setReviewFactory(() -> new ReviewView( - openTab.sessionId(), searchRoot, repo.root(), diffService, changedLineService, gitStatusService, - annotationStore, openTab.agentName(), openTab::sendPrompt, - new ReviewView.ExplorerBridge() { - @Override - public void openFileAtLine(Path relativeFile, int line) { - openTab.openExplorerAt(relativeFile, line); - } - - @Override - public void searchText(String token) { - openTab.searchInExplorer(token); - } - }))); - // Branch of the session's own checkout: for a worktree session the // search root IS the worktree, so its branch (not the main // checkout's) fills the header/sub-tab context lines. The main diff --git a/app/src/main/java/app/drydock/ui/OpenSessionTab.java b/app/src/main/java/app/drydock/ui/OpenSessionTab.java index 58c6ae77..33d27f92 100644 --- a/app/src/main/java/app/drydock/ui/OpenSessionTab.java +++ b/app/src/main/java/app/drydock/ui/OpenSessionTab.java @@ -10,7 +10,6 @@ import app.drydock.terminal.api.TerminalRuntime; import app.drydock.terminal.api.TerminalSurface; import app.drydock.ui.explorer.SessionExplorerView; -import app.drydock.ui.review.ReviewView; import javafx.animation.PauseTransition; import javafx.application.Platform; import javafx.css.PseudoClass; @@ -82,8 +81,15 @@ final class OpenSessionTab { private static final long SHELL_CLOSE_GRACE_MILLIS = 3000; private static final long SHELL_CLOSE_POLL_MILLIS = 100; - /** The four views a session tab can show in its content area (design handoff "Session Explorer" / "Diff Review"). */ - enum SubTab { CLAUDE, TERMINAL, EXPLORER, REVIEW } + /** + * The three views a session tab can show in its content area (design + * handoff "Session Explorer"). Review used to be a fourth: it is now a + * global destination (Review handoff §1), because a per-session tab + * cannot show a queue that spans repositories, and because it gated + * review off for remote repositories -- while reviewing a remote PR is + * the primary use case. {@code ⌘4} navigates there instead. + */ + enum SubTab { CLAUDE, TERMINAL, EXPLORER } /** One lazily-created native trio for the shell sub-tab (runtime + host, themed by MainWorkspace). */ record ShellTerminal(TerminalRuntime runtime, TerminalHostView host) { } @@ -105,20 +111,16 @@ record ShellTerminal(TerminalRuntime runtime, TerminalHostView host) { } private final Label statusLabel = new Label("Starting session..."); private final BorderPane content = new BorderPane(); - // -- Bottom Terminal/Explorer/Review sub-tab bar (handoff "Session Explorer" / "Diff Review") -- + // -- Bottom Agent/Terminal/Explorer sub-tab bar (handoff "Session Explorer") -- /** Text set in the constructor: it names THIS session's agent (Claude, Codex, …). */ private final ToggleButton claudeSubTabButton = new ToggleButton(); private final ToggleButton terminalSubTabButton = new ToggleButton("❯_ Terminal"); private final ToggleButton explorerSubTabButton = new ToggleButton("▤ Explorer"); - private final ToggleButton reviewSubTabButton = new ToggleButton("◨ Review"); private final Label subTabContext = new Label(); private SubTab activeSubTab = SubTab.CLAUDE; /** Built on first switch to Explorer, via {@link #setExplorerFactory}. */ private Region explorerView; private Supplier explorerFactory; - /** Built on first switch to Review, via {@link #setReviewFactory}. */ - private Region reviewView; - private Supplier reviewFactory; // -- Ephemeral shell Terminal sub-tab (never persisted; created on first switch) -- /** Supplies a fresh shell runtime+host whose wakeup drives the argument (the shell bridge's tickAndDraw). */ @@ -175,6 +177,8 @@ record ShellTerminal(TerminalRuntime runtime, TerminalHostView host) { } private Runnable onPreviousSessionTab = () -> { }; private Runnable onNextSessionTab = () -> { }; private Runnable onToggleSidebar = () -> { }; + /** ⌘4 intercepted inside the terminal: navigate to Review, scoped to this session. */ + private Runnable onShowReview = () -> { }; private String displayName; @@ -303,23 +307,15 @@ private Region buildSubTabBar() { explorerSubTabButton.setFocusTraversable(false); explorerSubTabButton.setOnAction(e -> showSubTab(SubTab.EXPLORER)); - reviewSubTabButton.getStyleClass().add("session-subtab"); - reviewSubTabButton.setFocusTraversable(false); - reviewSubTabButton.setOnAction(e -> showSubTab(SubTab.REVIEW)); - - // Remote repositories have no local checkout for Explorer (local file - // search) or Review (local diffs) to operate on -- spec: Feature - // gating. MainWorkspace never wires their factories for a remote - // tab, so disable the toggles up front instead of letting a click - // silently no-op in showSubTab. + // A remote repository has no local checkout for the Explorer's file + // search to operate on -- spec: Feature gating. MainWorkspace never + // wires its factory for a remote tab, so disable the toggle up front + // instead of letting a click silently no-op in showSubTab. if (isRemote) { explorerSubTabButton.setDisable(true); - reviewSubTabButton.setDisable(true); explorerSubTabButton.setTooltip(new Tooltip("Not available for remote repositories")); - reviewSubTabButton.setTooltip(new Tooltip("Not available for remote repositories")); } else { explorerSubTabButton.setTooltip(new Tooltip("Explorer (⌘3)")); - reviewSubTabButton.setTooltip(new Tooltip("Review (⌘4)")); } // The shortcut is spelled out ON the button, not only in its tooltip: @@ -329,15 +325,17 @@ private Region buildSubTabBar() { showKeyHint(terminalSubTabButton, "⌘2"); if (!isRemote) { showKeyHint(explorerSubTabButton, "⌘3"); - showKeyHint(reviewSubTabButton, "⌘4"); } + // No ⌘4 hint here: Review is no longer a sub-tab, so there is no + // button to put it on. It is advertised in the shortcuts overlay and + // on the sidebar's Review row instead. subTabContext.getStyleClass().add("session-subtab-context"); Region spacer = new Region(); HBox.setHgrow(spacer, Priority.ALWAYS); - HBox bar = new HBox(4, claudeSubTabButton, terminalSubTabButton, explorerSubTabButton, reviewSubTabButton, spacer, subTabContext); + HBox bar = new HBox(4, claudeSubTabButton, terminalSubTabButton, explorerSubTabButton, spacer, subTabContext); bar.setAlignment(Pos.CENTER_LEFT); bar.getStyleClass().add("session-subtab-bar"); return bar; @@ -348,11 +346,6 @@ void setExplorerFactory(Supplier factory) { this.explorerFactory = factory; } - /** Supplies the Review view on first use (MainWorkspace wires this; it knows the session's checkout + services). */ - void setReviewFactory(Supplier factory) { - this.reviewFactory = factory; - } - /** Supplies a fresh shell runtime+host on first switch to the Terminal sub-tab (MainWorkspace wires this). */ void setShellTerminalProvider(Function provider) { this.shellTerminalProvider = provider; @@ -369,9 +362,9 @@ void setShellCommand(String command) { } /** - * Explorer bridge for the Review tab (design handoff section C - * "Explorer integration"): builds the Explorer if needed, switches to - * it, and opens {@code relativeFile} at a 1-based line. + * Explorer bridge for the Review destination's {@code ⤢} (design handoff + * section C "Explorer integration"): builds the Explorer if needed, + * switches to it, and opens {@code relativeFile} at a 1-based line. */ void openExplorerAt(Path relativeFile, int line) { showSubTab(SubTab.EXPLORER); @@ -387,13 +380,7 @@ void diagTypeInExplorer(String text) { } } - /** Diagnostic-only (see MainWorkspace.diagShowReview): switches to the Review sub-tab and returns its view. */ - ReviewView diagShowReview() { - showSubTab(SubTab.REVIEW); - return reviewView instanceof ReviewView review ? review : null; - } - - /** Explorer bridge for the Review tab: switches to the Explorer and runs a text search for {@code token}. */ + /** Explorer bridge for Review: switches to the Explorer and runs a text search for {@code token}. */ void searchInExplorer(String token) { showSubTab(SubTab.EXPLORER); if (explorerView instanceof SessionExplorerView explorer) { @@ -407,8 +394,8 @@ SubTab activeSubTab() { /** * Switches between the native-surface sub-tabs (Claude, Terminal) and - * the scene-graph ones (Explorer, Review). The native views overlay the - * scene, so showing Explorer/Review must both swap the center node AND + * the scene-graph one (Explorer). The native views overlay the + * scene, so showing the Explorer must both swap the center node AND * hide the native hosts (else they keep painting over the view); * switching to a native sub-tab restores its placeholder center first * and re-runs geometry after the layout pass so the native frame tracks @@ -426,8 +413,8 @@ void showSubTab(SubTab subTab) { focusActiveNativeSubTab(); return; } - if (subTab == SubTab.EXPLORER || subTab == SubTab.REVIEW) { - Region view = subTab == SubTab.EXPLORER ? explorerViewOrBuild() : reviewViewOrBuild(); + if (subTab == SubTab.EXPLORER) { + Region view = explorerViewOrBuild(); if (view == null) { // Build failed: undo the button selection, stay put. selectSubTabButton(activeSubTab); @@ -462,6 +449,19 @@ void showSubTab(SubTab subTab) { } } + /** + * Re-runs the active native surface's geometry. Called after the + * workspace swaps the centre back from the Review destination: the swap + * only invalidates the placeholder's bounds at the next layout pass, so + * the native frame would otherwise keep tracking stale bounds. + */ + void updateGeometryNow() { + TerminalBridge active = activeSubTab == SubTab.TERMINAL ? shellBridge : bridge; + if (active != null) { + active.updateGeometry(); + } + } + /** * Puts the sub-tab's keyboard shortcut on the button itself, right of its * label, in the dimmer key style. A graphic (rather than more text) keeps @@ -488,7 +488,6 @@ private void selectSubTabButton(SubTab subTab) { claudeSubTabButton.setSelected(subTab == SubTab.CLAUDE); terminalSubTabButton.setSelected(subTab == SubTab.TERMINAL); explorerSubTabButton.setSelected(subTab == SubTab.EXPLORER); - reviewSubTabButton.setSelected(subTab == SubTab.REVIEW); } /** @@ -545,7 +544,9 @@ private void runShortcut(Shortcut shortcut) { case CLAUDE_SUB_TAB -> showSubTab(SubTab.CLAUDE); case TERMINAL_SUB_TAB -> showSubTab(SubTab.TERMINAL); case EXPLORER_SUB_TAB -> showSubTab(SubTab.EXPLORER); - case REVIEW_SUB_TAB -> showSubTab(SubTab.REVIEW); + // ⌘4 is a navigation command now, not a view switch: it selects + // the global Review destination, scoped to this session. + case REVIEW_SUB_TAB -> onShowReview.run(); case PREVIOUS_SESSION_TAB -> onPreviousSessionTab.run(); case NEXT_SESSION_TAB -> onNextSessionTab.run(); case TOGGLE_SIDEBAR -> onToggleSidebar.run(); @@ -559,12 +560,6 @@ private Region explorerViewOrBuild() { return explorerView; } - private Region reviewViewOrBuild() { - if (reviewView == null && reviewFactory != null) { - reviewView = reviewFactory.get(); - } - return reviewView; - } private HBox buildTabGraphic(Optional repository) { tabRepoLabel.getStyleClass().add("tab-repo-label"); @@ -833,6 +828,11 @@ void setOnToggleSidebar(Runnable handler) { this.onToggleSidebar = handler == null ? () -> { } : handler; } + /** ⌘4 target: the workspace navigates to the Review destination for this session's checkout. */ + void setOnShowReview(Runnable handler) { + this.onShowReview = handler == null ? () -> { } : handler; + } + /** * Immediate visual feedback while the session's graceful close runs * (up to the multi-second Ctrl+D grace period): without it the close diff --git a/app/src/main/java/app/drydock/ui/RepositorySidebar.java b/app/src/main/java/app/drydock/ui/RepositorySidebar.java index 50fb2cca..c4f2db2c 100644 --- a/app/src/main/java/app/drydock/ui/RepositorySidebar.java +++ b/app/src/main/java/app/drydock/ui/RepositorySidebar.java @@ -64,6 +64,7 @@ import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -73,6 +74,8 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; +import java.util.function.Function; +import java.util.function.IntSupplier; /** * The repository sidebar, rebuilt to the design handoff (README section 2) @@ -114,6 +117,29 @@ public final class RepositorySidebar extends VBox { private final ExternalEditorLauncher editorLauncher = new ExternalEditorLauncher(); private final TextField filterField = new TextField(); + + /** + * The Review destination row, pinned above the repository tree (Review + * handoff section 2). A real focus-traversable {@link Button}: the + * destination has to be reachable without the mouse, exactly like the + * tree below it. + */ + private final Button reviewDestinationButton = new Button(); + private final Label reviewDestinationBadge = new Label(); + + /** How many items the Review queue holds right now (drives the badge). */ + private IntSupplier reviewQueueSize = () -> 0; + + /** Open findings for a worktree checkout, if any -- the per-row ◨n badge. */ + private Function> openFindingsAt = path -> Optional.empty(); + + /** + * The finding counts the tree was last rendered with. A queue refresh + * that changes no count must not rebuild the tree: rebuild-the-world is + * a last resort (AGENTS.md), and Review reassembles its queue every time + * the destination is shown. + */ + private Map renderedFindingCounts = Map.of(); private final TreeItem treeRoot = new TreeItem<>(); private final TreeView tree = new TreeView<>(treeRoot); private final Label footerLabel = new Label(); @@ -218,6 +244,23 @@ public RepositorySidebar(RepositoryManager repositoryManager, GitStatusService g VBox header = new VBox(addButton, filterField); header.getStyleClass().add("sidebar-header"); + // -- Review destination, above the tree ------------------------------ + Label reviewGlyph = new Label("◨"); + reviewGlyph.getStyleClass().add("sidebar-destination-glyph"); + Label reviewLabel = new Label("Review"); + reviewLabel.getStyleClass().add("sidebar-destination-label"); + reviewDestinationBadge.getStyleClass().add("sidebar-destination-badge"); + Region reviewSpacer = new Region(); + HBox.setHgrow(reviewSpacer, Priority.ALWAYS); + HBox reviewRow = new HBox(8, reviewGlyph, reviewLabel, reviewSpacer, reviewDestinationBadge); + reviewRow.setAlignment(Pos.CENTER_LEFT); + reviewDestinationButton.setGraphic(reviewRow); + reviewDestinationButton.getStyleClass().add("sidebar-destination"); + reviewDestinationButton.setMaxWidth(Double.MAX_VALUE); + reviewDestinationButton.setTooltip(new Tooltip("Review — local changes, agent worktrees and PRs (⌘4)")); + reviewDestinationButton.setOnAction(e -> navigator.showReview()); + refreshReviewBadges(); + // -- Tree ----------------------------------------------------------- tree.getStyleClass().add("repo-tree"); tree.setShowRoot(false); @@ -242,7 +285,7 @@ public RepositorySidebar(RepositoryManager repositoryManager, GitStatusService g HBox footer = new HBox(footerDot, footerLabel); footer.getStyleClass().add("sidebar-footer"); - getChildren().addAll(header, tree, footer); + getChildren().addAll(header, reviewDestinationButton, tree, footer); // Keep the displayed list in sync with EVERY repository mutation, // not just the ones initiated by this sidebar's own handlers. The @@ -641,6 +684,54 @@ private void pruneRowCaches() { lockedBucketExpanded.retainAll(repoIds); } + /** Supplies the Review queue's item count for the destination badge. */ + public void setReviewQueueSize(IntSupplier supplier) { + this.reviewQueueSize = supplier == null ? () -> 0 : supplier; + refreshReviewBadges(); + } + + /** Supplies a worktree checkout's open-finding count for its {@code ◨n} badge. */ + public void setOpenFindingsAt(Function> lookup) { + this.openFindingsAt = lookup == null ? path -> Optional.empty() : lookup; + refreshReviewBadges(); + } + + /** + * Re-reads the Review counts: the destination's item badge, and the + * per-worktree {@code ◨n} badges the tree cells draw. Called after every + * queue reassembly. + */ + public void refreshReviewBadges() { + int items = reviewQueueSize.getAsInt(); + reviewDestinationBadge.setText(items == 0 ? "" : String.valueOf(items)); + reviewDestinationBadge.setVisible(items > 0); + reviewDestinationBadge.setManaged(items > 0); + + Map current = currentFindingCounts(); + if (!current.equals(renderedFindingCounts)) { + renderedFindingCounts = current; + rebuildTree(); + } + } + + /** Every worktree path the tree can show, mapped to its open-finding count. */ + private Map currentFindingCounts() { + Map counts = new LinkedHashMap<>(); + for (Repository repository : repositoryManager.repositories()) { + for (SidebarNode child : childNodesFor(repository)) { + Path checkout = switch (child) { + case SidebarNode.SessionNode node -> node.session().worktreeRoot().orElse(null); + case SidebarNode.UnopenedWorktreeNode node -> node.worktree().path(); + default -> null; + }; + if (checkout != null) { + openFindingsAt.apply(checkout).ifPresent(count -> counts.put(checkout, count)); + } + } + } + return Map.copyOf(counts); + } + /** Re-renders the one row backed by {@code worktreeRoot} (a worktree session row or an unopened row). */ private void updateWorktreeRow(Path worktreeRoot) { for (TreeItem repoItem : treeRoot.getChildren()) { @@ -1380,6 +1471,28 @@ private String repoCountsText(Repository repository) { return counts.toString(); } + /** + * The {@code ◨n} badge (Review handoff section 2): that worktree's + * open findings, and a jump into Review filtered to it. Absent + * rather than zero when no reviewer has run -- a confident zero would + * read as "reviewed, nothing found". + */ + private Optional