fix(mcp): match fidelity globs with path.Match, not filepath.Match

[tiendungdev]

Problem

matchFidelityGlob normalizes both the pattern and the path to forward slashes, then matches with 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")
  linux/macos -> false
  windows     -> true      <- TestMatchFidelityGlob, fidelity_globs_test.go:61

The file's own doc-comment states the assumption this breaks:

explicit ** support so the documented internal/** / **/*.go forms work as written (Go's filepath.Match never crosses /)

That is true on POSIX, which is exactly why the linux/macos matrix has never been able to see it.

fidelity_globs is a public tool parameter on read_file and get_editing_context, so on Windows a documented internal/*.go rule silently applied its fidelity to the entire subtree beneath internal/.

Change

path.Match / path.Base in matchSegmentGlob. Everything around it already works in slash space — ToSlash on entry, Split(rel, "/"), the ** prefix/suffix handling — so those are the primitives that space calls for. On POSIX they are identical to the filepath versions, since the separator already is /.

Verification

• TestMatchFidelityGlob red → green on windows/amd64, go1.26.6.
• Whole internal/mcp package, failing test names diffed against main (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_SaveQueryDelete also 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.Match and filepath.Match are the same function on POSIX, so an assertion here passes on linux/macos before and after. The guard belongs in the windows Test native-separator store path comparisons step, whose package list already carries internal/mcp — but I have left ci.yml untouched here because #646 is open against that exact -run line and I would rather not hand you a conflict. Happy to add FidelityGlob to 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.go has the mirror-image inconsistency: matchPathPattern there receives a native rel (filepath.Rel at :134), so its filepath.Match is correct, while :540 tests strings.HasPrefix(rel, prefix+"/") against that same native path. Different bug, different fix, own change.

Branched from main at c982cf52. Windows 11, go1.26.6.

[zzet]

@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

[tiendungdev]

Agreed, and #652 is the better answer than what I proposed here. My "no cross-platform test is possible, so it needs a windows selector step" reasoning was solving the wrong problem — a full Windows shard makes the selector question moot, and my own selector lists were exactly the guesswork you describe.

Two notes so this PR is easy to dispose of:

• The fix itself is independent of how it gets guarded. matchFidelityGlob normalizes to / and then matches with filepath.Match, whose separator is the platform's, so on Windows internal/*.go matches internal/sub/x.go. path.Match is the primitive for the slash space this file already works in, and it is the same function on POSIX. That holds whether the guard is a selector step, the full shard, or nothing.
• I deliberately did not touch ci.yml here (fix(review): join the changeset to native-separator graph paths #646 was open against that same -run line). With ci: run full test suite on Windows #652 removing the job outright, there is nothing left to add — so this PR is now purely the one-file change.

I have posted the full local Windows failure enumeration on #652, including which of them are my environment rather than the platform, since that draft is waiting on exactly that.

[zzet]

Requesting changes at head 53ad34c.

The path.Match / path.Base substitution correctly fixes the targeted internal/*.go behavior on Windows. Two issues should be addressed before merge:

1. Blocking CI coverage gap — the Windows job does not run TestMatchFidelityGlob. Its -run selector omits FidelityGlob, so the only platform where the old and new implementations differ never executes the regression assertion. Linux and macOS pass both before and after this patch, which means the green checks do not validate the fix. PR #646 has now merged and current main still omits FidelityGlob from .github/workflows/ci.yml:118-124, so the earlier conflict rationale no longer applies. Rebase onto current main, add FidelityGlob to the Windows selector, and rerun CI.

2. Medium contract/documentation mismatch — internal/mcp/fidelity_globs.go:14 and the new matcher comment state that a single * never crosses /. However, matchSegmentGlob intentionally inherits matchPathPattern directory-prefix semantics: internal/* matches internal itself and its entire subtree. That compatibility behavior should not be removed in this patch, but the public schema and comment need to document the dir/* recursive exception, with a preservation test covering the directory, direct child, and nested child.

No security, authentication, secret-handling, or dead-code concern was found.

[tiendungdev]

Both correct. Verified each before changing anything, and the second one turned up a third problem of my own making. Pushed 5302c872, rebased onto 812eb2d8.

1 — the selector gap

Confirmed. TestMatchFidelityGlob never ran on the one platform where the two matchers disagree, so the green checks proved nothing about the fix. Rebased and added FidelityGlob to the Windows selector; it picks up 8 tests:

TestParseFidelityGlobs
TestMatchFidelityGlob
TestMatchFidelityGlob_DirStarStaysRecursive
TestReadFile_FidelityGlobsOmit
TestReadFile_FidelityGlobsFull
TestReadFile_FidelityGlobsCompressFallback
TestReadFile_FidelityGlobsKeepComposes
TestGetEditingContext_FidelityGlobsOmit

Sabotage check that it now binds: reverting to filepath.Match fails on Windows with

--- FAIL: TestMatchFidelityGlob
    Messages: matchFidelityGlob("internal/*.go", "internal/sub/x.go")

2 — the dir/* exception is real

I probed it rather than reading the code and agreeing:

pattern	path	result
internal/*	internal	true
internal/*	internal/a.go	true
internal/*	internal/sub/x.go	true
internal/*	internal/sub/deep/y.go	true
internal/*	internalx/a.go	false
internal/*.go	internal/sub/x.go	false

So internal/* has the same reach as internal and internal/**. matchSegmentGlob's prefix shortcut answers before path.Match ever sees a nested path — it is not glob matching at all, which is why the blanket "a single * never crosses /" in my comment and in the schema was wrong. The behavior predates this patch and is untouched by it.

Both comments and the schema now state it, and TestMatchFidelityGlob_DirStarStaysRecursive pins the directory, a direct child, two nested depths, and the negative that keeps the shortcut segment-anchored. Removing the /* block fails it on the directory itself and a nested child.

3 — my first wording broke the tools/list byte gate

Worth flagging since you would have hit it in review. The full sentence I wrote first cost 522 bytes — the description is shared by two tool registrations — and core has only 332 bytes of headroom:

core  mode=defer  bytes=96690  (baseline 96500)   # FAIL, +190 over

Baseline on main is 96168. The terse wording that landed costs 144 and leaves 188 bytes of headroom:

core  mode=defer  bytes=96312  (baseline 96500)   # PASS

I kept it under the ceiling rather than raising the ceiling — the gate is a deliberate diet and documentation bytes should not be the thing that relaxes it. If you would rather have the fuller explanation and think the ceiling can move, say so and I will swap them.

Measurement

windows/amd64, go1.26.6, -count=1, against 812eb2d8:

internal/mcp   24 -> 23 failures

Newly-broken set empty — the 23 are a strict subset of the 24, with TestToolsListByteCeilings and TestMatchFidelityGlob the only differences. gofmt and go vet clean.

Neither assertion can fail on linux/macos: where / is the separator, filepath.Match and path.Match return the same answer. The Windows leg is the only thing binding them, which is the same limitation as #646 and the reason I did not try to write a cross-platform version.

[zzet]

Requesting changes on head 5302c872 after a full second-pass review.

The two earlier findings are fixed: the Windows selector now executes the fidelity-glob tests, and the recursive dir/* compatibility behavior is documented and pinned. One public Windows regression remains:

Medium, blocking — find_files loses documented deep matches for middle **.

internal/mcp/tools_find_files.go:24-32 advertises internal/**/*_test.go and states that ** crosses path segments. handleFindFiles passes that glob directly to matchFidelityGlob at lines 90-98.

However, internal/mcp/fidelity_globs.go:93-151 only handles ** specially when it is bare, leading, or trailing. A middle ** falls through to the new path.Match, where it is segment-bounded.

At the PR head:

internal/**/*_test.go

internal/foo_test.go                    -> false
internal/mcp/foo_test.go                -> true
internal/graph/store_sqlite/foo_test.go -> false

The POSIX contract defect was pre-existing. On Windows, though, the old filepath.Match treated normalized / as an ordinary character, so the deep path matched; this PR changes that result from true to false. It therefore introduces a Windows regression for the public documented pattern while fixing the single-star behavior.

Please implement arbitrary-position, component-aware globstar matching, or give find_files a separate matcher with the promised semantics. Preserve the established recursive dir/* rule and document that exception in the find_files schema as well. Add handler-level direct/one-level/deep/negative cases and include FindFiles_Glob in the Windows selector.

Validation on a synthetic merge with current base 65dcc88: build, full internal/mcp tests, race tests, actionlint, the six-package Windows selector, and Windows cross-compilation passed; all 11 hosted checks are green. No security, authentication, secret-handling, or dead-code issue was found.

[tiendungdev]

Confirmed, and it is worse than a documentation gap — the schema's own example was silently reaching one directory level. Pushed 58d2a36e, rebased onto 6347e7f2.

Verifying it first

Ran both matchers side by side on windows/amd64 rather than reasoning about it:

internal/**/*_test.go vs	at PR head	main, before this PR (Windows)
internal/foo_test.go	false	false
internal/mcp/foo_test.go	true	true
internal/graph/store_sqlite/foo_test.go	false	true
internal/a/b/c/d_test.go	false	true
cmd/gortex/foo_test.go	false	false
internal/mcp/foo.go	false	false

Exactly as you described: the switch to path.Match turned the deep cases from true to false on Windows, and POSIX never matched them at all.

The fix

matchGlobstarSegments walks pattern and path a segment at a time and lets ** consume zero or more whole segments wherever it appears; every other segment still goes through path.Match, so a single * stays inside its segment.

Zero is deliberate. internal/**/*_test.go has to reach internal/foo_test.go as well, or the pattern means a different thing at each depth. That case was false on both platforms before, so this is a behavior change on POSIX too, not only a Windows repair — flagging it explicitly since it goes beyond the regression you reported.

It runs first and can only add a match: every existing branch is left byte-for-byte as it was. That is the cheapest way I could think of to guarantee the compatibility you asked me to preserve, rather than reasoning case-by-case about a rewrite. Checked the ones that could plausibly have moved:

internal/*      internal/sub/deep/y.go   true    dir/* still recursive
internal/*      internalx/a.go           false   still segment-anchored
internal/*.go   internal/sub/x.go        false   still segment-bounded
**/internal/*   x/internal/sub/deep.go   true    leading **/ + the /* shortcut

That last one is why I did not replace the branches: the leading-**/ component walk resolves it through matchSegmentGlob's prefix shortcut, and a clean segment matcher alone returns false for it.

Tests and CI

TestFindFiles_GlobstarCrossesSegmentsAtAnyDepth goes through handleFindFiles with a fixture holding one file at each depth plus both negatives — direct, one level, two levels, wrong basename, wrong subtree — and asserts the exact count. It is a separate fixture because the existing setupFindFilesServer tests assert exact counts of their own.

FindFiles_Glob is in the Windows selector; it picks up both that test and the existing TestFindFiles_Glob.

Sabotage: deleting the matchGlobstarSegments call leaves

"internal/direct_test.go" should match internal/**/*_test.go;
  got [{internal/sub/one_test.go go internal\sub\one_test.go}]

— one directory level only, exactly the behavior in your review.

Measurement

windows/amd64, go1.26.6, -count=1, against 6347e7f2:

internal/mcp   23 failures — the identical set as the previous head,
               nothing new and nothing fixed by accident

gofmt and go vet clean.

One thing worth your attention

The core preset is now 29 bytes under its tools/list ceiling (96471 of 96500). My first wording for the dir/* sentence in the find_files schema measured 96505 and failed the gate; the terser one fits. There is effectively no room left for further schema prose without moving the ceiling — worth knowing before the next tool description grows.

[zzet]

Re-review at head 58d2a36 still requires changes.\n\n1. High — repeated globstars cause combinatorial CPU exhaustion\n\nmatchGlobstarSegments recursively tries every remaining path split for each ** without memoization, a complexity bound, or cancellation. A 41-byte pattern containing 12 globstar components against a 25-segment nonmatch exceeded a 3-second timeout for one matcher call. find_files applies this matcher to every candidate file before enforcing the result limit, so a small user-controlled glob can pin a daemon core and concurrent requests can degrade or deny service.\n\nPlease replace the recursive enumeration with memoized DP over pattern/path indices or an equivalent bounded matcher, collapse redundant adjacent globstars, and add an adversarial repeated-** nonmatch test that must complete promptly.\n\n2. Medium — middle ** does not compose with the documented recursive dir/ behavior*\n\nAt the current head:\n\n matchFidelityGlob("src//internal/", "src/a/internal/sub/deep.go") == false\n\nThis contradicts the public contract that ** crosses directory boundaries anywhere while trailing dir/ represents the whole subtree. It is also a Windows behavior regression: the previous filepath.Match implementation accepted this deep path. The component matcher consumes only one segment for the final *, while the legacy prefix fallback treats src//internal literally and therefore cannot resolve the preceding globstar.\n\nPlease make trailing subtree matching compose with a globbed directory prefix, and add matcher plus handler/fidelity-consumer tests for direct, deeply nested, and negative cases.\n\nThe earlier review findings are fixed, and all hosted checks plus focused tests and vet pass. These two remaining correctness and resource-exhaustion issues block merge.

[tiendungdev]

Both confirmed, and the first one is worse than the three seconds you measured. Pushed bebdcb23.

1 — the globstar walk

Reproduced before touching anything:

pattern="a/**/**/**/**/**/**/**/**/**/**/**/**/z.go" (42 bytes)
path segments=27
panic: test timed out after 1m40s

Over a hundred seconds for one call. The recursion re-derived each (pattern suffix, path suffix) pair once per way of reaching it, so every extra ** multiplied the work — and glob is user input that find_files evaluates against every candidate before applying the limit. That was a way to pin a daemon core from one request, and it was mine to catch. Exponential backtracking is the first thing to check when writing a globstar matcher and I did not check it.

Fixed as you asked: memoised on (pattern index, path index), bounded by the state count — O(len(pattern) * len(rel)^2) worst case — and adjacent globstars collapse, since a/**/**/b means a/**/b and the duplicate only inflated the state space. The same input now returns in microseconds.

TestMatchFidelityGlob_RepeatedGlobstarsStayBounded runs it behind a five-second deadline. The bound is deliberately loose: the real figure is about six orders of magnitude under it, so a throttled runner cannot trip it, but the enumeration coming back fails outright.

2 — composition

Confirmed with both matchers side by side:

matchFidelityGlob("src/**/internal/*", "src/a/internal/sub/deep.go")
  previous head:                    false
  main before this PR, on Windows:  true

globPatternSegments now rewrites a trailing * to ** before matching. That is not a widening — this package has always read dir/* as the whole subtree, the same reach as dir/**; spelling it that way is what lets the rule survive a globbed prefix, which the literal-prefix fallback cannot do.

One visible consequence worth stating plainly: src/**/internal/* now also matches src/a/internal itself, exactly as internal/* has always matched internal.

Coverage at all three levels you named:

test	level
TestMatchFidelityGlob_GlobstarComposesWithTrailingSubtree	matcher
TestFidelityGlobDecideForPath_SubtreeComposition	fidelity consumer
TestFindFiles_GlobstarComposesWithTrailingSubtree	handler

each with direct, deeply nested and negative cases. All three are already covered by the FidelityGlob / FindFiles_Glob tokens in the Windows selector.

Everything previously pinned still holds — internal/* recursive, internalx/a.go rejected, internal/*.go segment-bounded, **/internal/* deep, *_test.go by basename, bare vendor prefix.

Measurement

windows/amd64, go1.26.6, -count=1, against 6347e7f2:

internal/mcp   23 failures — the identical set as the previous head

gofmt and go vet clean.

[zzet]

Re-review at head bebdcb2 still requires changes.

1. P2 — rewriting every terminal star to a globstar drops required path depth

globPatternSegments changes the final * segment to ** for every pattern ending in /*, even when the pattern contains no actual whole-segment globstar. Since the replacement can consume zero segments, patterns that require a slash now accept shallower files.

Copy-paste reproduction in internal/mcp/fidelity_globs_test.go:

func TestTerminalStarDoesNotDropARequiredSegment(t *testing.T) {
	for _, tc := range []struct {
		pattern string
		rel     string
	}{
		{"*/*", "top.go"},
		{"src/*/*", "src/top.go"},
	} {
		assert.Falsef(t, matchFidelityGlob(tc.pattern, tc.rel),
			"%q must not match the shallower path %q", tc.pattern, tc.rel)
	}
}

Both assertions pass at the base and fail at this head:

"*/*"     vs "top.go"     false -> true
"src/*/*" vs "src/top.go" false -> true

Consequently, a common */* find_files glob now includes root files, and the same pattern in fidelity_globs can apply omit/compress rules to unintended files. This contradicts the public contract that ordinary * is segment-bounded.

Please restrict the trailing-star rewrite to patterns containing an actual whole-segment **, or otherwise compose the targeted ** + subtree rule without changing ordinary */* depth. Add matcher and consumer-level regression cases.

2. P2 — the bounded-runtime test collapses away the memoization path

TestMatchFidelityGlob_RepeatedGlobstarsStayBounded creates twelve adjacent globstars, but globPatternSegments collapses them to one before the memoized matcher runs.

Copy-paste reproduction:

func TestCurrentBoundedInputCollapsesToOneGlobstar(t *testing.T) {
	pattern := "a/" + strings.Repeat("**/", 12) + "z.go"

	assert.Equal(t,
		[]string{"a", "**", "z.go"},
		globPatternSegments(pattern),
	)
}

Instrumentation of the exact implementation produced the same result with memoization enabled or removed: 29 calls, 27 loop iterations, zero memo hits. Therefore deleting or breaking the memo still leaves the claimed security regression green.

Use a non-adjacent input when testing the memo:

pattern := "a/" + strings.Repeat("**/x/", 8) + "never"
rel := "a/" + strings.Repeat("x/", 20) + "q"

That input produced 1,360 calls with memoization versus 803,860 without it. Please replace the adjacent pattern with an alternating one and sabotage-verify by removing only the memo while leaving adjacent-globstar collapse enabled.

The original resource-exhaustion and globstar-composition bugs are functionally fixed. Full internal/mcp tests, focused race tests, vet, and all 11 hosted checks pass. No additional authentication, secret-handling, injection, or dead-code issue was found. These correctness and security-regression-test gaps still block merge.

[tiendungdev]

Both reproduced with your snippets before I changed anything. Pushed 244559ec.

1 — the terminal-star rewrite was too broad

Confirmed exactly:

"*/*"     vs "top.go"     false -> true
"src/*/*" vs "src/top.go" false -> true

I widened the rewrite past its purpose. It exists only so the subtree rule survives a globbed prefix — src/**/internal cannot be resolved by the literal fallback — and applying it to a pattern with no globstar dropped a segment that an ordinary * requires, because ** may consume zero.

It now requires an earlier whole-segment **. Patterns without one keep their old depth, and the legacy dir/* shortcut still answers for literal prefixes exactly as before. Pinned at both levels you asked for: TestMatchFidelityGlob_TerminalStarKeepsItsRequiredSegment and TestFidelityGlobTerminalStarDepthAtTheConsumer.

2 — the bounded-runtime test, and a correction to the suggested input

You were right that it proved nothing: twelve adjacent globstars collapse to [a ** z.go] before the matcher runs.

Your alternating input fixes the collapse but not the test. I removed the memo, kept the collapse, ran it — and it still passed. 803,860 calls is about 10 ms, far inside any wall-clock bound. Call count shows the memo matters; it does not make a timing assertion bind.

So I measured the scaling to pick a size that does:

globstars	path segments	calls	unmemoised	memoised
8	20	803,860	10 ms	< 1 ms
8	40	246,777,526	7.48 s	< 1 ms
10	30	151,946,378	8.04 s	< 1 ms
10	40	3,189,663,472	1 m 53.8 s	< 1 ms

The test now uses ten and forty. The five-second deadline has a margin of more than twenty times against the exponential path while sitting orders of magnitude above the memoised one, so a loaded runner cannot trip it and a missing memo cannot pass it.

Sabotage as you specified — memo removed, collapse left enabled:

--- FAIL: TestMatchFidelityGlob_RepeatedGlobstarsStayBounded (5.00s)
    matchFidelityGlob("a/**/x/**/x/**/x/**/x/**/x/**/x/**/x/**/x/**/x/**/x/never", ...)
    did not finish within 5s — a user-supplied glob can pin a daemon core

The test also asserts up front that the pattern still carries more than four globstars after normalisation, so a future change to the collapse cannot quietly empty it again — which is the failure mode you just caught.

Measurement

windows/amd64, go1.26.6, -count=1, against 06939fce:

internal/mcp   23 failures — the identical set as the previous head

gofmt and go vet clean.

Thanks for staying on this one. Three of the last four findings were tests of mine that did not bind what they claimed to, which is a pattern I have taken on board rather than a run of bad luck.

[zzet]

Thanks for the careful follow-up. I rechecked the two findings from the previous round at head 244559ec, and both are genuinely fixed:

• ordinary */* and src/*/* retain their required path depth;
• the memoization regression now uses separated globstars, and deleting only the memo lookup/storage makes the test fail at the five-second deadline.

I found one remaining resource-exhaustion issue in the bounded matcher that needs to be addressed before merge.

P1 — dense glob memo is allocated once for every candidate file, including patterns with no globstar

At internal/mcp/fidelity_globs.go:109-110, every pattern ending in /* enters matchGlobstarSegments, even when the normalized pattern contains no whole-segment **:

if strings.Contains(pattern, "**") || strings.HasSuffix(pattern, "/*") || pattern == "*" {
    if matchGlobstarSegments(globPatternSegments(pattern), strings.Split(rel, "/")) {
        return true
    }
}

The matcher then allocates a dense (patternSegments+1)*(pathSegments+1) memo at fidelity_globs.go:209. find_files applies the matcher to every candidate before enforcing the result limit (tools_find_files.go:80,96-98), and there is currently no glob-length or segment-count limit.

Copy-paste reproduction

Add this benchmark to internal/mcp/fidelity_globs_test.go:

func BenchmarkMatchFidelityGlob_LongTerminalStarWithoutGlobstar(b *testing.B) {
    pattern := strings.Repeat("segment/", 999) + "*"
    rel := strings.Repeat("segment/", 39) + "leaf.go"

    b.ReportAllocs()
    b.ResetTimer()
    for i := 0; i < b.N; i++ {
        if matchFidelityGlob(pattern, rel) {
            b.Fatal("the deliberately shallower path must not match")
        }
    }
}

Run:

go test -run '^$' \
  -bench '^BenchmarkMatchFidelityGlob_LongTerminalStarWithoutGlobstar$' \
  -benchmem ./internal/mcp

At the exact PR head on an M1 Pro I measured:

72.6–73.3 µs/op
99,008 B/op

The input is only about 8 KB and contains no **. Since find_files scans all candidates before applying limit, that scales approximately to:

10,000 files    ~990 MB allocated, ~0.73 s matcher time
100,000 files   ~9.9 GB allocated, ~7.3 s matcher time

A larger but still valid MCP string scales the memory pressure proportionally. Memoization fixes the exponential walk, but the dense matrix and per-candidate compilation still allow one user-controlled glob to create substantial GC and CPU pressure.

Proposed correction

1. Compile/normalize the glob once per request, outside the candidate loop. A small compiledGlob can hold the normalized pattern and segment slice.
2. Invoke the globstar DP only when the normalized segments contain a whole-segment **. Ordinary *, literal patterns, and literal dir/* are already handled by matchSegmentGlob; strings.Contains(pattern, "**") is also broader than the actual globstar contract because a**b is an ordinary segment glob.
3. Add a reasonable maximum glob byte/segment count before scanning files.
4. Prefer sparse memoization or iterative DP with O(path depth) working memory rather than allocating the full pattern×path matrix for each match.

At minimum, the entry gate should be based on the already-added hasGlobstarSegment helper instead of strings.Contains(...) || HasSuffix("/*") || pattern == "*". That removes this non-globstar amplification, although compiling once and bounding complexity are still needed for long patterns containing a real **.

Please add a handler-level oversized/over-complex glob test and retain an allocation benchmark so the fix is protected at the consumer boundary.

Everything else from this round looks good: full internal/mcp tests, focused race tests, the Windows selector, tools/list byte ceilings, and all hosted checks pass. I found no additional correctness, auth, secret-handling, injection, or dead-code concern.

[tiendungdev]

Reproduced your benchmark at the previous head before touching anything — 99,009 B/op, 311 µs/op here for that ~8 KB pattern with no ** in it. Pushed 9b57aef9.

1 — the entry gate

strings.HasSuffix(pattern, "/*") and pattern == "*" let a non-globstar pattern into the walk on the theory that normalisation might rewrite the terminal star. That rewrite is gated on an existing **, so such a pattern could never gain a match there — it only paid. Your minimum fix, taken:

if patternHasGlobstarSegment(pattern) &&
    matchGlobstarSegments(globPatternSegments(pattern), strings.Split(rel, "/")) {

patternHasGlobstarSegment answers from four string checks and allocates nothing, and it agrees with hasGlobstarSegment on a**b being an ordinary segment glob — you were right that strings.Contains was broader than the contract, and the two are now pinned against each other in a test.

2 — working memory

The dense (pattern × path) matrix was itself the per-candidate allocation an oversized pattern inflated. Same dynamic program, run bottom-up over a single row: O(len(rel)) instead of O(len(pattern) × len(rel)), and the recursion is gone with it.

3 — the size bound

find_files refuses a glob over 1024 bytes or 64 segments before it walks anything. parseFidelityGlobs applies the same bound by dropping the clause — that parser is fail-soft by contract, and its rules also run per file.

Measured

	B/op	ns/op	allocs
before	99,009	311,547	6
after	16,384	128,821	2

The remainder is the pattern + "/" concatenation in the legacy prefix fallback, now capped by (3).

Coverage

You asked for the benchmark to be retained — it is. But a benchmark does not fail CI and nothing breaks when its numbers regress, so the ceiling is asserted too:

• TestMatchFidelityGlob_NonGlobstarPatternDoesNotEnterTheWalk — allocation ceiling, 40,000 B/op, sitting ~2.4× either side of the before/after figures
• TestFindFiles_GlobOversizedIsRejectedBeforeScanning — handler level, both byte and segment limits, plus a glob at the limit that must still be served
• TestParseFidelityGlobs_DropsOversizedClause
• TestMatchFidelityGlob_GlobstarGateIgnoresNonSegmentStars

Sabotage, separately: reverting the gate fails the allocation test at 49,904 B/op; removing the handler bound fails both cases of the oversized-glob test.

Worth noting from that first number — the gate and the row each carry part of the win. With the row in place but the gate reverted it is 49,904, not the original 99,009, so neither change alone accounts for it.

One thing I did not do

I skipped the per-request compiled glob. With the gate closed a non-globstar pattern no longer reaches normalisation at all, and a globstar one is bounded to 64 segments, so hoisting the split is a real but much smaller win — and it changes a signature both call sites share. Say the word and I will add it, either here or as a follow-up.

Verification

windows/amd64, go1.26.6, -count=1, against 06939fce:

internal/mcp   23 failures — the identical set as the previous head

gofmt and go vet clean.

[tiendungdev]

lint is red on the module proxy, not on this diff. It needs a re-run, which I cannot trigger from a fork.

internal/parser/forest/dump_kinds_test.go:32:11: could not import
  github.com/alexaandru/go-sitter-forest/al
  (...@v1.9.14: read "https://proxy.golang.org/.../al/@v/v1.9.14.zip":
   stream error: stream ID 9; INTERNAL_ERROR; received from peer) (typecheck)

A truncated download from proxy.golang.org, in a package this PR does not touch — the diff is confined to internal/mcp. golangci-lint run --timeout=10m ./internal/mcp/... locally on the pushed head: 0 issues.

Every other check is green, including build-windows with the FidelityGlob / FindFiles_Glob selectors and the benchmark job.

[zzet]

Thanks for the careful follow-up. The previous dense-memo/exponential issue is fixed: the one-row globstar DP is linear in pattern segments × path segments, and I independently compared it with a brute-force reference across 564,102 small states without a mismatch. The Windows selector also now runs the intended MCP tests.

This head still needs changes because the new resource admission can be bypassed on Windows.

P1 — Windows native separators bypass maxGlobSegments

globTooComplex counts / in the raw pattern, but matchFidelityGlob subsequently calls filepath.ToSlash. handleFindFiles therefore validates and reports the wrong segment count on Windows.

Copy-paste Windows reproduction:

func TestGlobTooComplexRejectsNativeSeparatorOverflow(t *testing.T) {
    if runtime.GOOS != "windows" {
        t.Skip("Windows-native separator case")
    }

    glob := strings.Repeat(`x\`, maxGlobSegments) + `**`
    require.Equal(t, maxGlobSegments+1,
        strings.Count(filepath.ToSlash(glob), "/")+1)

    require.True(t, globTooComplex(glob)) // fails: actual false
}

A 1002-byte alternating native-separator pattern is accepted as one segment but becomes 400 segments after normalization. Running the exact matcher algorithm after the same normalization measured about 96.6 µs and 14.8 KB allocated per candidate — approximately 9.7 seconds and 1.48 GB allocated for 100,000 files.

Please normalize once before validation, use that normalized representation for byte/segment counts, matching, and error reporting, and add Windows handler/parser tests for native and mixed separators. Compiling the pattern once also removes the repeated pattern split from the candidate loop.

P2 — Total fidelity_globs size and rule count remain unbounded

parseFidelityGlobs splits the complete request string and accepts every individually small clause:

spec := strings.Repeat("never-*:omit,", 100_000) + "**:full"
rules := parseFidelityGlobs(spec)
require.Len(t, rules, 100_001)

Matching then scans this request-controlled list linearly. Please enforce total specification bytes before splitting, cap the rule count, and reject over-budget requests explicitly at the handlers.

P2 — Oversized ordered fidelity policies are silently weakened

An oversized first-match omit rule is silently dropped, allowing a later full rule or ordinary compression to win. The public parameter description does not disclose this limit or fallback.

deep := strings.Repeat("x/", maxGlobSegments) + "**"
rules := parseFidelityGlobs(deep + ":omit,**:full")
decide := fidelityDecideForPath(
    rules,
    strings.Repeat("x/", maxGlobSegments)+"secret.go",
)
// Actual: full. Requested first-match policy: omit.

Complexity violations should reject the request rather than being handled like fail-soft syntax mistakes. Please add read_file and get_editing_context endpoint coverage proving an invalid omit rule cannot silently become compress or full.

P2 — Leading-globstar nonmatches retain quadratic allocation

After the linear DP rejects a leading **/ pattern, matchFidelityGlob repeatedly builds every remaining suffix with strings.Join:

rel := strings.Repeat("segment/", 2000) + "leaf.go"
matchFidelityGlob("**/never", rel)

This allocated about 16.2 MB for one nonmatching candidate in review testing. Preserve the legacy directory-prefix behavior with a linear segment scan or encode it in the compiled matcher without constructing suffix strings.

Finally, the at-limit fixture in tools_find_files_test.go constructs 63 segments, not 64; exactly 1024 bytes is also untested. Please add explicit 64/65-segment and 1024/1025-byte cases.

All hosted checks are green. I found no authentication, secret-handling, injection, or dead-code issue; the remaining blocker is resource admission and fidelity-policy correctness.

[tiendungdev]

All five reproduced at the previous head before I changed anything. Pushed c7c2b425.

P1  raw segments=1  normalised=65  tooComplex=false   (130-byte glob)
P2a 1.3 MB spec -> 100,001 rules, 23.5 ms
P2b oversized first-match omit dropped -> `full` wins
P2c one 2000-segment nonmatch -> 17,024,952 B
fixture: 63 segments, not 64

P1 — made unrepresentable rather than patched

The bound read the raw pattern while the matcher read the normalised one. Rather than move the ToSlash call, the pattern is now compiled once into a compiledGlob that normalises first, and tooComplex is a method on it: there is no way to ask about complexity without holding the normalised form, and no way to obtain one except through compileGlob. The same change removes the per-candidate split the last round left in — that is the compile-once you asked for.

