Skip to content

fix(view): refuse worktree-cwd mutations when the route isn't ready, instead of silently editing the wrong checkout - #816

Merged
zzet merged 7 commits into
zzet:mainfrom
timkjr:fix/worktree-edit-session-cwd
Sep 29, 2026
Merged

zzet merged 7 commits into
zzet:mainfrom
timkjr:fix/worktree-edit-session-cwd

Conversation

@timkjr

@timkjr timkjr commented Sep 20, 2026

Copy link
Copy Markdown

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 the refactor facade's rename_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 the WithAuthorizedToolCall context 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.
  • A second, independent gate (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 a view_read_only refusal rather than writing base.

Test plan

  • TestCWDBindingRouteNotReadyMutationsRefuseLoudly / ...ReadFileIsLabeled / ...ReadsFallBackWithRider — pre-existing coverage for edit_file and 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 fixture
  • TestCWDBindingRouteNotReadyBatchEditRefusesLoudly — batch_edit (a different code path than edit_file, via handleAtomicBatchEdit) refuses identically
  • TestCWDBindingRouteNotReadyWriteAndEditSymbolRefuseLoudly — write_file and edit_symbol parity with edit_file
  • TestCWDBindingRouteNotReadyRefactorFacadeRefusesLoudly — extends coverage to the refactor facade'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 the edit facade but had no prior regression coverage
  • TestCWDBindingRouteNotReadyMissingMarkerFailsOpenToReadOnly — a marker-less mutation still gets caught by the second gate (view_read_only), not a silent write
  • go build ./... and go vet ./... clean
  • go test ./internal/mcp/... (full package, not just the new tests) — clean, twice
  • go test ./... (repo-wide, excluding -race per the known internal/mcp/store_sqlite/indexer timeout) — clean; one unrelated pre-existing flake in internal/indexer under 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 change
  • Live-verified against a running daemon (not just the test fixture): rebuilt the binary from this branch, restarted the daemon, ran a real edit_file through a fresh worktree-anchored session — the edit landed only in the worktree copy, main checkout untouched

timkjr and others added 6 commits September 20, 2026 17:32
…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>
…a worktree

worktreeRootedPath short-circuited on the first os.Stat hit against the
resolved (main) path and returned immediately, never checking whether a
linked worktree also carried the file. This is the 2026-09-24 live
incident: a session created a worktree, never cd'd into it, then issued
an unrouted, unprefixed edit whose path existed in both checkouts — the
edit landed in main instead of refusing or asking.

Scope this narrowly to the two call sites where the checkout is inferred
rather than caller-stated (resolveFilePath's sole-tracked-repo fallback,
and anchorUnprefixedExisting): an explicit repo-prefixed path, or one
recovered from graph metadata in resolveNodePath/resolveGraphPath, keeps
the prior behavior — refusing there would break the existing
main-checkout-prefix contract (TestEditFile_MainCheckoutPrefixStillEditsMainCheckout).

Adds TestWorktreeRootedPath/refuses_a_file_that_exists_in_both_main_and_a_worktree
to pin the new refusal.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@timkjr

timkjr commented Sep 24, 2026

Copy link
Copy Markdown
Author

I had this fix deployed in my local server, but found the exact bug this PR targets reproduced live today in an unrelated repo — a separate, narrower gap in the same incident family, not a regression in what's already here.

Root cause: worktreeRootedPath (internal/mcp/tools_fileops.go) short-circuited on the first os.Stat hit against the resolved (main) checkout and returned immediately — it never checked whether a linked worktree also carried the file. So an unrouted, unprefixed edit whose target existed in both checkouts silently landed in main instead of refusing or asking, even with this PR's CWD-gate fix in place (that gate only fires when the session's cwd is bound to the worktree; an unprefixed path resolved via the sole-repo/anchor fallback never goes through it).

Added in 0679bea2: refuse with a clear error when an inferred path anchor (not an explicit repo-prefixed one) resolves ambiguously between main and a worktree, plus a regression test pinning it. Scoped narrowly — an explicitly repo-prefixed path keeps the existing behavior, so it doesn't regress the main-checkout-prefix contract this PR already tests for.

