fix(mcp): match fidelity globs with path.Match, not filepath.Match - #647
Open
tiendungdev wants to merge 1 commit into
Open
fix(mcp): match fidelity globs with path.Match, not filepath.Match#647tiendungdev wants to merge 1 commit into
tiendungdev wants to merge 1 commit into
Conversation
matchFidelityGlob normalizes both the pattern and the path to forward
slashes, then hands them to filepath.Match — whose separator is the
platform's. On Windows '/' is an ordinary character to that matcher, so a
single `*` crosses it:
matchFidelityGlob("internal/*.go", "internal/sub/x.go")
= true on windows, false on linux/macos
The file's own doc-comment states the assumption this breaks: "Go's
filepath.Match never crosses `/`". That holds on POSIX and is why the
linux/macos matrix has never seen it.
Everything around it already works in slash space — ToSlash on entry,
Split(rel, "/"), the `**` prefix and suffix handling — so path.Match and
path.Base are the matching primitives that space calls for. Identical to
filepath.Match on POSIX, where the separator already is '/'.
fidelity_globs is a public tool parameter on read_file and
get_editing_context, so on Windows a documented `internal/*.go` rule
silently applied to the whole subtree beneath internal/.
Whole package on windows: TestMatchFidelityGlob flips, newly-broken set
empty.
Owner
|
@tiendungdev IMO, it will be more beneficial to have a full test suite run for Windows as well as for Linux/MacOS - which is added in the #652 PR |
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.
Problem
matchFidelityGlobnormalizes both the pattern and the path to forward slashes, then matches withfilepath.Match— whose separator is the platform's. On Windows/is an ordinary character to that matcher, so a single*crosses it:The file's own doc-comment states the assumption this breaks:
That is true on POSIX, which is exactly why the linux/macos matrix has never been able to see it.
fidelity_globsis a public tool parameter onread_fileandget_editing_context, so on Windows a documentedinternal/*.gorule silently applied its fidelity to the entire subtree beneathinternal/.Change
path.Match/path.BaseinmatchSegmentGlob. Everything around it already works in slash space —ToSlashon entry,Split(rel, "/"), the**prefix/suffix handling — so those are the primitives that space calls for. On POSIX they are identical to thefilepathversions, since the separator already is/.Verification
TestMatchFidelityGlobred → green on windows/amd64, go1.26.6.internal/mcppackage, failing test names diffed againstmain(c982cf52): newly-broken set empty.One result I am not claiming
The full-package run came back 27 → 25, but only one of those is mine.
TestNotesManager_SaveQueryDeletealso flipped to green in that run — and it is not fixed by this change: run in isolation on this very branch it fails 3 times out of 3. It is a pre-existing order/parallelism-dependent flake that happened to pass in that particular full-package run, so the honest attributable delta is one test, not two.No cross-platform test is possible
path.Matchandfilepath.Matchare the same function on POSIX, so an assertion here passes on linux/macos before and after. The guard belongs in the windowsTest native-separator store path comparisonsstep, whose package list already carriesinternal/mcp— but I have leftci.ymluntouched here because #646 is open against that exact-runline and I would rather not hand you a conflict. Happy to addFidelityGlobto it in a follow-up once #646 lands, or fold it into #646 if you prefer that order.Related, deliberately not included
internal/mcp/tools_generate_skill.gohas the mirror-image inconsistency:matchPathPatternthere receives a nativerel(filepath.Relat :134), so itsfilepath.Matchis correct, while:540testsstrings.HasPrefix(rel, prefix+"/")against that same native path. Different bug, different fix, own change.Branched from
mainatc982cf52. Windows 11, go1.26.6.