Repository navigation
fix(mcp): preserve backslash paths during bare-filename rewriting - #5815
WillemJiang merged 2 commits into
Conversation
|
|
github-actions
Bot
added
area:docs
b5463c7 to
76a1bef
Compare
76a1bef to
017ff5f
Compare
willem-bd
left a comment
willem-bd
left a comment
There was a problem hiding this comment.
Clean, well-tested fix; I verified it independently at head 017ff5f. Ran the new regression test plus its class through the production converter (32 passed, 3 skipped) and confirmed red/green by reverting the one-line pattern change: exactly the 4 backslash cases fail pre-fix while both forward-slash controls pass, matching the PR description. The change also brings _rewrite_unique_bare_filenames in line with the boundary class _rewrite_changed_paths_with_spaces already used, and README + mcp AGENTS.md are updated per the repo's docs policy. One nit-level observation about a rare narrowing (trailing backslash / Markdown hard break) posted inline; nothing blocking.
| pattern = re.compile(rf"(?<![\w./-]){re.escape(name)}(?!(?:[\w/-]|\.[\w]))") | ||
| # Do not rewrite inside longer paths/words, with either path separator. | ||
| # A final sentence period is allowed, but ".bak" or another segment is not. | ||
| pattern = re.compile(rf"(?<![\w./\\-]){re.escape(name)}(?!(?:[\w/\\-]|\.[\w]))") |
There was a problem hiding this comment.
Verified the added \ boundaries are correct and that they mirror the existing class in _rewrite_changed_paths_with_spaces below. One minor narrowing worth knowing about: a single trailing \ immediately before a newline is most plausibly a Markdown hard line break, not a path segment, but the new lookahead now suppresses a genuine bare-filename rewrite in that position. Confirmed at this commit: "Saved as page.yml\<newline>next line" is left unrewritten, where the pre-fix pattern rewrote the filename (leaving the trailing \ in place). Backslash is also a legal filename character on POSIX, so treating it as a boundary is a defensible conservative tradeoff and I would not hold the PR on it — just flagging that neither the new tests nor the AGENTS.md sentence covers the trailing-separator case.
There was a problem hiding this comment.
Thanks for the independent red/green run and for pinning the narrowing precisely — you're right that the trailing-separator case was covered by neither the tests nor the docs sentence.
Pushed b39c18e to close the coverage gap without touching runtime behavior:
test_bare_filename_before_markdown_hard_break_is_left_untouchedlocks in the current reading through the production result converter, so the tradeoff is explicit rather than incidental.- The
AGENTS.mdpath-translation bullet now states that a trailing\immediately before a newline counts as a separator, and why.
I kept the conservative behavior rather than special-casing \ + newline: the rewrite is a convenience for correlating a produced file, while corrupting a real path reference is a correctness bug, so losing that one rewrite seems the better side of the tradeoff. Happy to narrow the lookahead if you read it the other way.
Verified locally on macOS, Python 3.12.14, uv sync --locked:
pytest tests/test_mcp_file_migration.py -q→ 81 passed, 6 skipped. At017ff5f9the same command gives 80 passed, 6 skipped, so the delta is exactly the one new case and nothing regressed.make lint→ruff checkclean,ruff format --check1631 files already formatted.make test-blocking-io→ 160 passed.git diff --check→ clean.
One caveat on the full suite, since it is not green on this machine: make test → 19450 passed, 22 failed, 212 skipped. All 22 failures are in tests/test_web_fetch_relative_links.py, and stderr shows readabilipy warning "node executable not found, reverting to pure-Python mode" — node is absent from PATH here, so the real-extractor assertions cannot hold. They are pre-existing and unrelated to this change: with both commits' test/docs edits stashed, that file still gives an identical 22 failed / 26 passed, and it contains no reference to the MCP rewrite path. Flagging it so the number isn't mistaken for a regression from this PR.
…rite Review noted that treating `\` as a separator also suppresses a rewrite when a single trailing backslash precedes a newline, which is more often a Markdown hard break, and that neither the tests nor AGENTS.md covered that position. Lock the behavior in through the production result converter and document the tradeoff: a backslash is a legal filename character on POSIX, so forgoing that one rewrite is preferable to risking a corrupted path reference. Runtime behavior is unchanged.
…tedance#5815) * fix(mcp): preserve backslash paths during bare-filename rewriting * test(mcp): cover the trailing backslash boundary in bare-filename rewrite Review noted that treating `\` as a separator also suppresses a rewrite when a single trailing backslash precedes a newline, which is more often a Markdown hard break, and that neither the tests nor AGENTS.md covered that position. Lock the behavior in through the production result converter and document the tradeoff: a backslash is a legal filename character on POSIX, so forgoing that one rewrite is preferable to risking a corrupted path reference. Runtime behavior is unchanged. --------- Co-authored-by: lakers-abing <[email protected]>
Why
When a stdio MCP call creates
page.ymland returns text that also mentionsmissing\page.yml, the bare-filename correlation pass rewrites the basename inside that unrelated path. For example,C:\outside\page.ymlbecomesC:\outside\/mnt/user-data/workspace/page.yml. The full-path pass has already left that reference unchanged, but the second pass treats a backslash as a bare-filename boundary.What changed
Treat backslashes as path separators on both sides of a correlated bare filename, just like forward slashes. Genuine bare filenames still resolve to the newly produced file; names embedded in unresolved Windows paths or directory segments remain unchanged.
Add parameterized regressions through the production MCP result converter, using real workspace files. Cover drive-qualified, UNC, relative and directory-segment references, with forward-slash controls and a valid bare filename in the same response.
Surface area
The runtime change is confined to the MCP harness result-conversion layer. No API, schema or dependency changes.
Bug fix verification
backend/tests/test_mcp_file_migration.py::TestRewriteLocalPathsInText::test_bare_filename_does_not_rewrite_path_segmentsBefore the source fix: 4 failed, 2 passed. After the fix: all 6 cases passed on macOS. The UNC case is skipped on Windows to avoid contacting an SMB server. The failures show that each backslash reference is corrupted while both forward-slash controls preserve the reference.
Validation
On macOS with Python 3.12.14 and repository-locked dependencies (
uv sync --locked):cd backend && .venv/bin/python -m pytest tests/test_mcp_file_migration.py -q— 71 passed, 6 skipped.cd backend && make test-blocking-io— 149 passed (both before and after the source fix).cd backend && make lint— passed; 1587 files already formatted.cd backend && make test— 18394 passed, 208 skipped, 3 deselected, 32 warnings in 423.35 seconds.git diff --check— passed.The first full-suite baseline attempt was interrupted after unrelated environment failures (local socket/DNS restrictions and missing Git HEAD in the initial archive checkout). The exact upstream Git baseline was restored and the final full run was performed with local network access. The focused red/green test used the unmodified production source and then the one-line fix. No live-provider tests were enabled. No Windows-host test run is claimed.
AI assistance
Tool(s) used: Codex
How you used it: Codex inspected upstream source and related PRs, reproduced the defect, and drafted the regression tests, minimal fix, documentation and this PR description.