Skip to content

fix(glob): match the pattern relative to the path base dir, not the project root#2978

Open
Tatlatat wants to merge 2 commits into
esengine:v1from
Tatlatat:fix/glob-match-relative-to-path
Open

fix(glob): match the pattern relative to the path base dir, not the project root#2978
Tatlatat wants to merge 2 commits into
esengine:v1from
Tatlatat:fix/glob-match-relative-to-path

Conversation

@Tatlatat
Copy link
Copy Markdown
Contributor

@Tatlatat Tatlatat commented Jun 3, 2026

What

The glob tool documents its path argument as "Base directory to walk … The
pattern matches relative to this path."
But globFiles walked from startAbs
while testing the pattern against the path relative to ctx.rootDir (project
root).

So glob(path: "src", pattern: "*.ts") compared "src/index.ts" against
"*.ts", matched nothing, and returned (no matches) — even though index.ts
matches *.ts relative to src.

Fix

Match against a path relative to startAbs (the walk root), while keeping the
displayed result relative to rootDir so output still shows src/index.ts:

const rel = displayRel(ctx.rootDir, full);      // for display (unchanged)
const matchRel = displayRel(startAbs, full);     // for matching (new)
if (!isMatch(matchRel)) continue;

When no path is given (startAbs == rootDir) behavior is unchanged.

Test

Added a case: globbing *.ts under path: "src" finds src/index.ts and does
not return (no matches). Verified it fails on the old code and passes on the
fix (123 filesystem-tool tests pass).

Verification

npx vitest run tests/filesystem-tools.test.ts (123 passed); biome check
clean on the changed files.

The glob tool documents its path argument as "Base directory to walk … The
pattern matches relative to this path." But globFiles walked from startAbs
while testing the pattern against the path relative to ctx.rootDir. So
glob(path: "src", pattern: "*.ts") compared "src/index.ts" to "*.ts",
matched nothing, and returned "(no matches)" even though index.ts matches
*.ts relative to src.

Match against a path relative to startAbs (the walk root) while keeping the
displayed result relative to rootDir, so output still shows "src/index.ts".
When no path is given (startAbs == rootDir) behavior is unchanged. Add a
test that globbing *.ts under path "src" finds src/index.ts.
@github-actions github-actions Bot added the v1 Legacy TypeScript line (0.x) — v1 branch, maintenance only label Jun 3, 2026
@HUQIANTAO
Copy link
Copy Markdown
Contributor

Thanks for the thorough investigation and clean fix — the diagnosis is spot-on for the TypeScript implementation.

Worth noting: the main codebase has since been rewritten in Go, and the Go glob tool (internal/tool/builtin/glob.go) doesn't have this bug. Its matchGlobSuffix already matches against the path relative to the walk root (not the project root), and it has no path parameter — the working directory is set at construction time via resolveIn(workDir, pattern).

A few suggestions:

  1. Scope: Since v1 is the legacy TypeScript branch and the active development is on main-v2 (Go), this fix only benefits users still on v1. If v1 is still shipping, it's worth merging; if it's been superseded, you might want to check with the maintainers whether v1 is still maintained before investing more time here.

  2. Edge case: You may want to add a test for when path is an absolute path (e.g. /tmp/test) — displayRel with an absolute startAbs should still produce correct relative paths for matching. Also worth testing path: "." (current directory) to confirm it doesn't break.

Nice catch overall. The single-line fix is exactly right.

Per review feedback: add a case where path is an absolute directory (the
match must still resolve relative to it) and a path "." case confirming
top-level *.ts matches while nested files are not pulled in by the
non-recursive pattern.
@Tatlatat
Copy link
Copy Markdown
Contributor Author

Tatlatat commented Jun 4, 2026

Thanks for the careful review! Added both edge-case tests in the same describe block:

  • Absolute path: globs with an absolute directory arg and asserts the .ts file resolves (and not (no matches)). Confirmed it fails on the pre-fix code and passes after — so it exercises the fix, not just the happy path.
  • path: ".": asserts a top-level *.ts is found while a nested sub/a.ts is not pulled in by the non-recursive pattern, guarding against a .-base regression.

npx vitest run tests/filesystem-tools.test.ts → 125 passed; biome check clean.

On scope: agreed this is v1-only — the Go rewrite's glob already does the right thing (no path param, matches relative to the walk root), so nothing to port there. I targeted the v1 branch since v0.x is still the npm latest line and v1 is labeled the maintenance branch; happy to close this if v1 is no longer shipping fixes — your call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v1 Legacy TypeScript line (0.x) — v1 branch, maintenance only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants