diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index b02fe0c37..04dfdb378 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -4383,6 +4383,32 @@ describe('extractToolFilePaths', () => { ).toEqual([]); }); + it('extracts pattern for glob (path-shaped selector, glob-only)', () => { + // Regression: `glob({ pattern: 'src/**/*.tsx' })` with no `path` is a + // common shape that previously produced an empty candidate set, so a + // skill keyed on `paths: ['src/**/*.tsx']` would never activate from + // a glob call. + expect(extractToolFilePaths('glob', { pattern: 'src/**/*.tsx' })).toEqual([ + 'src/**/*.tsx', + ]); + // Both `path` (search root) and `pattern` (selector) when both + // present. + expect( + extractToolFilePaths('glob', { path: 'src', pattern: '**/*.ts' }), + ).toEqual(['src', '**/*.ts']); + }); + + it('does not extract pattern for non-glob tools', () => { + // Grep's `pattern` is a regex, not a path glob; treating it as a + // path would false-match. Pattern is only path-shaped for `glob`. + expect( + extractToolFilePaths('grep_search', { + pattern: 'TODO|FIXME', + filePath: '/proj/a.ts', + }), + ).toEqual(['/proj/a.ts']); + }); + it('returns empty for tool names outside the FS allowlist', () => { // Regression: MCP tools and other non-FS tools that happen to use // `path` / `paths` for non-filesystem semantics (e.g. URL routes, diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 9adc11508..3e00231cc 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -207,13 +207,24 @@ const FS_PATH_TOOL_NAMES: ReadonlySet = new Set([ * Pull the filesystem path-bearing fields out of a tool's input. Different * core tools name these differently: * - `file_path` — read-file, edit, write-file - * - `path` — ls, ripGrep + * - `path` — ls (search root); glob (search root, optional) * - `filePath` — grep, lsp * - `paths` — ripGrep array form + * - `pattern` — glob (treated as a path-shaped glob, see below) * Used by ConditionalRulesRegistry / SkillActivationRegistry hooks to * route every path the tool just touched through the same activation * pipeline. Returns input order, with empty / non-string entries dropped. * + * For `glob` specifically, `pattern` is the actual selector — `path` is + * only the optional search root. `glob({ pattern: 'src/**\/*.tsx' })` + * with no `path` therefore needs `pattern` extracted, otherwise no + * conditional skill keyed on `paths: ['src/**\/*.tsx']` would ever + * activate from a glob call. The pattern goes through the same + * registry; picomatch matches glob-vs-glob on common shapes (e.g. + * `**\/*.tsx` matches `src/**\/*.tsx` because `**` accepts the + * literal `**` in the input). False matches are bounded to the same + * project-root scope as any other path. + * * Returns `[]` for tool names outside `FS_PATH_TOOL_NAMES` — see that * set's docstring for why this is gated rather than universal. */ @@ -235,6 +246,12 @@ export function extractToolFilePaths( if (Array.isArray(pathsField)) { for (const p of pathsField) push(p); } + if (toolName === ToolNames.GLOB) { + // Glob-only: include `pattern` as a path-shaped selector. Don't add + // it for grep_search even though grep also has a `pattern` field — + // grep's pattern is a regex, not a path glob, and would false-match. + push(obj['pattern']); + } return out; } diff --git a/packages/core/src/skills/skill-activation.test.ts b/packages/core/src/skills/skill-activation.test.ts index 6a14622e3..71cd81870 100644 --- a/packages/core/src/skills/skill-activation.test.ts +++ b/packages/core/src/skills/skill-activation.test.ts @@ -4,9 +4,11 @@ * SPDX-License-Identifier: Apache-2.0 */ +import * as path from 'node:path'; import { describe, expect, it } from 'vitest'; import { SkillActivationRegistry, + resolveProjectRelativePath, splitConditionalSkills, } from './skill-activation.js'; import type { SkillConfig } from './types.js'; @@ -151,3 +153,51 @@ describe('SkillActivationRegistry', () => { expect(reg.activatedCount).toBe(0); }); }); + +describe('resolveProjectRelativePath', () => { + // Pure helper, exercised directly so the Windows-specific cross-drive + // branch is testable on POSIX CI runners. On a real POSIX host + // `path.win32` always behaves like Windows path semantics regardless + // of the host OS. + it('returns the forward-slash-normalized relative path for in-project files (POSIX)', () => { + expect( + resolveProjectRelativePath( + '/project/src/App.tsx', + '/project', + path.posix, + ), + ).toBe('src/App.tsx'); + }); + + it('returns null for paths outside the project root (POSIX, `..` prefix)', () => { + expect( + resolveProjectRelativePath('/elsewhere/foo.ts', '/project', path.posix), + ).toBeNull(); + }); + + it('returns null for Windows cross-drive paths (different drive letter)', () => { + // Direct exercise of the new `path.isAbsolute(rawRelativePath)` + // branch. `path.win32.relative('C:\\project', 'D:\\other\\file.ts')` + // returns an absolute string like `D:\\other\\file.ts` — without the + // isAbsolute guard, the helper would normalize the backslashes and + // return `D:/other/file.ts`, which would false-match a broad glob + // such as `**/*.ts`. Must return null instead. + expect( + resolveProjectRelativePath( + 'D:\\other\\file.ts', + 'C:\\project', + path.win32, + ), + ).toBeNull(); + }); + + it('normalizes backslashes for in-project Windows paths', () => { + expect( + resolveProjectRelativePath( + 'C:\\project\\src\\App.tsx', + 'C:\\project', + path.win32, + ), + ).toBe('src/App.tsx'); + }); +}); diff --git a/packages/core/src/skills/skill-activation.ts b/packages/core/src/skills/skill-activation.ts index c0ff666c7..b0a809a3a 100644 --- a/packages/core/src/skills/skill-activation.ts +++ b/packages/core/src/skills/skill-activation.ts @@ -27,6 +27,35 @@ interface CompiledSkill { readonly matchers: picomatch.Matcher[]; } +/** + * Compute a project-relative, forward-slash-normalized path for matching + * against skill `paths:` globs, or `null` if the input falls outside the + * project root. Pure (no I/O), and parameterized over a `path` module so + * unit tests can pin the Windows-specific `path.win32` cross-drive case + * (where `path.relative('C:\\proj', 'D:\\elsewhere')` returns an + * absolute string that, after normalizing backslashes, would otherwise + * false-match a broad glob like `**\/*.ts`). + */ +export function resolveProjectRelativePath( + filePath: string, + projectRoot: string, + pathModule: typeof path = path, +): string | null { + const absolutePath = pathModule.isAbsolute(filePath) + ? filePath + : pathModule.resolve(projectRoot, filePath); + const rawRelativePath = pathModule.relative(projectRoot, absolutePath); + if ( + rawRelativePath === '..' || + rawRelativePath.startsWith(`..${pathModule.sep}`) || + rawRelativePath.startsWith('../') || + pathModule.isAbsolute(rawRelativePath) + ) { + return null; + } + return rawRelativePath.replace(/\\/g, '/'); +} + /** * Splits a skill list into unconditional skills (no `paths:`) and conditional * skills (with non-empty `paths:`). Unconditional skills are always offered to @@ -79,30 +108,17 @@ export class SkillActivationRegistry { matchAndConsume(filePath: string): string[] { if (this.compiled.length === 0) return []; - const absolutePath = path.isAbsolute(filePath) - ? filePath - : path.resolve(this.projectRoot, filePath); - const rawRelativePath = path.relative(this.projectRoot, absolutePath); - - // Skip files outside the project root — conditional skills are scoped to - // the project, matching ConditionalRulesRegistry's behavior. - // - // On Windows, `path.relative` between paths on different drives returns - // an absolute path (e.g. `D:\\other`), which would otherwise normalize - // to forward slashes and false-match a glob like `**/*.ts`. Reject - // absolute results before normalizing. - if ( - rawRelativePath === '..' || - rawRelativePath.startsWith(`..${path.sep}`) || - rawRelativePath.startsWith('../') || - path.isAbsolute(rawRelativePath) - ) { + // Skip files outside the project root — conditional skills are scoped + // to the project, matching ConditionalRulesRegistry's behavior. The + // helper handles the Windows cross-drive case (where `path.relative` + // returns an absolute string). + const relativePath = resolveProjectRelativePath(filePath, this.projectRoot); + if (relativePath === null) { debugLogger.debug( - `Skipping ${filePath}: outside project root (relative=${rawRelativePath})`, + `Skipping ${filePath}: outside project root or cross-drive`, ); return []; } - const relativePath = rawRelativePath.replace(/\\/g, '/'); debugLogger.debug(`matchAndConsume ${filePath} → relative=${relativePath}`); const newlyActivated: string[] = [];