From 6aaa200edb6444a1c65362dbf103a6c920ade957 Mon Sep 17 00:00:00 2001 From: Haitao Pan Date: Wed, 25 Mar 2026 16:01:01 +0800 Subject: [PATCH] Refine single-agent local skill root precedence --- lib/app/app_controller_desktop.dart | 136 ++++++++--- .../app_controller_thread_skills_suite.dart | 211 +++++++++++++++++- 2 files changed, 308 insertions(+), 39 deletions(-) diff --git a/lib/app/app_controller_desktop.dart b/lib/app/app_controller_desktop.dart index e9f36cf0..f145eb60 100644 --- a/lib/app/app_controller_desktop.dart +++ b/lib/app/app_controller_desktop.dart @@ -4107,24 +4107,34 @@ class AppController extends ChangeNotifier { Future> _scanSingleAgentLocalSkillEntries() async { final dedupedByName = {}; + final dedupedPriorityByName = {}; for (final rootSpec in _singleAgentLocalSkillScanRoots) { - final root = Directory(_resolveSingleAgentSkillRootPath(rootSpec.path)); - if (!await root.exists()) { - continue; - } - await for (final entity in root.list( - recursive: true, - followLinks: false, + final rootPriority = _singleAgentSkillRootPriority(rootSpec); + for (final resolvedRootPath in _resolveSingleAgentSkillRootPaths( + rootSpec.path, )) { - if (entity is! File || entity.uri.pathSegments.last != 'SKILL.md') { + final root = Directory(resolvedRootPath); + if (!await root.exists()) { continue; } - final entry = await _skillEntryFromFile(entity, rootSpec); - final normalizedName = entry.label.trim().toLowerCase(); - if (normalizedName.isEmpty) { - continue; + await for (final entity in root.list( + recursive: true, + followLinks: false, + )) { + if (entity is! File || entity.uri.pathSegments.last != 'SKILL.md') { + continue; + } + final entry = await _skillEntryFromFile(entity, rootSpec); + final normalizedName = entry.label.trim().toLowerCase(); + if (normalizedName.isEmpty) { + continue; + } + final existingPriority = dedupedPriorityByName[normalizedName]; + if (existingPriority == null || rootPriority >= existingPriority) { + dedupedByName[normalizedName] = entry; + dedupedPriorityByName[normalizedName] = rootPriority; + } } - dedupedByName[normalizedName] = entry; } } final entries = dedupedByName.values.toList(growable: false); @@ -4132,6 +4142,13 @@ class AppController extends ChangeNotifier { return entries; } + int _singleAgentSkillRootPriority(_SingleAgentSkillScanRoot root) { + return switch (root.scope) { + 'workspace' => 0, + _ => 1, + }; + } + List<_SingleAgentSkillScanRoot> _resolveDefaultSingleAgentSkillScanRoots() { return <_SingleAgentSkillScanRoot>[ ..._defaultGatewayOnlySkillScanRoots, @@ -4156,25 +4173,29 @@ class AppController extends ChangeNotifier { _SingleAgentSkillScanRoot _singleAgentSkillScanRootFromOverride( String rawPath, ) { - final normalizedPath = rawPath.trim(); + final normalizedPath = rawPath.trim().replaceFirst(RegExp(r'^\./'), ''); final lowered = normalizedPath.toLowerCase(); - final workspacePath = settings.workspacePath.trim(); - final normalizedWorkspace = workspacePath.endsWith('/') - ? workspacePath - : '$workspacePath/'; + final workspaceBases = _singleAgentRelativeSkillRootBasePaths(); final inferredWorkspace = lowered.contains('/workspace/.agents/') || lowered.contains('/workspace/.claude/') || - lowered.contains('/workspace/.codex/'); + lowered.contains('/workspace/.codex/') || + lowered.contains('/workspace/.workbuddy/'); + final explicitWorkspaceRoot = + lowered.startsWith('.agents/') || + lowered.startsWith('.claude/') || + lowered.startsWith('.codex/') || + lowered.startsWith('.workbuddy/'); + final scopedToWorkspace = workspaceBases.any((basePath) { + final normalizedBase = basePath.endsWith('/') + ? basePath + : '$basePath/'; + return normalizedPath == basePath || + normalizedPath.startsWith(normalizedBase); + }); final scope = normalizedPath.startsWith('/etc/') ? 'system' - : (workspacePath.isNotEmpty && - (normalizedPath == workspacePath || - normalizedPath.startsWith(normalizedWorkspace)) || - inferredWorkspace || - lowered.startsWith('.agents/') || - lowered.startsWith('.claude/') || - lowered.startsWith('.codex/')) + : (scopedToWorkspace || inferredWorkspace || explicitWorkspaceRoot) ? 'workspace' : 'user'; return _SingleAgentSkillScanRoot( @@ -4184,24 +4205,48 @@ class AppController extends ChangeNotifier { ); } - String _resolveSingleAgentSkillRootPath(String rawPath) { - final trimmed = rawPath.trim(); + List _resolveSingleAgentSkillRootPaths(String rawPath) { + final trimmed = rawPath.trim().replaceFirst(RegExp(r'^\./'), ''); if (trimmed.isEmpty) { - return trimmed; + return const []; } if (trimmed.startsWith('/')) { - return trimmed; + return [trimmed]; } if (trimmed.startsWith('~/')) { final home = Platform.environment['HOME']?.trim() ?? ''; - return home.isEmpty ? trimmed : '$home/${trimmed.substring(2)}'; + return [home.isEmpty ? trimmed : '$home/${trimmed.substring(2)}']; } - final workspacePath = settings.workspacePath.trim(); - if (workspacePath.isNotEmpty) { - return '$workspacePath/$trimmed'; + return _singleAgentRelativeSkillRootBasePaths() + .map((basePath) => '$basePath/$trimmed') + .toList(growable: false); + } + + List _singleAgentRelativeSkillRootBasePaths() { + final paths = []; + final seen = {}; + + void addCandidate(String rawPath) { + final trimmed = rawPath.trim(); + if (trimmed.isEmpty) { + return; + } + final normalized = trimmed.endsWith('/') + ? trimmed.substring(0, trimmed.length - 1) + : trimmed; + if (normalized.isEmpty || !seen.add(normalized)) { + return; + } + paths.add(normalized); } - final home = Platform.environment['HOME']?.trim() ?? ''; - return home.isEmpty ? trimmed : '$home/$trimmed'; + + addCandidate(settings.workspacePath); + try { + addCandidate(Directory.current.path); + } catch (_) { + // Best effort only for current workspace fallback discovery. + } + return paths; } String _sourceForSkillRootPath(String path) { @@ -4248,7 +4293,10 @@ class AppController extends ChangeNotifier { .where((item) => item.isNotEmpty) .last) .trim(); - final rootPath = _resolveSingleAgentSkillRootPath(root.path); + final rootPath = _resolveBestSingleAgentSkillRootPath( + directory.path, + root.path, + ); final relativeSource = directory.path.startsWith(rootPath) ? directory.path .substring(rootPath.length) @@ -4272,6 +4320,20 @@ class AppController extends ChangeNotifier { ); } + String _resolveBestSingleAgentSkillRootPath( + String targetPath, + String rawRootPath, + ) { + final candidates = _resolveSingleAgentSkillRootPaths(rawRootPath) + ..sort((left, right) => right.length.compareTo(left.length)); + for (final candidate in candidates) { + if (targetPath.startsWith(candidate)) { + return candidate; + } + } + return candidates.isNotEmpty ? candidates.first : rawRootPath.trim(); + } + void _restoreAssistantThreads(List records) { _assistantThreadRecords.clear(); _assistantThreadMessages.clear(); diff --git a/test/runtime/app_controller_thread_skills_suite.dart b/test/runtime/app_controller_thread_skills_suite.dart index 8ec5b383..738b49c6 100644 --- a/test/runtime/app_controller_thread_skills_suite.dart +++ b/test/runtime/app_controller_thread_skills_suite.dart @@ -453,25 +453,36 @@ void main() { ); test( - 'AppController resolves relative single-agent skill roots against workspace path', + 'AppController uses settings.workspacePath as fallback for relative single-agent skill roots', () async { SharedPreferences.setMockInitialValues({}); final tempDirectory = await Directory.systemTemp.createTemp( 'xworkmate-workspace-local-skills-', ); + final currentWorkspace = await Directory.systemTemp.createTemp( + 'xworkmate-empty-current-workspace-', + ); + final originalCurrentDirectory = Directory.current; addTearDown(() async { + Directory.current = originalCurrentDirectory; if (await tempDirectory.exists()) { try { await tempDirectory.delete(recursive: true); } catch (_) {} } + if (await currentWorkspace.exists()) { + try { + await currentWorkspace.delete(recursive: true); + } catch (_) {} + } }); + Directory.current = currentWorkspace.path; await _writeSkill( Directory('${tempDirectory.path}/.codex/skills'), 'workspace-only', skillName: 'Workspace Only Skill', - description: 'Repo-local skill should be discovered by default roots', + description: 'Default workspace fallback should be discovered', ); final store = SecureConfigStore( @@ -505,6 +516,202 @@ void main() { ); }, ); + + test( + 'AppController keeps high-priority user roots ahead of workspace fallbacks', + () async { + SharedPreferences.setMockInitialValues({}); + final tempDirectory = await Directory.systemTemp.createTemp( + 'xworkmate-priority-relative-skills-', + ); + final currentWorkspace = Directory('${tempDirectory.path}/workspace'); + await currentWorkspace.create(recursive: true); + final workbuddyRoot = Directory('${tempDirectory.path}/workbuddy-skills'); + await _writeSkill( + workbuddyRoot, + 'shared-skill', + skillName: 'Shared Skill', + description: 'High-priority user root wins', + ); + await _writeSkill( + Directory('${currentWorkspace.path}/.codex/skills'), + 'shared-skill', + skillName: 'Shared Skill', + description: 'Workspace fallback should not override user roots', + ); + + final originalCurrentDirectory = Directory.current; + addTearDown(() async { + Directory.current = originalCurrentDirectory; + if (await tempDirectory.exists()) { + try { + await tempDirectory.delete(recursive: true); + } catch (_) {} + } + }); + Directory.current = currentWorkspace.path; + + final store = SecureConfigStore( + enableSecureStorage: false, + databasePathResolver: () async => '${tempDirectory.path}/settings.sqlite3', + fallbackDirectoryPathResolver: () async => tempDirectory.path, + ); + await store.initialize(); + await store.saveSettingsSnapshot( + SettingsSnapshot.defaults().copyWith(workspacePath: currentWorkspace.path), + ); + + final controller = AppController( + store: store, + availableSingleAgentProvidersOverride: const [ + SingleAgentProvider.codex, + ], + singleAgentLocalSkillScanRoots: [ + workbuddyRoot.path, + '.codex/skills', + ], + ); + addTearDown(controller.dispose); + await _waitFor(() => !controller.initializing); + await controller.setAssistantExecutionTarget( + AssistantExecutionTarget.singleAgent, + ); + + final sharedSkill = controller + .assistantImportedSkillsForSession(controller.currentSessionKey) + .firstWhere((item) => item.label == 'Shared Skill'); + expect(sharedSkill.description, 'High-priority user root wins'); + expect(sharedSkill.source, 'workbuddy'); + }, + ); + + test( + 'AppController prefers current workspace roots over settings.workspacePath fallback', + () async { + SharedPreferences.setMockInitialValues({}); + final tempDirectory = await Directory.systemTemp.createTemp( + 'xworkmate-current-workspace-skills-', + ); + final defaultWorkspace = Directory( + '${tempDirectory.path}/default-workspace', + ); + final currentWorkspace = Directory( + '${tempDirectory.path}/current-workspace', + ); + await currentWorkspace.create(recursive: true); + await _writeSkill( + Directory('${defaultWorkspace.path}/.codex/skills'), + 'shared-skill', + skillName: 'Shared Skill', + description: 'Default workspace fallback', + ); + await _writeSkill( + Directory('${currentWorkspace.path}/.codex/skills'), + 'shared-skill', + skillName: 'Shared Skill', + description: 'Current workspace wins', + ); + + final originalCurrentDirectory = Directory.current; + addTearDown(() async { + Directory.current = originalCurrentDirectory; + if (await tempDirectory.exists()) { + try { + await tempDirectory.delete(recursive: true); + } catch (_) {} + } + }); + Directory.current = currentWorkspace.path; + + final store = SecureConfigStore( + enableSecureStorage: false, + databasePathResolver: () async => '${tempDirectory.path}/settings.sqlite3', + fallbackDirectoryPathResolver: () async => tempDirectory.path, + ); + await store.initialize(); + await store.saveSettingsSnapshot( + SettingsSnapshot.defaults().copyWith( + workspacePath: defaultWorkspace.path, + ), + ); + + final controller = AppController( + store: store, + availableSingleAgentProvidersOverride: const [ + SingleAgentProvider.codex, + ], + singleAgentLocalSkillScanRoots: const ['.codex/skills'], + ); + addTearDown(controller.dispose); + await _waitFor(() => !controller.initializing); + await controller.setAssistantExecutionTarget( + AssistantExecutionTarget.singleAgent, + ); + + final sharedSkill = controller + .assistantImportedSkillsForSession(controller.currentSessionKey) + .firstWhere((item) => item.label == 'Shared Skill'); + expect(sharedSkill.description, 'Current workspace wins'); + }, + ); + + test( + 'AppController can return empty skills when relative roots have no matching workspace roots', + () async { + SharedPreferences.setMockInitialValues({}); + final tempDirectory = await Directory.systemTemp.createTemp( + 'xworkmate-empty-relative-skills-', + ); + final emptyWorkspace = await Directory.systemTemp.createTemp( + 'xworkmate-empty-relative-current-workspace-', + ); + final originalCurrentDirectory = Directory.current; + addTearDown(() async { + Directory.current = originalCurrentDirectory; + if (await tempDirectory.exists()) { + try { + await tempDirectory.delete(recursive: true); + } catch (_) {} + } + if (await emptyWorkspace.exists()) { + try { + await emptyWorkspace.delete(recursive: true); + } catch (_) {} + } + }); + Directory.current = emptyWorkspace.path; + + final store = SecureConfigStore( + enableSecureStorage: false, + databasePathResolver: () async => '${tempDirectory.path}/settings.sqlite3', + fallbackDirectoryPathResolver: () async => tempDirectory.path, + ); + await store.initialize(); + await store.saveSettingsSnapshot( + SettingsSnapshot.defaults().copyWith( + workspacePath: '', + ), + ); + + final controller = AppController( + store: store, + availableSingleAgentProvidersOverride: const [ + SingleAgentProvider.codex, + ], + singleAgentLocalSkillScanRoots: const ['.codex/skills'], + ); + addTearDown(controller.dispose); + await _waitFor(() => !controller.initializing); + await controller.setAssistantExecutionTarget( + AssistantExecutionTarget.singleAgent, + ); + + expect( + controller.assistantImportedSkillsForSession(controller.currentSessionKey), + isEmpty, + ); + }, + ); } Future _writeSkill(