fix(skills): join glob.path with glob.pattern as effective selector

Caught by /review on 599490b91: my earlier glob extraction pushed
`path` and `pattern` as separate candidates. `glob({ path: 'src',
pattern: '**/*.ts' })` produced `['src', '**/*.ts']` — neither
component matches a skill keyed on `paths: ['src/**/*.ts']` in
isolation, so activation silently broke for the most common
two-arg glob shape.

The glob call actually searches `<path>/<pattern>`. Replace the
standalone pattern push with `path.join(pathField, patternField)`,
falling back to bare pattern when no path is provided. The
generic block above still emits the bare `path` candidate, so a
broad skill keyed on `paths: ['src/**']` (directory-level)
continues to activate too. Combined output for the regression
example: `['src', 'src/**/*.ts']` — covers both the directory-
level and file-level skill cases.

Adds three tests: an updated unit test pinning the joined
effective selector, an absolute-`path` variant whose joined form
gets rejected downstream by the project-root guard
(`/tmp/external/**/*.ts`), and the audit-suggested integration
regression that pipes `extractToolFilePaths` output straight into
`SkillActivationRegistry` and verifies a `paths: ['src/**/*.ts']`
skill activates from `glob({ path: 'src', pattern: '**/*.ts' })`.
This commit is contained in:
wenshao 2026-04-30 20:29:53 +08:00
parent 599490b916
commit 7cb7145bbe
3 changed files with 92 additions and 7 deletions

View file

@ -4391,11 +4391,34 @@ describe('extractToolFilePaths', () => {
expect(extractToolFilePaths('glob', { pattern: 'src/**/*.tsx' })).toEqual([
'src/**/*.tsx',
]);
// Both `path` (search root) and `pattern` (selector) when both
// present.
});
it('joins glob.path + glob.pattern into the effective selector', () => {
// Regression: glob({ path: 'src', pattern: '**/*.ts' }) actually
// searches src/**/*.ts. Emitting them as separate candidates
// ('src', '**/*.ts') would NOT activate a skill keyed on
// `paths: ['src/**/*.ts']`, because neither component matches the
// skill glob in isolation. Join them with path.join so the
// effective-selector candidate reflects what the tool really
// touched. (The standalone `path` candidate is still emitted by the
// generic block above so a broad skill keyed on `paths: ['src/**']`
// still matches.)
expect(
extractToolFilePaths('glob', { path: 'src', pattern: '**/*.ts' }),
).toEqual(['src', '**/*.ts']);
).toEqual(['src', 'src/**/*.ts']);
});
it('joins absolute glob.path with pattern (registry guard rejects downstream)', () => {
// glob({ path: '/tmp/external', pattern: '**/*.ts' }) joins to an
// absolute path. SkillActivationRegistry's project-root guard
// rejects it; the test pins the joined shape so absolute roots
// stay distinguishable from project-relative ones.
expect(
extractToolFilePaths('glob', {
path: '/tmp/external',
pattern: '**/*.ts',
}),
).toEqual(['/tmp/external', '/tmp/external/**/*.ts']);
});
it('does not extract pattern for non-glob tools', () => {

View file

@ -47,6 +47,7 @@ import type {
Part,
PartListUnion,
} from '@google/genai';
import * as path from 'node:path';
import { ToolNames } from '../tools/tool-names.js';
import { CONCURRENCY_SAFE_KINDS } from '../tools/tools.js';
import { isShellCommandReadOnly } from '../utils/shellReadOnlyChecker.js';
@ -247,10 +248,24 @@ export function extractToolFilePaths(
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']);
// Glob-only: derive the effective selector from `path` + `pattern`.
// The previous version pushed `pattern` standalone, but the glob
// call actually searches `<path>/<pattern>` — so a skill keyed on
// `paths: ['src/**/*.ts']` would not activate from
// `glob({ path: 'src', pattern: '**/*.ts' })` because the candidate
// was just `**/*.ts`. Joining matches what the tool really touched.
// (Don't add `pattern` for grep_search even though it also has a
// `pattern` field — grep's pattern is a regex, not a path glob,
// and would false-match.)
const pathField = obj['path'];
const patternField = obj['pattern'];
if (typeof patternField === 'string' && patternField.length > 0) {
if (typeof pathField === 'string' && pathField.length > 0) {
push(path.join(pathField, patternField));
} else {
push(patternField);
}
}
}
return out;
}

View file

@ -201,3 +201,50 @@ describe('resolveProjectRelativePath', () => {
).toBe('src/App.tsx');
});
});
describe('extractToolFilePaths → SkillActivationRegistry integration', () => {
// Regression: feed the real candidate output for a `glob` call into
// the registry and assert end-to-end activation. The earlier per-field
// extraction (path + pattern as separate candidates) silently failed
// to activate skills keyed on the joined effective selector — there
// was no test exercising the path that mattered.
it('activates a skill keyed on src/**/*.ts from glob({ path: "src", pattern: "**/*.ts" })', async () => {
const { extractToolFilePaths } = await import(
'../core/coreToolScheduler.js'
);
const candidates = extractToolFilePaths('glob', {
path: 'src',
pattern: '**/*.ts',
});
const reg = new SkillActivationRegistry(
[makeSkill({ name: 'tsx-helper', paths: ['src/**/*.ts'] })],
'/project',
);
// Hand each candidate the registry the way coreToolScheduler does;
// collect the union.
const activated = new Set<string>();
for (const c of candidates) {
for (const n of reg.matchAndConsume(c)) activated.add(n);
}
expect(Array.from(activated)).toEqual(['tsx-helper']);
});
it('does NOT activate from external glob.path (project-root guard wins)', async () => {
const { extractToolFilePaths } = await import(
'../core/coreToolScheduler.js'
);
const candidates = extractToolFilePaths('glob', {
path: '/tmp/external',
pattern: '**/*.ts',
});
const reg = new SkillActivationRegistry(
[makeSkill({ name: 'broad', paths: ['**/*.ts'] })],
'/project',
);
const activated = new Set<string>();
for (const c of candidates) {
for (const n of reg.matchAndConsume(c)) activated.add(n);
}
expect(activated.size).toBe(0);
});
});