P2 — the ordered-policy hazard is the one that worried me

Dropping an over-budget rule was not a cosmetic difference. First-match ordering means an oversized omit disappearing lets a later full win, so the file the request asked to hide comes back in a response that looks like a normal successful read. parseFidelityGlobs now returns an error for an admission failure while still skipping malformed clauses — the two failure modes are deliberately different, and the doc comment says why.

Admission moved to the top of each handler. My first attempt validated at the point of use, inside if len(fileBytes) > 0, which would have served a malformed request through any path that did not reach the compressor. Worth mentioning since it is the kind of placement that tests pass over.

P2 — spec and rule-count bounds

8192 bytes, 64 rules. Both are per-request multipliers on a per-file scan.

P2 — the leading-**/ fallback

It rebuilt every suffix with strings.Join. Walking separator offsets and slicing rel shares the original bytes instead.

Boundaries

Both limits at and one past: 64/65 segments, 1024/1025 bytes, plus native and mixed separators. The native-separator case asserts the fixture is small before normalisation, so it cannot silently stop testing the bypass.

Sabotage, each separately

reverted	result
count segments on the raw pattern	oversized-glob test fails
drop the over-budget rule instead of erroring	both endpoint subtests fail — "must refuse the request rather than serve it under a weakened policy"
restore the strings.Join suffix rebuild	17,024,846 B for one candidate, against a 1 MB ceiling

