From 26b2c63c287297bb1f30a1300544b2efaf58c40e Mon Sep 17 00:00:00 2001 From: Haitao Pan Date: Wed, 25 Mar 2026 22:48:57 +0800 Subject: [PATCH] fix(assistant): require bookmarks for shared skill roots --- lib/app/app_controller_desktop.dart | 24 +- lib/runtime/skill_directory_access.dart | 135 ++++++--- macos/Runner/AppDelegate.swift | 51 +++- macos/Runner/MainFlutterWindow.swift | 3 + .../app_controller_thread_skills_suite.dart | 276 ++++++++++++++++++ 5 files changed, 430 insertions(+), 59 deletions(-) diff --git a/lib/app/app_controller_desktop.dart b/lib/app/app_controller_desktop.dart index a5cd31b6..0beafb6b 100644 --- a/lib/app/app_controller_desktop.dart +++ b/lib/app/app_controller_desktop.dart @@ -30,6 +30,7 @@ import '../runtime/desktop_thread_artifact_service.dart'; import '../runtime/mode_switcher.dart'; import '../runtime/agent_registry.dart'; import '../runtime/multi_agent_orchestrator.dart'; +import '../runtime/platform_environment.dart'; import '../runtime/single_agent_runner.dart'; import '../runtime/skill_directory_access.dart'; @@ -266,6 +267,7 @@ class AppController extends ChangeNotifier { String? _bootstrapError; StreamSubscription? _runtimeEventsSubscription; bool _disposed = false; + String _resolvedUserHomeDirectory = resolveUserHomeDirectory(); SettingsSnapshot _lastObservedSettingsSnapshot = SettingsSnapshot.defaults(); Future _assistantThreadPersistQueue = Future.value(); Future _settingsObservationQueue = Future.value(); @@ -276,6 +278,8 @@ class AppController extends ChangeNotifier { _singleAgentSharedSkillScanRootFromOverride, ))?.toList(growable: false) ?? _defaultSingleAgentGlobalSkillScanRoots; + final requiresAuthorizedSharedRoots = + _skillDirectoryAccessService.requiresAuthorizedSharedRoots; final authorizedByPath = { for (final directory in settings.authorizedSkillDirectories) normalizeAuthorizedSkillDirectoryPath(directory.path): directory, @@ -288,11 +292,16 @@ class AppController extends ChangeNotifier { continue; } final authorizedDirectory = authorizedByPath.remove(resolvedPath); - resolvedRoots.add( - root.copyWith(bookmark: authorizedDirectory?.bookmark ?? ''), - ); + final bookmark = authorizedDirectory?.bookmark.trim() ?? ''; + if (requiresAuthorizedSharedRoots && bookmark.isEmpty) { + continue; + } + resolvedRoots.add(root.copyWith(bookmark: bookmark)); } for (final directory in authorizedByPath.values) { + if (requiresAuthorizedSharedRoots && directory.bookmark.trim().isEmpty) { + continue; + } resolvedRoots.add( _singleAgentSharedSkillScanRootFromAuthorizedDirectory(directory), ); @@ -368,7 +377,7 @@ class AppController extends ChangeNotifier { _defaultSingleAgentGlobalSkillScanRoots .map((item) => item.path) .toList(growable: false); - String get userHomeDirectory => Platform.environment['HOME']?.trim() ?? ''; + String get userHomeDirectory => _resolvedUserHomeDirectory; String get settingsYamlPath => defaultUserSettingsFilePath() ?? ''; bool get hasSettingsDraftChanges => settingsDraft.toJsonString() != settings.toJsonString() || @@ -2964,6 +2973,8 @@ class AppController extends ChangeNotifier { Future _initialize() async { try { + _resolvedUserHomeDirectory = await _skillDirectoryAccessService + .resolveUserHomeDirectory(); await _settingsController.initialize(); _restoreAssistantThreads(await _store.loadAssistantThreadRecords()); await _restoreSharedSingleAgentLocalSkillsCache(); @@ -4342,7 +4353,8 @@ class AppController extends ChangeNotifier { ); } - _SingleAgentSkillScanRoot _singleAgentSharedSkillScanRootFromAuthorizedDirectory( + _SingleAgentSkillScanRoot + _singleAgentSharedSkillScanRootFromAuthorizedDirectory( AuthorizedSkillDirectory directory, ) { final normalizedPath = normalizeAuthorizedSkillDirectoryPath( @@ -4369,7 +4381,7 @@ class AppController extends ChangeNotifier { return trimmed; } if (trimmed.startsWith('~/')) { - final home = Platform.environment['HOME']?.trim() ?? ''; + final home = _resolvedUserHomeDirectory.trim(); return home.isEmpty ? trimmed : '$home/${trimmed.substring(2)}'; } final normalizedWorkspace = workspaceRef.trim(); diff --git a/lib/runtime/skill_directory_access.dart b/lib/runtime/skill_directory_access.dart index f487cfde..d92316e3 100644 --- a/lib/runtime/skill_directory_access.dart +++ b/lib/runtime/skill_directory_access.dart @@ -4,10 +4,13 @@ import 'dart:io'; import 'package:file_selector/file_selector.dart'; import 'package:flutter/services.dart'; +import 'platform_environment.dart'; import 'runtime_models.dart'; abstract class SkillDirectoryAccessService { bool get isSupported; + bool get requiresAuthorizedSharedRoots; + Future resolveUserHomeDirectory(); Future authorizeDirectory({ String suggestedPath = '', @@ -48,6 +51,14 @@ class UnsupportedSkillDirectoryAccessService @override bool get isSupported => false; + @override + bool get requiresAuthorizedSharedRoots => false; + + @override + Future resolveUserHomeDirectory() async { + return _fallbackUserHomeDirectory(); + } + @override Future authorizeDirectory({ String suggestedPath = '', @@ -68,6 +79,14 @@ class FileSelectorSkillDirectoryAccessService @override bool get isSupported => true; + @override + bool get requiresAuthorizedSharedRoots => false; + + @override + Future resolveUserHomeDirectory() async { + return _fallbackUserHomeDirectory(); + } + @override Future authorizeDirectory({ String suggestedPath = '', @@ -104,31 +123,53 @@ class MacOsSkillDirectoryAccessService implements SkillDirectoryAccessService { static const MethodChannel _channel = MethodChannel( 'plus.svc.xworkmate/skill_directory_access', ); + final FileSelectorSkillDirectoryAccessService _fallbackService = + FileSelectorSkillDirectoryAccessService(); @override bool get isSupported => true; + @override + bool get requiresAuthorizedSharedRoots => true; + + @override + Future resolveUserHomeDirectory() async { + try { + final response = await _channel.invokeMethod( + 'resolveUserHomeDirectory', + ); + final trimmed = response?.trim() ?? ''; + return trimmed.isEmpty ? _fallbackUserHomeDirectory() : trimmed; + } on MissingPluginException { + return _fallbackUserHomeDirectory(); + } + } + @override Future authorizeDirectory({ String suggestedPath = '', }) async { - final response = await _channel.invokeMapMethod( - 'authorizeDirectory', - {'suggestedPath': suggestedPath}, - ); - if (response == null) { - return null; + try { + final response = await _channel.invokeMapMethod( + 'authorizeDirectory', + {'suggestedPath': suggestedPath}, + ); + if (response == null) { + return null; + } + final normalized = normalizeAuthorizedSkillDirectoryPath( + response['path']?.toString() ?? '', + ); + if (normalized.isEmpty) { + return null; + } + return AuthorizedSkillDirectory( + path: normalized, + bookmark: response['bookmark']?.toString().trim() ?? '', + ); + } on MissingPluginException { + return _fallbackService.authorizeDirectory(suggestedPath: suggestedPath); } - final normalized = normalizeAuthorizedSkillDirectoryPath( - response['path']?.toString() ?? '', - ); - if (normalized.isEmpty) { - return null; - } - return AuthorizedSkillDirectory( - path: normalized, - bookmark: response['bookmark']?.toString().trim() ?? '', - ); } @override @@ -149,37 +190,45 @@ class MacOsSkillDirectoryAccessService implements SkillDirectoryAccessService { onClose: () async {}, ); } - final response = await _channel.invokeMapMethod( - 'startDirectoryAccess', - {'bookmark': bookmark}, - ); - if (response == null) { - return null; + try { + final response = await _channel.invokeMapMethod( + 'startDirectoryAccess', + {'bookmark': bookmark}, + ); + if (response == null) { + return null; + } + final accessId = response['accessId']?.toString().trim() ?? ''; + final resolvedPath = normalizeAuthorizedSkillDirectoryPath( + response['path']?.toString() ?? normalizedPath, + ); + if (accessId.isEmpty || resolvedPath.isEmpty) { + return null; + } + final refreshedBookmark = + response['bookmark']?.toString().trim().isNotEmpty == true + ? response['bookmark'].toString().trim() + : directory.bookmark; + return SkillDirectoryAccessHandle( + path: resolvedPath, + refreshedBookmark: refreshedBookmark, + onClose: () async { + await _channel.invokeMethod( + 'stopDirectoryAccess', + {'accessId': accessId}, + ); + }, + ); + } on MissingPluginException { + return _fallbackService.openDirectory(directory); } - final accessId = response['accessId']?.toString().trim() ?? ''; - final resolvedPath = normalizeAuthorizedSkillDirectoryPath( - response['path']?.toString() ?? normalizedPath, - ); - if (accessId.isEmpty || resolvedPath.isEmpty) { - return null; - } - final refreshedBookmark = - response['bookmark']?.toString().trim().isNotEmpty == true - ? response['bookmark'].toString().trim() - : directory.bookmark; - return SkillDirectoryAccessHandle( - path: resolvedPath, - refreshedBookmark: refreshedBookmark, - onClose: () async { - await _channel.invokeMethod( - 'stopDirectoryAccess', - {'accessId': accessId}, - ); - }, - ); } } +String _fallbackUserHomeDirectory() { + return resolveUserHomeDirectory(); +} + String _initialDirectoryForSuggestion(String suggestedPath) { final trimmed = normalizeAuthorizedSkillDirectoryPath(suggestedPath); if (trimmed.isEmpty) { diff --git a/macos/Runner/AppDelegate.swift b/macos/Runner/AppDelegate.swift index 9e9df0aa..98f9ea48 100644 --- a/macos/Runner/AppDelegate.swift +++ b/macos/Runner/AppDelegate.swift @@ -1,10 +1,13 @@ import Cocoa +import Darwin import FlutterMacOS @main class AppDelegate: FlutterAppDelegate { private let skillDirectoryChannelName = "plus.svc.xworkmate/skill_directory_access" private var directoryAccessSessions: [String: URL] = [:] + private var skillDirectoryChannel: FlutterMethodChannel? + private var skillDirectoryMessengerId: ObjectIdentifier? override func applicationDidFinishLaunching(_ notification: Notification) { super.applicationDidFinishLaunching(notification) @@ -12,13 +15,7 @@ class AppDelegate: FlutterAppDelegate { guard let controller = mainFlutterWindow?.contentViewController as? FlutterViewController else { return } - let channel = FlutterMethodChannel( - name: skillDirectoryChannelName, - binaryMessenger: controller.engine.binaryMessenger - ) - channel.setMethodCallHandler { [weak self] call, result in - self?.handleSkillDirectoryCall(call, result: result) - } + registerSkillDirectoryChannel(for: controller) } override func applicationShouldTerminateAfterLastWindowClosed(_ sender: NSApplication) -> Bool { @@ -37,8 +34,27 @@ class AppDelegate: FlutterAppDelegate { super.applicationWillTerminate(notification) } + func registerSkillDirectoryChannel(for controller: FlutterViewController) { + let messengerObject = controller.engine.binaryMessenger as AnyObject + let messengerId = ObjectIdentifier(messengerObject) + if skillDirectoryMessengerId == messengerId { + return + } + let channel = FlutterMethodChannel( + name: skillDirectoryChannelName, + binaryMessenger: controller.engine.binaryMessenger + ) + channel.setMethodCallHandler { [weak self] call, result in + self?.handleSkillDirectoryCall(call, result: result) + } + skillDirectoryChannel = channel + skillDirectoryMessengerId = messengerId + } + private func handleSkillDirectoryCall(_ call: FlutterMethodCall, result: @escaping FlutterResult) { switch call.method { + case "resolveUserHomeDirectory": + result(resolveUserHomeDirectoryPath()) case "authorizeDirectory": authorizeDirectory(call, result: result) case "startDirectoryAccess": @@ -170,10 +186,10 @@ class AppDelegate: FlutterAppDelegate { private func initialDirectoryURL(for suggestedPath: String) -> URL? { let trimmed = suggestedPath.trimmingCharacters(in: .whitespacesAndNewlines) if trimmed.isEmpty { - return FileManager.default.homeDirectoryForCurrentUser + return URL(fileURLWithPath: resolveUserHomeDirectoryPath(), isDirectory: true) } - var candidate = URL(fileURLWithPath: (trimmed as NSString).expandingTildeInPath) + var candidate = URL(fileURLWithPath: expandUserPath(trimmed)) var isDirectory: ObjCBool = false while true { if FileManager.default.fileExists(atPath: candidate.path, isDirectory: &isDirectory) { @@ -185,6 +201,21 @@ class AppDelegate: FlutterAppDelegate { } candidate = parent } - return FileManager.default.homeDirectoryForCurrentUser + return URL(fileURLWithPath: resolveUserHomeDirectoryPath(), isDirectory: true) + } + + private func expandUserPath(_ path: String) -> String { + guard path.hasPrefix("~/") else { + return path + } + let relative = String(path.dropFirst(2)) + return (resolveUserHomeDirectoryPath() as NSString).appendingPathComponent(relative) + } + + private func resolveUserHomeDirectoryPath() -> String { + if let directoryPointer = getpwuid(getuid())?.pointee.pw_dir { + return String(cString: directoryPointer) + } + return FileManager.default.homeDirectoryForCurrentUser.path } } diff --git a/macos/Runner/MainFlutterWindow.swift b/macos/Runner/MainFlutterWindow.swift index 3772395e..9ca46cfb 100644 --- a/macos/Runner/MainFlutterWindow.swift +++ b/macos/Runner/MainFlutterWindow.swift @@ -9,6 +9,9 @@ class MainFlutterWindow: NSWindow { self.setFrame(windowFrame, display: true) RegisterGeneratedPlugins(registry: flutterViewController) + (NSApp.delegate as? AppDelegate)?.registerSkillDirectoryChannel( + for: flutterViewController + ) super.awakeFromNib() diff --git a/test/runtime/app_controller_thread_skills_suite.dart b/test/runtime/app_controller_thread_skills_suite.dart index 87097dc7..22e358e5 100644 --- a/test/runtime/app_controller_thread_skills_suite.dart +++ b/test/runtime/app_controller_thread_skills_suite.dart @@ -8,6 +8,7 @@ import 'package:shared_preferences/shared_preferences.dart'; import 'package:xworkmate/app/app_controller.dart'; import 'package:xworkmate/runtime/runtime_models.dart'; import 'package:xworkmate/runtime/secure_config_store.dart'; +import 'package:xworkmate/runtime/skill_directory_access.dart'; void main() { test( @@ -207,6 +208,240 @@ void main() { }, ); + test( + 'AppController resolves preset shared roots against the access service home directory', + () async { + SharedPreferences.setMockInitialValues({}); + final tempDirectory = await Directory.systemTemp.createTemp( + 'xworkmate-skill-directory-home-', + ); + addTearDown(() async { + if (await tempDirectory.exists()) { + try { + await tempDirectory.delete(recursive: true); + } catch (_) {} + } + }); + final userHome = Directory('${tempDirectory.path}/real-home'); + final agentsRoot = Directory('${userHome.path}/.agents/skills'); + await _writeSkill( + agentsRoot, + 'browser', + skillName: 'Browser', + description: 'Browser tasks', + ); + + final controller = AppController( + store: await _createStore(tempDirectory.path), + skillDirectoryAccessService: _FakeSkillDirectoryAccessService( + userHomeDirectory: userHome.path, + ), + availableSingleAgentProvidersOverride: const [ + SingleAgentProvider.codex, + ], + singleAgentSharedSkillScanRootOverrides: const [ + '~/.agents/skills', + ], + ); + addTearDown(controller.dispose); + await _waitFor(() => !controller.initializing); + await controller.setAssistantExecutionTarget( + AssistantExecutionTarget.singleAgent, + ); + await _waitFor( + () => controller + .assistantImportedSkillsForSession(controller.currentSessionKey) + .any((item) => item.label == 'Browser'), + ); + + expect(controller.userHomeDirectory, userHome.path); + expect( + controller + .assistantImportedSkillsForSession(controller.currentSessionKey) + .map((item) => item.label), + contains('Browser'), + ); + }, + ); + + test( + 'AppController skips preset shared roots without bookmarks when the access service requires authorization', + () async { + SharedPreferences.setMockInitialValues({}); + final tempDirectory = await Directory.systemTemp.createTemp( + 'xworkmate-skill-directory-macos-preset-unauthorized-', + ); + addTearDown(() async { + if (await tempDirectory.exists()) { + try { + await tempDirectory.delete(recursive: true); + } catch (_) {} + } + }); + final userHome = Directory('${tempDirectory.path}/real-home'); + final agentsRoot = Directory('${userHome.path}/.agents/skills'); + await _writeSkill( + agentsRoot, + 'browser', + skillName: 'Browser', + description: 'Browser tasks', + ); + + final controller = AppController( + store: await _createStore(tempDirectory.path), + skillDirectoryAccessService: _FakeSkillDirectoryAccessService( + userHomeDirectory: userHome.path, + requiresAuthorizedSharedRoots: true, + ), + availableSingleAgentProvidersOverride: const [ + SingleAgentProvider.codex, + ], + singleAgentSharedSkillScanRootOverrides: const [ + '~/.agents/skills', + ], + ); + addTearDown(controller.dispose); + await _waitFor(() => !controller.initializing); + await controller.setAssistantExecutionTarget( + AssistantExecutionTarget.singleAgent, + ); + await Future.delayed(const Duration(milliseconds: 100)); + + expect( + controller + .assistantImportedSkillsForSession(controller.currentSessionKey) + .where((item) => item.label == 'Browser'), + isEmpty, + ); + }, + ); + + test( + 'AppController scans preset shared roots with bookmarks when the access service requires authorization', + () async { + SharedPreferences.setMockInitialValues({}); + final tempDirectory = await Directory.systemTemp.createTemp( + 'xworkmate-skill-directory-macos-preset-authorized-', + ); + addTearDown(() async { + if (await tempDirectory.exists()) { + try { + await tempDirectory.delete(recursive: true); + } catch (_) {} + } + }); + final userHome = Directory('${tempDirectory.path}/real-home'); + final agentsRoot = Directory('${userHome.path}/.agents/skills'); + await _writeSkill( + agentsRoot, + 'browser', + skillName: 'Browser', + description: 'Browser tasks', + ); + + final store = await _createStore(tempDirectory.path); + await store.saveSettingsSnapshot( + _singleAgentTestSettings(workspacePath: tempDirectory.path).copyWith( + authorizedSkillDirectories: [ + AuthorizedSkillDirectory( + path: agentsRoot.path, + bookmark: 'bookmark-1', + ), + ], + ), + ); + final controller = AppController( + store: store, + skillDirectoryAccessService: _FakeSkillDirectoryAccessService( + userHomeDirectory: userHome.path, + requiresAuthorizedSharedRoots: true, + ), + availableSingleAgentProvidersOverride: const [ + SingleAgentProvider.codex, + ], + singleAgentSharedSkillScanRootOverrides: const [ + '~/.agents/skills', + ], + ); + addTearDown(controller.dispose); + await _waitFor(() => !controller.initializing); + await controller.setAssistantExecutionTarget( + AssistantExecutionTarget.singleAgent, + ); + await _waitFor( + () => controller + .assistantImportedSkillsForSession(controller.currentSessionKey) + .any((item) => item.label == 'Browser'), + ); + + expect( + controller + .assistantImportedSkillsForSession(controller.currentSessionKey) + .map((item) => item.label), + contains('Browser'), + ); + }, + ); + + test( + 'AppController skips custom shared directories without bookmarks when the access service requires authorization', + () async { + SharedPreferences.setMockInitialValues({}); + final tempDirectory = await Directory.systemTemp.createTemp( + 'xworkmate-skill-directory-macos-custom-unauthorized-', + ); + addTearDown(() async { + if (await tempDirectory.exists()) { + try { + await tempDirectory.delete(recursive: true); + } catch (_) {} + } + }); + final customRoot = Directory( + '${tempDirectory.path}/custom-shared-skills', + ); + await _writeSkill( + customRoot, + 'browser', + skillName: 'Browser', + description: 'Browser tasks', + ); + + final store = await _createStore(tempDirectory.path); + await store.saveSettingsSnapshot( + _singleAgentTestSettings(workspacePath: tempDirectory.path).copyWith( + authorizedSkillDirectories: [ + AuthorizedSkillDirectory(path: customRoot.path), + ], + ), + ); + final controller = AppController( + store: store, + skillDirectoryAccessService: _FakeSkillDirectoryAccessService( + userHomeDirectory: tempDirectory.path, + requiresAuthorizedSharedRoots: true, + ), + availableSingleAgentProvidersOverride: const [ + SingleAgentProvider.codex, + ], + singleAgentSharedSkillScanRootOverrides: const [], + ); + addTearDown(controller.dispose); + await _waitFor(() => !controller.initializing); + await controller.setAssistantExecutionTarget( + AssistantExecutionTarget.singleAgent, + ); + await Future.delayed(const Duration(milliseconds: 100)); + + expect( + controller + .assistantImportedSkillsForSession(controller.currentSessionKey) + .where((item) => item.label == 'Browser'), + isEmpty, + ); + }, + ); + test( 'AppController keeps thread-bound skills isolated and restores them after restart', () async { @@ -781,3 +1016,44 @@ SettingsSnapshot _singleAgentTestSettings({required String workspacePath}) { workspacePath: workspacePath, ); } + +class _FakeSkillDirectoryAccessService implements SkillDirectoryAccessService { + _FakeSkillDirectoryAccessService({ + required this.userHomeDirectory, + this.requiresAuthorizedSharedRoots = false, + }); + + final String userHomeDirectory; + @override + final bool requiresAuthorizedSharedRoots; + + @override + bool get isSupported => true; + + @override + Future resolveUserHomeDirectory() async { + return userHomeDirectory; + } + + @override + Future authorizeDirectory({ + String suggestedPath = '', + }) async { + final normalized = normalizeAuthorizedSkillDirectoryPath(suggestedPath); + if (normalized.isEmpty) { + return null; + } + return AuthorizedSkillDirectory(path: normalized); + } + + @override + Future openDirectory( + AuthorizedSkillDirectory directory, + ) async { + final normalized = normalizeAuthorizedSkillDirectoryPath(directory.path); + if (normalized.isEmpty) { + return null; + } + return SkillDirectoryAccessHandle(path: normalized, onClose: () async {}); + } +}