@zzet zzet left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes: the new ambiguity guard still falls back to the main checkout when more than one linked worktree contains the inferred target, preserving the wrong-checkout mutation this PR is intended to prevent.

@@ -270,16 +307,26 @@ func worktreeRootedPath(abs, root string, mi multiRepoLookup) string {
continue
}
if match != "" && match != candidate {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Refuse ambiguity between multiple linked worktrees

When a second linked worktree contains the inferred file, this branch returns the original main-checkout path with no error before the new ambiguity check can run. An unrouted mutation can therefore still edit—or create—the main-checkout file when two worktrees contain the target. When refuseAmbiguous is true, return errPathAmbiguousCheckout on the second distinct match, and add tests for main + two worktrees and for two worktrees with no main file.

…orktrees

worktreeRootedPath returned the originally-resolved (main-checkout) path
as soon as a second linked worktree matched, before the main-vs-worktree
ambiguity refusal could run. Called with refuseAmbiguous=true, the helper
could therefore hand back a main-checkout path for a target carried by
two worktrees — one that existed in main (main + two worktrees) or one
that did not (two worktrees, no main copy, so a write would create it).

Refuse with errPathAmbiguousCheckout on the second distinct worktree
match when refuseAmbiguous is set. An explicit selection
(refuseAmbiguous=false) keeps the resolved path, preserving the
main-checkout-prefix contract.

Tests:
- TestWorktreeRootedPath_MultipleWorktrees pins the helper for main + two
  worktrees, two worktrees with no main copy, and the explicit-selection
  pass-through.
- TestEditTools_UnprefixedPathAmbiguousAcrossTwoWorktrees drives
  edit_file and write_file end to end against a real MultiIndexer
  tracking main plus two worktrees, asserting the checkout-ambiguity
  refusal and that no checkout changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@timkjr

timkjr commented Sep 29, 2026

Copy link
Copy Markdown
Author

Thanks for catching this. You're right: the second-worktree branch returned abs before the main-vs-worktree refusal could run. Fixed in b2302cb.

Fix: with refuseAmbiguous set, worktreeRootedPath now returns errPathAmbiguousCheckout on the second distinct worktree match. An explicit selection (refuseAmbiguous=false, i.e. a repo-prefixed path) still keeps the resolved path, so the main-checkout-prefix contract is unchanged. I updated the doc comment to match.

Tests:

  • TestWorktreeRootedPath_MultipleWorktrees covers the two cases you asked for, main + two worktrees and two worktrees with no main file, plus the explicit-selection pass-through.
  • TestEditTools_UnprefixedPathAmbiguousAcrossTwoWorktrees drives edit_file and write_file end to end against a real MultiIndexer tracking main plus two worktrees. It asserts the refusal surfaces as the tool error and that no checkout's bytes change, and no file is created in main.

Scope of the gap: writing the end-to-end test showed that in that topology the tool path was already refusing before this fix. anchorUnprefixedExisting counted the file in several tracked repos and returned "names a file in multiple tracked repos", so no wrong-checkout write went through. The gap was in the helper: called directly with refuseAmbiguous=true, it returned the main-checkout path. The sole-tracked-repo branch of resolveFilePath can't see linked worktrees in the first place, because a tracked worktree adds its own prefix. With this change the helper enforces the refusal itself instead of relying on the caller's match count, and the end-to-end test now asserts the checkout-ambiguity error specifically.

Also worth noting:

  • Read-only callers of resolveFilePath now get errPathAmbiguousCheckout for an unprefixed path present in two or more worktrees, matching the main-plus-one-worktree refusal from 0679bea.
  • I added the blank // line gofmt wants before the refuseAmbiguous paragraph in the doc comment. 0679bea had left that paragraph unformatted.

Verified locally: go test -race ./internal/mcp/... and golangci-lint v2.13.1 on ./internal/mcp/... are both clean.

@zzet
zzet merged commit 70509f1 into zzet:main Sep 29, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants