Merge pull request #983 from web3dev1337/fix/windows-pty-bidirectional-compat

fix(windows): bidirectional pty compat for all conpty.node variants

Author: web3dev1337

ai-memory/fix-windows-pty-bidirectional-compat-36922fc/init.md

@@ -0,0 +1 @@
+Fix Windows PTY bidirectional argument compat so Commander works on ALL machines.

ai-memory/fix-windows-pty-bidirectional-compat-36922fc/progress.md

@@ -0,0 +1,7 @@
+- [x] Analyze the problem: runtime compat only handles too-many-args, not too-few-args
+- [x] Fix wrapConptyStartProcess: handle 6→7 (append false) in addition to 7→6
+- [x] Fix wrapConptyConnect: handle 5→6 (insert false before callback) in addition to 6→5
+- [x] Fix wrapConptyTrailingBooleanArg: parse expected arg count from usage error, handle both directions
+- [x] Add tests for all too-few-args scenarios
+- [x] All 545 tests pass
+- [ ] Commit, push, create PR

server/utils/nodePtyCompat.js

@@ -63,8 +63,13 @@ function wrapConptyStartProcess(nativeModule) {
     methodName: 'startProcess',
     patchFlag: START_PROCESS_PATCH_FLAG,
     retry: ({ args, error }) => {
-      if (args.length >= 7 && isStartProcessUsageError(error)) {
-        return args.slice(0, 6);
+      if (isStartProcessUsageError(error)) {
+        if (args.length >= 7) {
+          return args.slice(0, 6);
+        }
+        if (args.length === 6) {
+          return [...args, false];
+        }
       }
       return null;
     }
@@ -76,21 +81,43 @@ function wrapConptyConnect(nativeModule) {
     methodName: 'connect',
     patchFlag: CONNECT_PATCH_FLAG,
     retry: ({ args, error }) => {
-      if (args.length >= 6 && isConnectUsageError(error)) {
-        return [args[0], args[1], args[2], args[3], args[5]];
+      if (isConnectUsageError(error)) {
+        if (args.length >= 6) {
+          return [args[0], args[1], args[2], args[3], args[5]];
+        }
+        if (args.length === 5) {
+          return [args[0], args[1], args[2], args[3], false, args[4]];
+        }
       }
       return null;
     }
   });
 }
 
+function parseExpectedArgCount(error) {
+  const match = String(error?.message || '').match(/\(([^)]*)\)/);
+  if (!match) return 0;
+  return match[1].split(',').filter(Boolean).length;
+}
+
 function wrapConptyTrailingBooleanArg(nativeModule, methodName, patchFlag) {
   return wrapConptyMethod(nativeModule, {
     methodName,
     patchFlag,
     retry: ({ args, error }) => {
-      if (args.length >= 2 && isConptyUsageError(error, methodName)) {
-        return args.slice(0, -1);
+      if (isConptyUsageError(error, methodName)) {
+        const expected = parseExpectedArgCount(error);
+        if (expected > 0 && args.length > expected) {
+          return args.slice(0, expected);
+        }
+        if (expected > 0 && args.length < expected) {
+          const padded = [...args];
+          while (padded.length < expected) padded.push(false);
+          return padded;
+        }
+        if (args.length >= 2) {
+          return args.slice(0, -1);
+        }
       }
       return null;
     }

tests/unit/nodePtyCompat.test.js

@@ -64,6 +64,39 @@ describe('nodePtyCompat', () => {
     );
   });
 
+  test('wrapConptyStartProcess retries with an appended boolean when native expects 7 args but JS sends 6', () => {
+    const originalStartProcess = jest.fn((...args) => {
+      if (args.length < 7) {
+        throw new Error('Usage: pty.startProcess(file, cols, rows, debug, pipeName, inheritCursor, useConptyDll)');
+      }
+      return { pty: 789 };
+    });
+    const nativeModule = { startProcess: originalStartProcess };
+
+    expect(wrapConptyStartProcess(nativeModule)).toBe(true);
+
+    expect(nativeModule.startProcess('powershell.exe', 120, 40, false, 'pipe-1', true)).toEqual({ pty: 789 });
+    expect(originalStartProcess).toHaveBeenNthCalledWith(
+      1,
+      'powershell.exe',
+      120,
+      40,
+      false,
+      'pipe-1',
+      true
+    );
+    expect(originalStartProcess).toHaveBeenNthCalledWith(
+      2,
+      'powershell.exe',
+      120,
+      40,
+      false,
+      'pipe-1',
+      true,
+      false
+    );
+  });
+
   test('wrapConptyConnect retries without useConptyDll on the native usage error', () => {
     const exitCallback = jest.fn();
     const originalConnect = jest.fn((...args) => {
@@ -96,6 +129,38 @@ describe('nodePtyCompat', () => {
     );
   });
 
+  test('wrapConptyConnect retries with inserted useConptyDll when native expects 6 args but JS sends 5', () => {
+    const exitCallback = jest.fn();
+    const originalConnect = jest.fn((...args) => {
+      if (args.length < 6) {
+        throw new Error('Usage: pty.connect(id, cmdline, cwd, env, useConptyDll, exitCallback)');
+      }
+      return { pid: 555 };
+    });
+    const nativeModule = { connect: originalConnect };
+
+    expect(wrapConptyConnect(nativeModule)).toBe(true);
+
+    expect(nativeModule.connect(7, 'powershell.exe', 'C:\\repo', { PATH: 'x' }, exitCallback)).toEqual({ pid: 555 });
+    expect(originalConnect).toHaveBeenNthCalledWith(
+      1,
+      7,
+      'powershell.exe',
+      'C:\\repo',
+      { PATH: 'x' },
+      exitCallback
+    );
+    expect(originalConnect).toHaveBeenNthCalledWith(
+      2,
+      7,
+      'powershell.exe',
+      'C:\\repo',
+      { PATH: 'x' },
+      false,
+      exitCallback
+    );
+  });
+
   test('wrapConptyCompatMethods adapts trailing boolean compatibility calls', () => {
     const originalResize = jest.fn((...args) => {
       if (args.length === 4) {
@@ -140,6 +205,53 @@ describe('nodePtyCompat', () => {
     expect(originalKill).toHaveBeenCalledTimes(2);
   });
 
+  test('wrapConptyCompatMethods adapts trailing boolean calls when native expects MORE args', () => {
+    const originalResize = jest.fn((...args) => {
+      if (args.length < 4) {
+        throw new Error('Usage: pty.resize(id, cols, rows, useConptyDll)');
+      }
+      return undefined;
+    });
+    const originalClear = jest.fn((...args) => {
+      if (args.length < 2) {
+        throw new Error('Usage: pty.clear(id, useConptyDll)');
+      }
+      return undefined;
+    });
+    const originalKill = jest.fn((...args) => {
+      if (args.length < 2) {
+        throw new Error('Usage: pty.kill(id, useConptyDll)');
+      }
+      return undefined;
+    });
+    const nativeModule = {
+      startProcess: jest.fn(() => ({ pty: 1 })),
+      connect: jest.fn(() => ({ pid: 2 })),
+      resize: originalResize,
+      clear: originalClear,
+      kill: originalKill
+    };
+
+    expect(wrapConptyCompatMethods(nativeModule)).toEqual([
+      'startProcess',
+      'connect',
+      'resize',
+      'clear',
+      'kill'
+    ]);
+
+    nativeModule.resize(1, 120, 40);
+    nativeModule.clear(1);
+    nativeModule.kill(1);
+
+    expect(originalResize).toHaveBeenCalledTimes(2);
+    expect(originalResize).toHaveBeenNthCalledWith(2, 1, 120, 40, false);
+    expect(originalClear).toHaveBeenCalledTimes(2);
+    expect(originalClear).toHaveBeenNthCalledWith(2, 1, false);
+    expect(originalKill).toHaveBeenCalledTimes(2);
+    expect(originalKill).toHaveBeenNthCalledWith(2, 1, false);
+  });
+
   test('ensureWindowsNodePtyCompat wraps loadNativeModule for the affected Windows build', () => {
     const originalStartProcess = jest.fn(() => ({ pty: 789 }));
     const originalConnect = jest.fn(() => ({ pid: 456 }));