mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-24 13:39:58 +00:00
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:
parent
599490b916
commit
7cb7145bbe
3 changed files with 92 additions and 7 deletions
|
|
@ -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', () => {
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue