Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/core/src/core/coreToolScheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,6 @@ const TOOL_FAILURE_KIND_BACKGROUND_AGENT_DENIED = 'background_agent_denied';

const TOOL_SPAN_STATUS_PRE_HOOK_BLOCKED = 'Tool execution blocked by hook';


const TOOL_SPAN_STATUS_POST_HOOK_STOPPED = 'Tool execution stopped by hook';
const TOOL_SPAN_STATUS_PERMISSION_DENIED = 'Permission denied for tool';
const TOOL_SPAN_STATUS_PERMISSION_HOOK_DENIED =
Expand Down Expand Up @@ -3737,7 +3736,8 @@ export class CoreToolScheduler {
for (const candidatePath of candidatePaths) {
// Inject conditional rules at most once per session per rule
// file. The registry tracks dedup internally.
const rulesCtx = rulesRegistry?.matchAndConsume(candidatePath);
const rulesCtx =
await rulesRegistry?.matchAndConsume(candidatePath);
if (rulesCtx) reminderBlocks.push(rulesCtx);
}

Expand Down
139 changes: 106 additions & 33 deletions packages/core/src/skills/skill-activation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,21 @@ function makeSkill(overrides: Partial<SkillConfig>): SkillConfig {
}

describe('splitConditionalSkills', () => {
it('treats skills without paths as unconditional', () => {
it('treats skills without paths as unconditional', async () => {
const skills = [makeSkill({ name: 'a' })];
const { unconditional, conditional } = splitConditionalSkills(skills);
expect(unconditional).toHaveLength(1);
expect(conditional).toHaveLength(0);
});

it('treats empty paths array as unconditional', () => {
it('treats empty paths array as unconditional', async () => {
const skills = [makeSkill({ name: 'a', paths: [] })];
const { unconditional, conditional } = splitConditionalSkills(skills);
expect(unconditional).toHaveLength(1);
expect(conditional).toHaveLength(0);
});

it('classifies skills with non-empty paths as conditional', () => {
it('classifies skills with non-empty paths as conditional', async () => {
const skills = [
makeSkill({ name: 'a' }),
makeSkill({ name: 'b', paths: ['src/**/*.tsx'] }),
Expand All @@ -53,72 +53,74 @@ describe('splitConditionalSkills', () => {
describe('SkillActivationRegistry', () => {
const projectRoot = '/project';

it('returns empty when no conditional skills are registered', () => {
it('returns empty when no conditional skills are registered', async () => {
const reg = new SkillActivationRegistry([], projectRoot);
expect(reg.matchAndConsume('/project/src/App.tsx')).toEqual([]);
expect(await reg.matchAndConsume('/project/src/App.tsx')).toEqual([]);
expect(reg.totalCount).toBe(0);
});

it('activates a conditional skill when a matching path is touched', () => {
it('activates a conditional skill when a matching path is touched', async () => {
const reg = new SkillActivationRegistry(
[makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] })],
projectRoot,
);
const newly = reg.matchAndConsume('/project/src/App.tsx');
const newly = await reg.matchAndConsume('/project/src/App.tsx');
expect(newly).toEqual(['tsx-helper']);
expect(reg.isActivated('tsx-helper')).toBe(true);
expect(reg.activatedCount).toBe(1);
});

it('does not re-activate an already-active skill on subsequent matches', () => {
it('does not re-activate an already-active skill on subsequent matches', async () => {
const reg = new SkillActivationRegistry(
[makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] })],
projectRoot,
);
expect(reg.matchAndConsume('/project/src/A.tsx')).toEqual(['tsx-helper']);
expect(await reg.matchAndConsume('/project/src/A.tsx')).toEqual([
'tsx-helper',
]);
// Second touch of the same pattern returns nothing new.
expect(reg.matchAndConsume('/project/src/B.tsx')).toEqual([]);
expect(await reg.matchAndConsume('/project/src/B.tsx')).toEqual([]);
expect(reg.activatedCount).toBe(1);
});

it('returns empty for paths that do not match any skill', () => {
it('returns empty for paths that do not match any skill', async () => {
const reg = new SkillActivationRegistry(
[makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] })],
projectRoot,
);
expect(reg.matchAndConsume('/project/lib/utils.py')).toEqual([]);
expect(await reg.matchAndConsume('/project/lib/utils.py')).toEqual([]);
});

it('activates multiple skills whose globs overlap on a single file', () => {
it('activates multiple skills whose globs overlap on a single file', async () => {
const reg = new SkillActivationRegistry(
[
makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] }),
makeSkill({ name: 'app-helper', paths: ['src/App.tsx'] }),
],
projectRoot,
);
const newly = reg.matchAndConsume('/project/src/App.tsx');
const newly = await reg.matchAndConsume('/project/src/App.tsx');
expect(newly.sort()).toEqual(['app-helper', 'tsx-helper']);
});

it('accepts relative file paths by resolving against the project root', () => {
it('accepts relative file paths by resolving against the project root', async () => {
const reg = new SkillActivationRegistry(
[makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] })],
projectRoot,
);
expect(reg.matchAndConsume('src/App.tsx')).toEqual(['tsx-helper']);
expect(await reg.matchAndConsume('src/App.tsx')).toEqual(['tsx-helper']);
});

it('ignores paths outside the project root', () => {
it('ignores paths outside the project root', async () => {
const reg = new SkillActivationRegistry(
[makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] })],
projectRoot,
);
expect(reg.matchAndConsume('/other/project/src/App.tsx')).toEqual([]);
expect(await reg.matchAndConsume('/other/project/src/App.tsx')).toEqual([]);
expect(reg.activatedCount).toBe(0);
});

it('supports multiple glob patterns per skill (OR semantics)', () => {
it('supports multiple glob patterns per skill (OR semantics)', async () => {
const reg = new SkillActivationRegistry(
[
makeSkill({
Expand All @@ -129,11 +131,13 @@ describe('SkillActivationRegistry', () => {
projectRoot,
);
// Both patterns should activate the same skill, but only once total.
expect(reg.matchAndConsume('/project/test/foo.ts')).toEqual(['multi']);
expect(reg.matchAndConsume('/project/src/Bar.tsx')).toEqual([]);
expect(await reg.matchAndConsume('/project/test/foo.ts')).toEqual([
'multi',
]);
expect(await reg.matchAndConsume('/project/src/Bar.tsx')).toEqual([]);
});

it('activates broad globs on dotfiles too (dot: true semantics)', () => {
it('activates broad globs on dotfiles too (dot: true semantics)', async () => {
// Regression: picomatch `dot: false` would silently exclude
// `.eslintrc.js`, `.env`, `.github/*.yml`, etc. from broad globs.
// Skill activation is "did the model touch a file matching this
Expand All @@ -143,12 +147,12 @@ describe('SkillActivationRegistry', () => {
[makeSkill({ name: 'lint-helper', paths: ['**/*.js'] })],
projectRoot,
);
expect(reg.matchAndConsume('/project/.eslintrc.js')).toEqual([
expect(await reg.matchAndConsume('/project/.eslintrc.js')).toEqual([
'lint-helper',
]);
});

it('survives an invalid picomatch pattern (drops it, keeps the rest)', () => {
it('survives an invalid picomatch pattern (drops it, keeps the rest)', async () => {
// Regression: picomatch can throw on pathological patterns
// (oversized strings, broken extglob nesting). The constructor
// previously let that throw escape and abort skill loading
Expand All @@ -169,10 +173,10 @@ describe('SkillActivationRegistry', () => {
);
expect(reg.totalCount).toBe(1);
// The good pattern still works.
expect(reg.matchAndConsume('/project/src/App.ts')).toEqual(['mixed']);
expect(await reg.matchAndConsume('/project/src/App.ts')).toEqual(['mixed']);
});

it('rejects an absolute relative path (Windows cross-drive case)', () => {
it('rejects an absolute relative path (Windows cross-drive case)', async () => {
// Regression: on Windows, `path.relative('C:\\project', 'D:\\other')`
// returns an absolute path like `D:\\other`. After normalizing
// backslashes to forward slashes, broad globs like `**/*.ts` would
Expand All @@ -188,7 +192,9 @@ describe('SkillActivationRegistry', () => {
// as the existing `..` guard; on Windows the new isAbsolute branch
// catches it. Either way the registry must return [] for paths outside
// the project root regardless of platform.
expect(reg.matchAndConsume('/totally/other/place/file.ts')).toEqual([]);
expect(await reg.matchAndConsume('/totally/other/place/file.ts')).toEqual(
[],
);
expect(reg.activatedCount).toBe(0);
});
});
Expand All @@ -198,7 +204,7 @@ describe('resolveProjectRelativePath', () => {
// 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)', () => {
it('returns the forward-slash-normalized relative path for in-project files (POSIX)', async () => {
expect(
resolveProjectRelativePath(
'/project/src/App.tsx',
Expand All @@ -208,13 +214,13 @@ describe('resolveProjectRelativePath', () => {
).toBe('src/App.tsx');
});

it('returns null for paths outside the project root (POSIX, `..` prefix)', () => {
it('returns null for paths outside the project root (POSIX, `..` prefix)', async () => {
expect(
resolveProjectRelativePath('/elsewhere/foo.ts', '/project', path.posix),
).toBeNull();
});

it('returns null for Windows cross-drive paths (different drive letter)', () => {
it('returns null for Windows cross-drive paths (different drive letter)', async () => {
// 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
Expand All @@ -230,7 +236,7 @@ describe('resolveProjectRelativePath', () => {
).toBeNull();
});

it('normalizes backslashes for in-project Windows paths', () => {
it('normalizes backslashes for in-project Windows paths', async () => {
expect(
resolveProjectRelativePath(
'C:\\project\\src\\App.tsx',
Expand Down Expand Up @@ -267,7 +273,7 @@ describe('extractToolFilePaths → SkillActivationRegistry integration', () => {
// collect the union.
const activated = new Set<string>();
for (const c of candidates) {
for (const n of reg.matchAndConsume(c)) activated.add(n);
for (const n of await reg.matchAndConsume(c)) activated.add(n);
}
expect(Array.from(activated)).toEqual(['tsx-helper']);
}, 30_000);
Expand All @@ -286,8 +292,75 @@ describe('extractToolFilePaths → SkillActivationRegistry integration', () => {
);
const activated = new Set<string>();
for (const c of candidates) {
for (const n of reg.matchAndConsume(c)) activated.add(n);
for (const n of await reg.matchAndConsume(c)) activated.add(n);
}
expect(activated.size).toBe(0);
}, 30_000);

it.skipIf(process.platform === 'win32')(
'activates skills when file is reached via symlinked directory',
async () => {
const fs = await import('node:fs/promises');
const path = await import('node:path');
const os = await import('node:os');
const testRoot = await fs.mkdtemp(
path.join(os.tmpdir(), 'symlink-test-'),
);
const projectRoot = path.join(testRoot, 'project');
const srcDir = path.join(projectRoot, 'src');
await fs.mkdir(srcDir, { recursive: true });
const symlinkDir = path.join(projectRoot, 'symlink-to-src');
await fs.symlink(srcDir, symlinkDir);

// Create the actual file so realpath can resolve it
const realFile = path.join(srcDir, 'App.tsx');
await fs.writeFile(realFile, '// test');

const reg = new SkillActivationRegistry(
[makeSkill({ name: 'tsx-helper', paths: ['src/**/*.tsx'] })],
projectRoot,
);

// Access file via symlinked path
const symlinkedFile = path.join(symlinkDir, 'App.tsx');
const result = await reg.matchAndConsume(symlinkedFile);
expect(result).toEqual(['tsx-helper']);

await fs.rm(testRoot, { recursive: true, force: true });
},
);

it.skipIf(process.platform === 'win32')(
'activates skills when project root itself is a symlink',
async () => {
const fs = await import('node:fs/promises');
const path = await import('node:path');
const os = await import('node:os');
const testRoot = await fs.mkdtemp(
path.join(os.tmpdir(), 'symlink-test-'),
);
const realProject = path.join(testRoot, 'real-project');
await fs.mkdir(realProject, { recursive: true });
const srcDir = path.join(realProject, 'src');
await fs.mkdir(srcDir, { recursive: true });
const symlinkProject = path.join(testRoot, 'symlink-project');
await fs.symlink(realProject, symlinkProject);

// Create the actual file so realpath can resolve it
const realFile = path.join(srcDir, 'foo.ts');
await fs.writeFile(realFile, '// test');

const reg = new SkillActivationRegistry(
[makeSkill({ name: 'ts-helper', paths: ['src/**/*.ts'] })],
symlinkProject,
);

const result = await reg.matchAndConsume(
path.join(symlinkProject, 'src', 'foo.ts'),
);
expect(result).toEqual(['ts-helper']);

await fs.rm(testRoot, { recursive: true, force: true });
},
);
});
25 changes: 14 additions & 11 deletions packages/core/src/skills/skill-activation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import picomatch from 'picomatch';
import type { SkillConfig } from './types.js';
import { createDebugLogger } from '../utils/debugLogger.js';
import { resolveProjectRelativePath } from '../utils/projectPath.js';
import { resolveSymlinkAwareRelativePaths } from '../utils/projectPath.js';

// Re-export so existing consumers (skill-activation.test.ts) keep
// working through the old import path. The canonical home is now
Expand Down Expand Up @@ -124,31 +124,34 @@ export class SkillActivationRegistry {
* Activate any conditional skills whose `paths` globs match `filePath`.
* Returns the names of skills newly activated by this call (empty when
* either no skill matched, or every match was already active).
*
* Handles symlinked paths by checking both the original path and the
* realpath-resolved path, so skills work correctly in git worktrees and
* monorepos with symlinked directories.
*/
matchAndConsume(filePath: string): string[] {
async matchAndConsume(filePath: string): Promise<string[]> {
if (this.compiled.length === 0) return [];

// 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) {
// Resolve symlinks and check both original and realpath-resolved paths
const relativePaths = await resolveSymlinkAwareRelativePaths(
filePath,
this.projectRoot,
);
if (relativePaths.length === 0) {
debugLogger.debug(
`Skipping ${filePath}: outside project root or cross-drive`,
);
return [];
}
debugLogger.debug(`matchAndConsume ${filePath} → relative=${relativePath}`);

const newlyActivated: string[] = [];
for (const { skill, matchers } of this.compiled) {
if (this.activated.has(skill.name)) continue;
if (matchers.some((m) => m(relativePath))) {
if (relativePaths.some((relPath) => matchers.some((m) => m(relPath)))) {
this.activated.add(skill.name);
newlyActivated.push(skill.name);
debugLogger.info(
`Activated skill "${skill.name}" via path "${relativePath}"`,
`Activated skill "${skill.name}" via path "${relativePaths.join(' or ')}"`,
);
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/skills/skill-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,7 @@ export class SkillManager {
if (!registry || filePaths.length === 0) return [];
const newlyAcrossPaths = new Set<string>();
for (const filePath of filePaths) {
for (const name of registry.matchAndConsume(filePath)) {
for (const name of await registry.matchAndConsume(filePath)) {
newlyAcrossPaths.add(name);
}
}
Expand Down
Loading
Loading