Conversation
…ot serve The 2026-09-19 wrong-checkout edit incident: a session whose cwd sits inside a linked worktree made an edit_file call with a prefixed path; the bytes landed in the MAIN checkout silently. Root cause chain: viewForSessionCWD returned (nil, nil) when the checkout's route could not serve (no dirty generation published yet), leaving the request on the base corpus with no rider; checkoutRootedPath then fell to the worktreeRootedPath existence heuristic, which leaves an exists-in-both file at the main checkout. viewForSessionCWD now checks route readiness before materializing: a mutative tool (per facades.mutatesSource on the authorized call marker) fails loudly with view_building — the same code an explicit worktree selector produces — instead of silently writing the base corpus. Read tools keep the soft base fallback but carry fallback_reason on the freshness rider so a degraded answer is visible. Regression tests: mutation refusal on unrouted checkout (no bytes move anywhere), read fallback rider fields. The binding happy path (route ready, bytes land in the worktree) is already pinned end-to-end by TestWorktreeMutationCoordinatorEndToEnd's cwd subtest.
Review pass (independent, larger model) found two P3s and one high-priority test gap in 8ac90591: - The route-not-ready base fallback rode a SelectorAuto rider, hiding which checkout the session actually bound. The rider now carries the resolved worktree selector and the primary base graph id, so a client can see both the intent and what actually answered. CheckoutID is unchanged; GraphID and RequestedView are the additions. - The read_file incident twin had no test. Added TestCWDBindingRouteNotReadyReadFileIsLabeled: repo-prefixed read from a worktree-anchored session with a retired route answers base with the labelled rider (requested_view=worktree:<id>, actual_view=base, fallback_reason=view_building, checkout_id and graph_id set). All session-cwd defence plus the full targeted regression (167s) pass.
… lanes Adds the 4 regression tests scoped in the 2026-09-20 handoff for the session-cwd route-not-ready mutation defence: - sole-repo topology (resolveFilePath's soleTrackedRepo branch, otherwise unreachable behind the two-repo fixture) still refuses loudly - batch_edit (handleAtomicBatchEdit) refuses like edit_file - write_file and edit_symbol refuse in parity with edit_file - a marker-less edit_file fails open to the second gate (refuseRoutedViewMutation) rather than bypassing both gates newViewStack is refactored into newViewStackWithRepos(t, includeOther) so the sole-repo fixture reuses the same generation/catalog/route setup instead of duplicating it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code review flagged that the newViewStackWithRepos doc comment implied the sole-repo test exercises resolveFilePath's soleTrackedRepo branch directly. It doesn't: the route-not-ready refusal fires in the outer middleware before any handler reaches resolveFilePath. The test is still valid — it proves the refusal is topology-independent — just not for the reason the old comment stated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…acade sourceMutatingFacades protects two facades, "edit" and "refactor" (facade_registry.go:166), but every existing route-not-ready regression test — old and new — only drove tools behind "edit" (edit_file, write_file, edit_symbol, batch_edit). The "refactor" facade's six legacy tools (apply_code_action, safe_delete_symbol, fix_all_in_file, inline_symbol, move_symbol, rename_symbol) go through the identical wrapToolHandler middleware and the same mutatesSource gate, but had zero coverage proving the route-not-ready refusal actually reaches them. Verified mechanically before writing the test: every refactor-facade handler is registered via s.addTool, which wraps it in the same s.wrapToolHandler(handler) chain as edit_file (server.go:3241-3247), and none of the six legacy names match checkoutControlOperationName or viewlessCatalogTool, so all six take the normal resolveRequestView path that produces the view_building refusal before the handler runs — same as the edit facade. All six now assert that refusal explicitly instead of relying on inference from the edit-facade tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Fixes a silent wrong-checkout write: when an MCP session's working directory is anchored inside a linked git worktree, and that worktree's view route can't yet serve (not fully published), a mutating tool call (
edit_file,write_file,edit_symbol,batch_edit, and therefactorfacade'srename_symbol/move_symbol/safe_delete_symbol/inline_symbol/apply_code_action/fix_all_in_file) previously degraded silently to the base corpus. For a file that exists in both the worktree and the main checkout, that meant the edit landed in the main checkout instead of the worktree — while the caller's context (cwd) said it was editing the worktree.viewForSessionCWD(internal/mcp/view_request.go) now refuses loudly (view_building) when the checkout is ready+automatic but its route can't yet serve and the request is a mutation (detected via theWithAuthorizedToolCallcontext marker +facades.mutatesSource). A read still degrades softly to base, but now carries a rider naming which checkout was actually wanted, so the degradation is visible rather than silent.refuseRoutedViewMutation) catches the case where no marker was present (so the first gate couldn't tell it was a mutation): it keys off the request's own tool name instead, and refuses an inexact/fallback view for any mutating tool — so even a marker-less call fails safely to aview_read_onlyrefusal rather than writing base.Test plan
TestCWDBindingRouteNotReadyMutationsRefuseLoudly/...ReadFileIsLabeled/...ReadsFallBackWithRider— pre-existing coverage foredit_fileand reads (unchanged behavior, retained as regression pins)TestCWDBindingRouteNotReadySoleRepoMutationRefusesLoudly— same refusal holds in a single-tracked-repo topology (bare/unprefixed paths), not just the two-repo fixtureTestCWDBindingRouteNotReadyBatchEditRefusesLoudly—batch_edit(a different code path thanedit_file, viahandleAtomicBatchEdit) refuses identicallyTestCWDBindingRouteNotReadyWriteAndEditSymbolRefuseLoudly—write_fileandedit_symbolparity withedit_fileTestCWDBindingRouteNotReadyRefactorFacadeRefusesLoudly— extends coverage to therefactorfacade's six tools (apply_code_action,safe_delete_symbol,fix_all_in_file,inline_symbol,move_symbol,rename_symbol), which share the mutation gate with theeditfacade but had no prior regression coverageTestCWDBindingRouteNotReadyMissingMarkerFailsOpenToReadOnly— a marker-less mutation still gets caught by the second gate (view_read_only), not a silent writego build ./...andgo vet ./...cleango test ./internal/mcp/...(full package, not just the new tests) — clean, twicego test ./...(repo-wide, excluding-raceper the knowninternal/mcp/store_sqlite/indexertimeout) — clean; one unrelated pre-existing flake ininternal/indexerunder full-parallel load (TestSpecLaunch_4_5_CrossWorkspaceHappyPath, times out only under full-suite parallelism, passes in isolation in <1s, zero overlap with this diff) — not touched by this changeedit_filethrough a fresh worktree-anchored session — the edit landed only in the worktree copy, main checkout untouched