[fix] Let open() propagate errors instead of swallowing them

src/classes/ManagerLocal.ts

@@ -628,38 +628,37 @@ export class ManagerLocal extends Manager {
       throw new Error(`Package ${slug} has no compatible file with open command defined`);
     }
 
-    try {
-      const openPath = (openableFile as any).open;
-      const fileExt: string = path.extname(openPath).slice(1).toLowerCase();
-      let packageDir: string;
-
-      if (this.type === RegistryType.Plugins) {
-        // For plugins, use type-specific subdirectories
-        const formatDir: string = pluginFormatDir[fileExt as PluginFormat] || 'Plugin';
-        packageDir = path.join(this.typeDir, formatDir, slug, versionNum);
-      } else {
-        // For apps/projects/presets, files are in direct package directory
-        packageDir = path.join(this.typeDir, slug, versionNum);
-      }
-      let fullPath: string;
-      if (path.isAbsolute(openPath)) {
-        fullPath = openPath;
-      } else if (fileExt === 'app') {
-        // For .app bundles, construct path to executable inside Contents/MacOS/
-        const appName = path.basename(openPath, '.app');
-        fullPath = path.join(packageDir, openPath, 'Contents', 'MacOS', appName);
-      } else {
-        fullPath = path.join(packageDir, openPath);
-      }
-      const command = `"${fullPath}" ${options.join(' ')}`;
-
-      this.log(`Running: ${command}`);
-      fileOpen(fullPath, options);
-      return true;
-    } catch (error) {
-      this.log(`Error opening package ${slug}:`, error);
-      return false;
+    // Let fileOpen()/path errors propagate rather than catching them here - every other
+    // mutating method on this class (install, uninstall, installDependency, ...) throws on
+    // failure, and swallowing errors into a `false` return would be the only exception to that,
+    // silently discarding the actual cause in the (default, debug logging disabled) case.
+    const openPath = (openableFile as any).open;
+    const fileExt: string = path.extname(openPath).slice(1).toLowerCase();
+    let packageDir: string;
+
+    if (this.type === RegistryType.Plugins) {
+      // For plugins, use type-specific subdirectories
+      const formatDir: string = pluginFormatDir[fileExt as PluginFormat] || 'Plugin';
+      packageDir = path.join(this.typeDir, formatDir, slug, versionNum);
+    } else {
+      // For apps/projects/presets, files are in direct package directory
+      packageDir = path.join(this.typeDir, slug, versionNum);
     }
+    let fullPath: string;
+    if (path.isAbsolute(openPath)) {
+      fullPath = openPath;
+    } else if (fileExt === 'app') {
+      // For .app bundles, construct path to executable inside Contents/MacOS/
+      const appName = path.basename(openPath, '.app');
+      fullPath = path.join(packageDir, openPath, 'Contents', 'MacOS', appName);
+    } else {
+      fullPath = path.join(packageDir, openPath);
+    }
+    const command = `"${fullPath}" ${options.join(' ')}`;
+
+    this.log(`Running: ${command}`);
+    fileOpen(fullPath, options);
+    return true;
   }
 
   async uninstall(slug: string, version?: string) {

src/helpers/file.ts

@@ -170,11 +170,17 @@ export function dirMove(dir: string, dirNew: string): void | boolean {
 
 export function dirOpen(dir: string) {
   if (process.env.CI) return Buffer.from('');
-  // execFileSync never invokes a shell, so `dir` can't break out into a second command
-  // regardless of its contents.
+  // execFileSync never invokes a shell itself, but on Windows the target of that call would be
+  // cmd.exe - which *is* a command interpreter, and re-parses its `/c` command line using cmd's
+  // own grammar (where `&`, `|`, `^`, etc are metacharacters) regardless of how Node quoted the
+  // argv it was given. explorer.exe has no such reinterpretation - it treats its argument as a
+  // literal path - so it's used instead of cmd.exe /c start. Its exit code is unreliable
+  // (frequently non-zero even on success), so this uses spawn() and doesn't wait on the result,
+  // same as the CI short-circuit above already implies callers don't depend on one.
   if (getSystem() === SystemType.Win) {
-    log('⎋', `cmd.exe /c start "" "${dir}"`);
-    return execFileSync('cmd.exe', ['/c', 'start', '""', dir]);
+    log('⎋', `explorer.exe "${dir}"`);
+    spawn('explorer.exe', [dir], { stdio: 'ignore' });
+    return;
   } else if (getSystem() === SystemType.Mac) {
     log('⎋', `open "${dir}"`);
     return execFileSync('open', [dir]);
@@ -455,9 +461,10 @@ export function filesMove(dirSource: string, dirTarget: string, dirSub: string,
   return filesMoved;
 }
 
-// filePath (and, for the Windows/Linux branches, the surrounding options) ultimately come from
-// a package's `open` field in registry metadata, so this is the same command-injection surface
-// as fileInstall - execFileSync (no shell) rather than execSync everywhere below.
+// filePath (and, for the Mac/Linux branches, the surrounding options) ultimately come from a
+// package's `open` field in registry metadata, so this is the same command-injection surface as
+// fileInstall - execFileSync (no shell) rather than execSync for those branches. The Windows
+// branch below needs a different fix: see its own comment.
 export function fileOpen(filePath: string, options: string[] = []) {
   if (process.env.CI) return Buffer.from('');
 
@@ -475,8 +482,17 @@ export function fileOpen(filePath: string, options: string[] = []) {
   }
 
   if (getSystem() === SystemType.Win) {
-    log('⎋', `cmd.exe /c start "" "${filePath}"`);
-    return execFileSync('cmd.exe', ['/c', 'start', '""', filePath]);
+    // execFileSync never invokes a shell itself, but on Windows the target of that call would
+    // be cmd.exe - which *is* a command interpreter, and re-parses its `/c` command line using
+    // cmd's own grammar (where `&`, `|`, `^`, etc are metacharacters) regardless of how Node
+    // quoted the argv it was given, so a filePath containing them (this is untrusted, coming
+    // from a package's `open` field) could still be reinterpreted. explorer.exe has no such
+    // reinterpretation - it treats its argument as a literal path - so it's used instead of
+    // cmd.exe /c start. Its exit code is unreliable (frequently non-zero even on success), so
+    // this uses spawn() and doesn't wait on/check the result.
+    log('⎋', `explorer.exe "${filePath}"`);
+    spawn('explorer.exe', [filePath], { stdio: 'ignore' });
+    return;
   }
   log('⎋', `xdg-open "${filePath}"`);
   return execFileSync('xdg-open', [filePath]);

tests/classes/ManagerLocal.test.ts

@@ -337,3 +337,46 @@ test('Clone throws for nonexistent template repo', async () => {
     manager.clone('template-org/template-plugin-3', 'open-audio-stack/this-repo-does-not-exist-xyz123'),
   ).rejects.toThrow('not found on GitHub');
 });
+
+test('Open throws for a package not found in the registry', () => {
+  const manager = new ManagerLocal(RegistryType.Plugins, CONFIG);
+  expect(() => manager.open('nonexistent-org/nonexistent-plugin')).toThrow('not found');
+});
+
+test('Open throws for a package version not found in the registry', async () => {
+  const manager = new ManagerLocal(RegistryType.Projects, CONFIG);
+  await manager.sync();
+  expect(() => manager.open(PROJECT_PACKAGE.slug, '99.99.99')).toThrow('version 99.99.99 not found');
+});
+
+test('Open throws for a package that is not installed', async () => {
+  const manager = new ManagerLocal(RegistryType.Projects, CONFIG);
+  await manager.sync();
+  // Earlier tests in this file install PROJECT_PACKAGE and don't always uninstall it
+  // afterward (they're only exercising dependency install/uninstall) - don't assume a fresh
+  // not-installed state, ensure it here regardless of file execution order.
+  if (manager.isPackageInstalled(PROJECT_PACKAGE.slug, PROJECT_PACKAGE.version)) {
+    await manager.uninstall(PROJECT_PACKAGE.slug, PROJECT_PACKAGE.version);
+  }
+  expect(() => manager.open(PROJECT_PACKAGE.slug, PROJECT_PACKAGE.version)).toThrow('not installed');
+});
+
+test('Open runs the compatible file and propagates errors instead of swallowing them', async () => {
+  const manager = new ManagerLocal(RegistryType.Projects, CONFIG);
+  await manager.sync();
+  await manager.install(PROJECT_PACKAGE.slug, PROJECT_PACKAGE.version);
+
+  const fileOpenSpy = vi.spyOn(fileHelpers, 'fileOpen').mockReturnValue(undefined as any);
+  expect(manager.open(PROJECT_PACKAGE.slug, PROJECT_PACKAGE.version)).toEqual(true);
+  expect(fileOpenSpy).toHaveBeenCalled();
+
+  // Regression check for the swallowed-error bug: open() must throw the underlying failure
+  // rather than catching it and returning false.
+  fileOpenSpy.mockImplementation(() => {
+    throw new Error('boom');
+  });
+  expect(() => manager.open(PROJECT_PACKAGE.slug, PROJECT_PACKAGE.version)).toThrow('boom');
+
+  fileOpenSpy.mockRestore();
+  await manager.uninstall(PROJECT_PACKAGE.slug, PROJECT_PACKAGE.version);
+});