Skip to content

fix(mcp): edit writes land in the checkout the caller meant - #23

Merged
wongk merged 4 commits into
mainfrom
fix/edit-path-resolution
Sep 17, 2026
Merged

wongk merged 4 commits into
mainfrom
fix/edit-path-resolution

Conversation

@wongk

@wongk wongk commented Sep 17, 2026

Copy link
Copy Markdown

Edit writes land in the checkout the caller meant

Three defects in mcp__lc__edit’s path resolution, found after an edit in a live session wrote a file into an unrelated task’s worktree. Each is fixed with a regression test that was watched failing first.

1. Absolute paths were refused against a checkout the caller never chose

The handler builds _allowed_edit_roots — which includes the main checkout — and confines against it. It then called the writer with a different, smaller list that omitted the main checkout, and the writer refuses anything outside what it is given. So whenever a worktree was inferred, an absolute path under the main checkout failed with path escape denied: ... is outside the workspace root <some other worktree>.

The two checks now share one list.

2. A relative path could silently land in another agent’s worktree

Relative paths resolve against a process-global set from the cwd of the last bash call made by any agent sharing the workspace’s daemon. The same repo-relative path usually exists in both the inferred worktree and the main checkout, so the write went wherever that shared value happened to point — correct change, wrong copy, reported as success.

Now: if a relative path names an existing file under more than one candidate root, the edit is refused and names both candidates, telling the caller to pass an absolute path or the new root. A path existing under exactly one root still resolves there, and a create (existing under neither) is unaffected.

Note for anyone reading the original commit’s lc-debt note: keying the global by MCP session id would not have fixed this. Sibling subagents share one session.

3. An explicit root

edit takes an optional root, so a caller can name the checkout instead of being guessed at. It outranks both the inference and the workspace root, is validated (workspace root, a linked worktree of it, or an allowed additional directory — anything else is refused rather than ignored), and suppresses the guess entirely. The disclosure now distinguishes explicit root argument from from last bash cwd, because those carry different trust.

4. A symlinked workspace root silently defeated fix 1

Found by probing fix 1 end to end rather than by its tests, which all passed. Edit paths come back resolved; the roots they were compared against did not, and Path.is_relative_to is lexical — so a /var/... root never contains a /private/var/... file. On macOS that is any workspace under /tmp or /var, or a home directory reached through a symlink.

This is the same trap the original commit already worked around for /tmp by carrying both /tmp and its resolved twin in the allow-list. Both sides are now resolved before comparison, in the handler, in rich_edit._resolve, and in the new root validator (whose additional-directories branch had the same asymmetry). The /tmp twin is removed as redundant rather than left dead — and the allowance was re-probed under both spellings to prove it still works.

Two further instances of the same lexical trap turned up inside the handler while checking: hook-diagnostic filtering and the contract review’s touched-file list both compared resolved paths against the raw root, so a symlinked workspace silently dropped every diagnostic and handed the review an empty file list. Signal loss rather than a confinement hole, fixed with the same local. repo_root itself is deliberately not resolved: it keys the code-context engine and the incremental reindex, and resolving it would fork a second index instance for the same repo.

Verified independently, not just by the tests

The tests are real — each was watched failing before its fix — but the symlink defect got past five of them, so the fix was also probed end to end against the live handler with a purpose-built fixture (a real repo, a real linked worktree, a real symlink):

  • absolute path under an unresolved workspace root → succeeds, worktree untouched
  • relative path existing in both checkouts → refused, both candidates named, neither file changed
  • /tmp and /private/tmp absolute edits → still allowed after the twin was removed
  • explicit root → writes exactly where named; root=/etc → refused

115 targeted tests pass; ruff check --no-cache and black --check clean; mypy --strict clean on both changed sources.

What this does not cover

  • The process-global still exists for callers that pass no root. Defect 2 makes its dangerous case loud rather than silent, but the guess itself only disappears per-call, as callers adopt the argument.
  • Symbol-kind edits bypass the ambiguity check. It reads file_path/path, so an edit that carries neither and resolves its target through the symbol index is not examined. Worth a follow-up.
  • Detection of which worktree a session is in is unchanged; only what is done with it changed.

wongk and others added 4 commits September 17, 2026 09:36
…s relative edit

Refuse a relative edit whose path exists in both the inferred worktree and
the workspace root, and add an explicit root argument that names the
checkout outright. The disclosure now says which of the two resolved it.
Path membership is lexical, so a root that still carries a symlink (macOS
/tmp or /var, a symlinked home) matched none of the resolved paths checked
against it -- which silently voided the main checkout added to the
allow-list. Resolve both sides in tool_smart_edit, rich_edit._resolve and
_resolve_explicit_edit_root; the /tmp twin entry is redundant now.

Co-Authored-By: lemoncrow <302591943+lemoncrow-agent[bot]@users.noreply.github.com>
LemonCrow-Session: s1
…s the same on Linux

Co-Authored-By: lemoncrow <302591943+lemoncrow-agent[bot]@users.noreply.github.com>
LemonCrow-Session: s1
@wongk
wongk merged commit 81d71fa into main Sep 17, 2026
9 checks passed
@wongk
wongk deleted the fix/edit-path-resolution branch September 17, 2026 21:33
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.

1 participant