Merged
Conversation
Exposes review annotations, worktree/session creation, and read-only workspace awareness to the claude sessions Drydock spawns, over a localhost HTTP MCP endpoint with per-session token identity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eight TDD tasks from registry to session wiring. Also corrects two spec premises found while grounding the plan: GitStatusService.createWorktree is already public (so no consolidation is needed), and the shared --settings file cannot carry a per-session token (so MCP config is a separate per-session --mcp-config file, capability-gated). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three adversarial reviews (security, implementation correctness, design) found 16 substantiated issues, all verified against source before acting: Would have broken the build or the packaged app: - ReviewView's exhaustive AnnotationStatus switch expression - WorktreeNaming package-private in app.drydock.ui - buildResumeCommand dereferences its session argument immediately - a third build*Command call site, plus 14 SessionManagerTest calls - jdk.httpserver absent from the jlink module list Would have corrupted data or shipped inert: - AnnotationStore has no observers, so a stale ReviewView card's read-modify-write silently discards an MCP write - notifications/initialized answered with -32601 risks the handshake never completing Design fixes: branch-name validation (origin/main shadows the remote-tracking ref), toRealPath membership, prompt sanitization, depth-1 fan-out with per-session budgets, repos_list without ssh probes, review_comments with base_branch and excerpt, review_reply replacing review_mark_addressed. Corrected two false claims in the spec: per-session isolation (the boundary is the user account) and "create-only means the worst outcome is clutter". Plan grows from 8 to 12 tasks, reordered so each lands compile-clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A pre-check-then-charge pair needed a non-throwing query the registry API did not expose. Charge-then-refund holds the limit exactly and keeps failures free, with no extra query.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AnnotationStore is about to gain a second writer (the MCP tool router on its own executor). ReviewView cached built annotation cards and its toggle/reply handlers captured the ReviewAnnotation value they were built from, so a second writer's change would be silently discarded by a stale read-modify-write. Add AnnotationStore.addChangeListener, firing outside the store's monitor after add/update/remove/ removeSession; make ReviewView's card handlers re-read by id before writing, and subscribe to rebuild affected rows. MainWorkspace tracks each Review view per tab so removeTab can unsubscribe it (lifecycle symmetry).
…Claude Review findings on the annotation-change-notification work: - sendToClaude() computed the SENT write from a captured ReviewAnnotation read before promptSender.accept() -- a synchronous hand-off wide enough for another writer to append a reply in between. Re-read via byId first, same as the toggle/reply handlers. - AnnotationStore is shared by every open session tab, so most change notifications are not about a given ReviewView at all (e.g. deleting session A fired a bulk null change every open tab's view reacted to, including session B's, wiping an in-progress reply draft). Extracted a pure routeSingleAnnotationChange(sessionId, scope, selectedFile, rendered, current) decision (IGNORE / REPLACE_ROW / REBUILD_FILE) that ignores changes to another session/scope, and a bulk-change check that only rebuilds when a currently-rendered card actually vanished. - A present-but-not-yet-rendered annotation (e.g. a fresh add from another writer, on the file already on screen) now routes to REBUILD_FILE instead of silently being dropped by replaceCardRow, which can only swap an existing row, never insert one. Added ReviewViewChangeRoutingTest for the pure routing decision (headless, mirrors ReviewRowModelsTest) and an AnnotationStoreTest documenting the store-level read-before-write contract sendToClaude's fix relies on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found the test asserted nothing: refunding with no prior charge returns early on the absent counter, so the clamp was never reached and deleting it left the test green. Charge-then-double-refund fails without the floor. Split the absent-counter branch into its own test.
Split the weak aRefundNeverDropsTheCounterBelowZero test into two: - aRefundNeverDropsTheCounterBelowZero now charges, refunds twice, and verifies the floor prevents counter underflow (exercises the clamp) - refundingWithNoPriorChargeIsSilent covers the absent-counter early return The original test called refund() before charge(), so no counter existed and the clamp was never reached. The mutation proof (removing the clamp) confirms the new tests actually exercise both branches of refund(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review verified empirically that Origin/main shadows origin/main on a case-insensitive filesystem — the macOS default, and this is a macOS app — so a Set.contains check on the first component is bypassed by one capital letter. One regionMatches rule covers case, remotes containing slashes, and the plain hijack, while still accepting originals/x. Also renamed a test whose name asserted the inverse of its body, and added one case per previously unpinned rule so deleting any rule turns the suite red.
…lash-safe
git branch Origin/main (case) and a remote whose own name contains a slash
both bypassed the exact Set.contains(firstComponent) shadow check, because
.git/refs/heads/<name> is a plain open() on the macOS default
case-insensitive filesystem and git permits remote.foo/bar.url. Replace the
check with a case-insensitive regionMatches against "<remote>/" for every
configured remote. Also reject a null remoteNames with McpToolException
instead of letting an NPE escape, and close test-coverage gaps (@{, //,
trailing slash, leading slash/empty component, backslash, control
character, a true isBlank()-only case). Drop the misleading/duplicate
"...IsFine" test whose body asserted rejection.
The fake returned every annotation regardless of caller, so a cross-session test could only pass if the router filtered again — putting session ownership back into the adapter the design keeps free of domain logic. Scoping the fake makes the test pin the interface contract.
FakeMcpSessionContext.annotations() ignored its caller argument, violating the documented "the calling session's annotations" contract. review_reply was compensating by re-filtering on sessionId itself, which put session ownership -- domain logic -- into the router, and let anotherSessionsAnnotationIsNotAddressable pass for the wrong reason. Scope the fake instead and drop the router-side filter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds jdk.httpserver to the jlink module list: test and run resolve com.sun.net.httpserver from the full JDK toolchain, so omitting it would break only the packaged app, at the first tool call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tool name Review found three defects in the plan's own test class: - bindsOnLoopbackOnly asserted on endpointUrl(), a string built from a literal, so it would pass if start() bound 0.0.0.0. Now asserts on the socket's actual bound address via a new boundAddress() accessor. - neitherPortNorTokenIsEverLogged compared a 256-bit random token against the constant "McpServer" and could not fail. Now also asserts the port is absent, which is the half that could realistically regress. - No test ever passed a Host value, so the Host branch had zero coverage and the X-Forwarded-Host stand-in was dead. Replaced with two raw-socket tests, since HttpClient refuses to set Host. Also: a tools/call with a missing or non-string name reached the router's String switch and NPE'd into a -32603. The server now returns an isError result instead.
… loopback test - toolsCall now rejects a missing/non-string tool name with an isError result instead of passing null into McpToolRouter's String switch, which threw NPE and surfaced as -32603. - McpServerTest gains a boundAddress()-based rawPost/Socket helper so aForeignHostHeaderIsRejected/aLoopbackHostHeaderIsAccepted exercise the real Host header (HttpClient cannot set it); the old test used a stand-in X-Forwarded-Host header the server never reads. - Added McpServer.boundAddress() so bindsOnLoopbackOnly asserts on the socket's actual bound address rather than a string built from a literal; replaced neitherPortNorTokenIsEverLogged (compared a token against the constant toString()) with a test that also checks the port is absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ory case A fresh install has no mcp/ directory yet, and Files.list throws NoSuchFileException for it -- an expected, common case, not a genuine failure worth a WARNING log line. Split it from the generic IOException catch, which still warns on real failures. Pinned by a test that installs a logging Handler and asserts no WARNING-or-above record is published.
Wires the MCP subsystem built in tasks 1-11 into running sessions: every locally launched claude now gets a per-session --mcp-config file pointing at a loopback McpServer, and WorkspaceMcpSessionContext answers the router's questions from the live workspace. Sessions an agent starts through session_start launch with Spawn.FORBIDDEN, so they cannot create worktrees or start further sessions -- agent-driven fan-out is depth 1. Remote SSH sessions get no config at all: claude runs on the host and cannot reach this machine's loopback address. A failed MCP startup or config write degrades to a session without Drydock tools rather than one that fails to launch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review found checklist item 11 asserting UI that no task implements: the spec promised a remote session's launch banner would say Drydock tools are unavailable, and nothing builds it. Rather than add unreviewed UI in the final task, the promise is withdrawn and recorded as a known legibility gap with follow-up work named.
…nused tokens The session-start path bounded each candidate repository's `git worktree list` separately while the waiting MCP call bounded the whole thing, so with enough registered repositories the outer wait timed out, McpToolRouter refunded the session charge -- and the tab opened anyway, letting an agent exceed the 4-session limit that bounds real spend. There is now ONE deadline across the owner lookup AND the FX hop that opens the tab, derived from WorkspaceMcpSessionContext.START_SESSION_TIMEOUT_SECONDS so the two bounds cannot drift apart, and the FX side refuses to open a tab once it has passed. Also: - mcpConfigFor now takes the detected capabilities and returns empty when claude lacks --mcp-config, instead of minting a token and writing a config file that mcpConfigFlag then discards. The resume path hit this on every capability-detection failure, since NO_CAPABILITIES has the flag false. - finalizeCreate releases the token and config file on the failure branch: a launch that never got a surface never reaches onSurfaceClosed, so its file outlived the app. - McpServer.server/executor/port are volatile; this task made them cross from the startup virtual thread to the FX shutdown path, where a stale null read would have leaked the bound listener socket. - sessions_list keys its branch lookup by real path on both sides. A session records the path it was opened with while `git worktree list` reports realpaths, so on macOS (/var -> /private/var) it reported a null branch for essentially every session. - The `../` excerpt test now writes its bait where the escape resolves to, so it fails on containment rather than passing on file absence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…re reimplemented Final whole-branch review found two overstatements. "The escalation is negligible" understated a sibling token: it can write annotations attributed as Claude into the human's open Review tab, spend another session's budget, and open real tabs — acting as the app, not just on files. And the spec described a git check-ref-format invocation that does not happen; the rules are reimplemented in Java on purpose.
The fixes touch the same three files (McpToolRouter, McpSessionContext, WorkspaceMcpSessionContext) from different directions, so they land as one commit rather than as splits that would not compile in between. Token lifecycle. markSessionExited is a fourth session-ending path and the most common one -- the user types `exit`, or claude finishes -- and it deliberately leaves the surface open, so onSurfaceClosed never runs and the <base>/mcp/ file kept a live bearer token on disk for as long as the tab stayed open. It now releases in its success branch (idempotent via the Optional), through a shared releaseMcpConfigAsync that onSurfaceClosed also uses. requireLiveSession additionally refuses a caller that is not RUNNING. Annotation writes. review_reply read a snapshot, checked for RESOLVED, then replaced by id, so a human clicking Resolve inside that window had their verdict (and any reply) overwritten with ADDRESSED. AnnotationStore gains mutate(id, transform) -- re-read, apply and store under the monitor, listeners fired after releasing it -- and the RESOLVED/FIXED refusal moves inside the transform, carried out as a checked McpToolException by a private Refusal wrapper. ReviewView's three read-then-update sites move over too, and update() is deleted now that nothing calls it. session_start no longer accepts the repository's main checkout: realWorktreesOf drops mainCheckout entries, so an agent cannot start a second claude in the tree the human is working in, and the router names why. repos_list and sessions_list now share ONE deadline per call instead of giving every registered repository its own 20s slice, in the same idiom as MainWorkspace.findWorktreeOwner; an expiry fails the call rather than letting the next repository start a fresh slice. initialize echoes the client's requested protocolVersion when it is one this server supports, and falls back to 2024-11-05 otherwise, instead of always naming one version and ignoring params. Tool descriptors declare their required arguments, so the model is told what review_reply, worktree_create and session_start cannot run without. Smaller items: session_start catches InvalidPathException alongside IOException; the annotation change-listener log passes the throwable; PromptSafety's provably dead second blank check and McpServer's dead null-result guard are deleted; <base>/mcp/ is created rwx------ (and tightened if it already exists); review_reply calls requireLiveSession like the other five tools; McpServer latches a `closed` flag that start() honours, so a shutdown racing startup cannot leak the listener socket -- the comment in DrydockApplication claimed that property without the code having it. The manual checklist gains the self-exit-without-closing-the-tab step, which item 12 did not cover. One test that could not fail is renamed to what it actually checks. 605 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… work The three defects the final review found all lived in seams between tasks, which is what a task-scoped review cannot see. Also names the four follow-ups and three accepted residuals so they survive the scratch ledger.
1. DrydockApplication.stop() closed AnnotationStore before McpServer, so an in-flight review_reply could reach AnnotationStore.persistAsync after its save executor had shut down — a RejectedExecutionException surfacing to the agent as a bare "Internal error". The comment between the two closes claimed exactly the property the ordering did not provide. Two independent review lenses found this; introduced by c58c636. 2. McpServer.close() called shutdownNow() without awaiting termination. stop(0) closes the listener but does not wait for exchanges already being handled, so a handler blocked in WorkspaceMcpSessionContext.join could still be touching SessionManager while stop() closed it on the next line. Now a bounded drain, mirroring SessionManager.close. 3. McpToolRouter's class Javadoc claimed it "owns no domain logic", unchanged since the stub-only commit while spawn grants, budgets, and argument validation were added to it. Reworded to state what is actually true: no domain *resolution*, but cross-cutting policy is enforced at the boundary deliberately, with the reason. Suite: 605 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rebase onto origin/main could not carry this: it lived in the merge commit the rebase discards, and the SPI files it touches were never conflicted (the branch had not modified them pre-merge), so they replayed at main's version without it. - AgentProvider gains supportsMcpConfig(), a cheap static fact modelled on supportsRemote(). Claude true; Codex and Pi decline -- --mcp-config is a Claude flag. Distinct from the probed ClaudeCapabilities.supportsMcpConfig, which stays provider-internal per the SPI's own rule. - CreateContext/ResumeContext carry Optional<Path> mcpConfig. - ClaudeAgentProvider appends the flag in mcpConfigFlag, after each remote early-return, so a remote session never gets it. No --strict-mcp-config. - SessionManagerMcpFlagTest is replaced by ClaudeAgentProviderMcpFlagTest: the flag is no longer assembled in SessionManager. Also fixes conflict markers this rebase committed into DrydockApplication: a 'git add -A' during the Task 12 replay staged that file while it was still conflicted. Caught by diffing the rebased tree against the verified pre-rebase merge.
…agTest The flag is assembled in ClaudeAgentProvider now, not SessionManager, so the coverage note named a test that no longer exists.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MCP server for the sessions Drydock hosts
Drydock owns state the
claudesessions inside it cannot reach: line-anchored review annotations, the worktree graph, the cross-repo registry, the session/tab graph. This PR exposes that over an MCP server hosted inside the app, which the sessions Drydock spawns connect back into over localhost HTTP.Direction matters: Drydock is the server, the hosted sessions are the clients. Drydock as an MCP client is explicitly out of scope.
The six tools
review_comments/review_reply— the loop this feature exists for. A human marks up a diff in the Review tab; the agent reads the threads on demand and reports back what it did. Comments carrybase_branchand a sourceexcerpt, because an annotation anchored to a line of a diff the agent cannot see is unusable, and the excerpt keeps later comments aligned as the agent's own edits shift line numbers.worktree_create/session_start— bounded fan-out into visible sidebar entries and terminal tabs, rather thangit worktree addinto directories nobody sees.repos_list/sessions_list— read-only workspace awareness, the cross-repo picture a cwd-bound harness structurally lacks.This is not net-new surface: the review loop already existed as keystroke injection (
ReviewView.sendToClaude). What MCP adds is re-reading on demand mid-task and writing back — and discovery deliberately stays with the existing Send button.Bounding fan-out, and what the token is not
A session started via
session_startgetsSpawn.FORBIDDEN, so it cannot spawn again — depth 1 — plus a per-session budget of 4 worktrees and 4 sessions, charged before the operation and refunded on failure so the limit can never be exceeded and a failure costs nothing. Without this, one instruction fans out into a dozen billableclaudeprocesses that MCP has no tool to clean up.The per-session token is attribution, not isolation, and the design says so in four places. Every session's config file is readable by any process running as the user, so a sibling agent can read another's token. That is accepted — an agent with a shell can already reach any repo — but the spec names what it actually grants rather than calling it negligible.
Three prerequisite refactors
WorktreeNamingmoved fromapp.drydock.uitoapp.drydock.git, so the router need not depend on the UI package.AnnotationStoregained change notification, closing a latent lost-update hazard:ReviewViewcached cards whose handlers captured the annotation value they were built from, so any second writer meant a human's click could silently discard it. Later replaced outright by an atomicmutate(id, transform); the unconditionalupdateis gone.AnnotationStatusgainedADDRESSED— the agent's claim about its own work, distinct from the human'sRESOLVED. This respects the "honest state" rule that retiredFIXED: the app never claims a fix on the agent's behalf, but an agent reporting its own work can.How it was built
Design → adversarial review before implementation → 12-task plan → subagent-driven TDD with a per-task review → whole-branch review → this code review. Both review docs record their own findings tables.
The adversarial pass changed 14 things before a line was written, including two claims that were simply false: per-session isolation, and "create-only means the worst outcome is clutter". It verified against real git that
worktree_create(branch="origin/main")would createrefs/heads/origin/mainand silently shadow the remote-tracking ref —git worktree addexits 0 and warns only on stderr.Reviews caught what inspection did not:
open()on a case-insensitive filesystem,Origin/mainshadowsorigin/mainidentically. Verified against real git 2.49.0.0.0.0bind; control-character bytes that vanished from a markdown code block. Each is now mutation-proved.review_replycould discard a human'sRESOLVED;session_startaccepted the repository's main checkout.AnnotationStoreclosed ahead of the listener socket, contradicting the comment sitting between them.Testing
605 tests green on Gradle 9.6.1 / JDK 26, JUnit 5 with hand-rolled fakes (no mocking library — which is why the router takes a narrow, JavaFX-free
McpSessionContextseam and is fully headless-testable).Covered: transport, auth,
Origin/Host(via raw sockets, sinceHttpClientrefuses to setHost), the notification path that decides whether the handshake completes at all, protocol-version negotiation, every tool's error cases, symlink resolution on both sides of the worktree membership test, prompt safety, remote-shadowing branch names, spawn grants, and budget exhaustion.Not verified — please run before merging
Two checks are unverifiable by automation and are steps 1 and 14 of
docs/manual-terminal-checklist.md:claudecompleting the handshake. The--mcp-configdocument shape and the negotiated protocol version are untested against an actual client. If either is wrong, all 605 tests stay green and the feature is inert.jdk.httpserveris in the jlink--add-moduleslist, verified by reading only.:app:testand:app:runresolvecom.sun.net.httpserverfrom the full JDK, so a missing module is silent everywhere except the shipped app.ReviewView's FX wiring also has no automated coverage — there is no TestFX harness in this repo — so the change-routing decision was extracted to a tested static method and the rest is verified by reading.Known follow-ups, documented in the spec
callertoMcpSessionContext.startSession, removing a repository scan that re-derives what the router already validated and the shared-deadline plumbing it forced.agentStartedflag: depth 1 is a property of the launch, not the session, so a human resuming restores spawn rights.🤖 Generated with Claude Code