fix(mcp): edit writes land in the checkout the caller meant - #23
Merged
Merged
Conversation
…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
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
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.
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 withpath 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
cwdof the lastbashcall 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-debtnote: keying the global by MCP session id would not have fixed this. Sibling subagents share one session.3. An explicit
rootedittakes an optionalroot, 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 distinguishesexplicit root argumentfromfrom 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_tois lexical — so a/var/...root never contains a/private/var/...file. On macOS that is any workspace under/tmpor/var, or a home directory reached through a symlink.This is the same trap the original commit already worked around for
/tmpby carrying both/tmpand its resolved twin in the allow-list. Both sides are now resolved before comparison, in the handler, inrich_edit._resolve, and in the new root validator (whose additional-directories branch had the same asymmetry). The/tmptwin 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_rootitself 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):
/tmpand/private/tmpabsolute edits → still allowed after the twin was removedroot→ writes exactly where named;root=/etc→ refused115 targeted tests pass;
ruff check --no-cacheandblack --checkclean;mypy --strictclean on both changed sources.What this does not cover
root. Defect 2 makes its dangerous case loud rather than silent, but the guess itself only disappears per-call, as callers adopt the argument.file_path/path, so an edit that carries neither and resolves its target through the symbol index is not examined. Worth a follow-up.