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
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix Windows PTY bidirectional argument compat so Commander works on ALL machines.
Original file line number Diff line number Diff line change
@@ -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
39 changes: 33 additions & 6 deletions server/utils/nodePtyCompat.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down
112 changes: 112 additions & 0 deletions tests/unit/nodePtyCompat.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 }));
Expand Down
Loading