One thing you should decide

The schema now discloses the limits and that violating them is an error, as you asked — but it did not fit. The core preset had 29 bytes of headroom and the sentence cost 274. I made room by tightening existing wording inside the same constant (96,465 of 96,500 now) rather than moving the ceiling.

That is a judgement call I made to keep the PR green, and it edits prose you did not ask me to touch. If you would rather keep the fuller phrasing and raise the ceiling, say so and I will swap it — the budget, not the wording, is what constrains this description now.

Verification

windows/amd64, go1.26.6, -count=1, against 06939fce:

internal/mcp   23 failures — the identical set as the previous head
go build ./... clean

gofmt and go vet clean on every touched file.

[tiendungdev]

The macOS shard was right and it was mine, not a flake. Pushed d233f8e5.

Error: "1" is not greater than "64"
FAIL github.com/zzet/gortex/internal/mcp

That is the native-separator fixture I added last push:

require.Greater(t, strings.Count(filepath.ToSlash(glob), "/")+1, maxGlobSegments)

It asserts that ToSlash expands the pattern past the bound — a Windows fact. On POSIX a backslash is an ordinary filename byte, ToSlash is a no-op, and the fixture stays one legitimate segment. I wrote a comment saying exactly that three lines below, and still left the assertion outside the branch.

The expansion claim now sits inside the Windows branch; POSIX asserts what is true there instead — the pattern is admitted, because there is no expansion and therefore no bypass to guard.

The mixed-separator subtest had the mirror defect: it only asserted on Windows, so on POSIX it was an empty subtest that passed while testing nothing. Its fixture already exceeds the bound on forward slashes alone, so it is now refused on both platforms, for different reasons.

On the root cause

I have been verifying on Windows only, and every assertion I wrote was true there. That is not a defensible way to work on a cross-platform matcher, so I set up a real POSIX run:

windows/amd64 go1.26.6            ok
linux/amd64   golang:1.26 docker  ok   internal/mcp  2.806s

Both from now on.

Worth noting how close I came to shipping a false green on that too: my first two docker attempts returned exit 0 with no output at all, because MSYS rewrote -w /src into a Windows path and the container never ran go test. An empty pass is not a pass — I only trusted the run once I saw it download modules, compile the tree-sitter cgo bindings, and print ok.

The ubuntu shard is CANCELLED, not failed — fail-fast cancelled it when macOS went red, so its log carries no signal about this diff.