Skip to content

Commit eb35c5b

Browse files
web3dev1337claude
andcommitted
fix(windows): make pty compat bidirectional for startProcess/connect arg count
The runtime compat previously only handled the case where the JS wrapper sent MORE args than the native conpty.node expected (7→6 for startProcess, 6→5 for connect). On some Windows machines, the native binary expects MORE args than the JS sends (e.g. requires useConptyDll as 7th arg). This caused Commander and worktree terminals to fail on those machines. Now handles both directions: - startProcess: 7→6 (strip useConptyDll) AND 6→7 (append false) - connect: 6→5 (strip useConptyDll) AND 5→6 (insert false before callback) - resize/clear/kill: parse expected arg count from usage error, pad or strip Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 36922fc commit eb35c5b

File tree

4 files changed

+153
-6
lines changed

4 files changed

+153
-6
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix Windows PTY bidirectional argument compat so Commander works on ALL machines.
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
- [x] Analyze the problem: runtime compat only handles too-many-args, not too-few-args
2+
- [x] Fix wrapConptyStartProcess: handle 6→7 (append false) in addition to 7→6
3+
- [x] Fix wrapConptyConnect: handle 5→6 (insert false before callback) in addition to 6→5
4+
- [x] Fix wrapConptyTrailingBooleanArg: parse expected arg count from usage error, handle both directions
5+
- [x] Add tests for all too-few-args scenarios
6+
- [x] All 545 tests pass
7+
- [ ] Commit, push, create PR

server/utils/nodePtyCompat.js

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,8 +63,13 @@ function wrapConptyStartProcess(nativeModule) {
6363
methodName: 'startProcess',
6464
patchFlag: START_PROCESS_PATCH_FLAG,
6565
retry: ({ args, error }) => {
66-
if (args.length >= 7 && isStartProcessUsageError(error)) {
67-
return args.slice(0, 6);
66+
if (isStartProcessUsageError(error)) {
67+
if (args.length >= 7) {
68+
return args.slice(0, 6);
69+
}
70+
if (args.length === 6) {
71+
return [...args, false];
72+
}
6873
}
6974
return null;
7075
}
@@ -76,21 +81,43 @@ function wrapConptyConnect(nativeModule) {
7681
methodName: 'connect',
7782
patchFlag: CONNECT_PATCH_FLAG,
7883
retry: ({ args, error }) => {
79-
if (args.length >= 6 && isConnectUsageError(error)) {
80-
return [args[0], args[1], args[2], args[3], args[5]];
84+
if (isConnectUsageError(error)) {
85+
if (args.length >= 6) {
86+
return [args[0], args[1], args[2], args[3], args[5]];
87+
}
88+
if (args.length === 5) {
89+
return [args[0], args[1], args[2], args[3], false, args[4]];
90+
}
8191
}
8292
return null;
8393
}
8494
});
8595
}
8696

97+
function parseExpectedArgCount(error) {
98+
const match = String(error?.message || '').match(/\(([^)]*)\)/);
99+
if (!match) return 0;
100+
return match[1].split(',').filter(Boolean).length;
101+
}
102+
87103
function wrapConptyTrailingBooleanArg(nativeModule, methodName, patchFlag) {
88104
return wrapConptyMethod(nativeModule, {
89105
methodName,
90106
patchFlag,
91107
retry: ({ args, error }) => {
92-
if (args.length >= 2 && isConptyUsageError(error, methodName)) {
93-
return args.slice(0, -1);
108+
if (isConptyUsageError(error, methodName)) {
109+
const expected = parseExpectedArgCount(error);
110+
if (expected > 0 && args.length > expected) {
111+
return args.slice(0, expected);
112+
}
113+
if (expected > 0 && args.length < expected) {
114+
const padded = [...args];
115+
while (padded.length < expected) padded.push(false);
116+
return padded;
117+
}
118+
if (args.length >= 2) {
119+
return args.slice(0, -1);
120+
}
94121
}
95122
return null;
96123
}

tests/unit/nodePtyCompat.test.js

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,39 @@ describe('nodePtyCompat', () => {
6464
);
6565
});
6666

67+
test('wrapConptyStartProcess retries with an appended boolean when native expects 7 args but JS sends 6', () => {
68+
const originalStartProcess = jest.fn((...args) => {
69+
if (args.length < 7) {
70+
throw new Error('Usage: pty.startProcess(file, cols, rows, debug, pipeName, inheritCursor, useConptyDll)');
71+
}
72+
return { pty: 789 };
73+
});
74+
const nativeModule = { startProcess: originalStartProcess };
75+
76+
expect(wrapConptyStartProcess(nativeModule)).toBe(true);
77+
78+
expect(nativeModule.startProcess('powershell.exe', 120, 40, false, 'pipe-1', true)).toEqual({ pty: 789 });
79+
expect(originalStartProcess).toHaveBeenNthCalledWith(
80+
1,
81+
'powershell.exe',
82+
120,
83+
40,
84+
false,
85+
'pipe-1',
86+
true
87+
);
88+
expect(originalStartProcess).toHaveBeenNthCalledWith(
89+
2,
90+
'powershell.exe',
91+
120,
92+
40,
93+
false,
94+
'pipe-1',
95+
true,
96+
false
97+
);
98+
});
99+
67100
test('wrapConptyConnect retries without useConptyDll on the native usage error', () => {
68101
const exitCallback = jest.fn();
69102
const originalConnect = jest.fn((...args) => {
@@ -96,6 +129,38 @@ describe('nodePtyCompat', () => {
96129
);
97130
});
98131

132+
test('wrapConptyConnect retries with inserted useConptyDll when native expects 6 args but JS sends 5', () => {
133+
const exitCallback = jest.fn();
134+
const originalConnect = jest.fn((...args) => {
135+
if (args.length < 6) {
136+
throw new Error('Usage: pty.connect(id, cmdline, cwd, env, useConptyDll, exitCallback)');
137+
}
138+
return { pid: 555 };
139+
});
140+
const nativeModule = { connect: originalConnect };
141+
142+
expect(wrapConptyConnect(nativeModule)).toBe(true);
143+
144+
expect(nativeModule.connect(7, 'powershell.exe', 'C:\\repo', { PATH: 'x' }, exitCallback)).toEqual({ pid: 555 });
145+
expect(originalConnect).toHaveBeenNthCalledWith(
146+
1,
147+
7,
148+
'powershell.exe',
149+
'C:\\repo',
150+
{ PATH: 'x' },
151+
exitCallback
152+
);
153+
expect(originalConnect).toHaveBeenNthCalledWith(
154+
2,
155+
7,
156+
'powershell.exe',
157+
'C:\\repo',
158+
{ PATH: 'x' },
159+
false,
160+
exitCallback
161+
);
162+
});
163+
99164
test('wrapConptyCompatMethods adapts trailing boolean compatibility calls', () => {
100165
const originalResize = jest.fn((...args) => {
101166
if (args.length === 4) {
@@ -140,6 +205,53 @@ describe('nodePtyCompat', () => {
140205
expect(originalKill).toHaveBeenCalledTimes(2);
141206
});
142207

208+
test('wrapConptyCompatMethods adapts trailing boolean calls when native expects MORE args', () => {
209+
const originalResize = jest.fn((...args) => {
210+
if (args.length < 4) {
211+
throw new Error('Usage: pty.resize(id, cols, rows, useConptyDll)');
212+
}
213+
return undefined;
214+
});
215+
const originalClear = jest.fn((...args) => {
216+
if (args.length < 2) {
217+
throw new Error('Usage: pty.clear(id, useConptyDll)');
218+
}
219+
return undefined;
220+
});
221+
const originalKill = jest.fn((...args) => {
222+
if (args.length < 2) {
223+
throw new Error('Usage: pty.kill(id, useConptyDll)');
224+
}
225+
return undefined;
226+
});
227+
const nativeModule = {
228+
startProcess: jest.fn(() => ({ pty: 1 })),
229+
connect: jest.fn(() => ({ pid: 2 })),
230+
resize: originalResize,
231+
clear: originalClear,
232+
kill: originalKill
233+
};
234+
235+
expect(wrapConptyCompatMethods(nativeModule)).toEqual([
236+
'startProcess',
237+
'connect',
238+
'resize',
239+
'clear',
240+
'kill'
241+
]);
242+
243+
nativeModule.resize(1, 120, 40);
244+
nativeModule.clear(1);
245+
nativeModule.kill(1);
246+
247+
expect(originalResize).toHaveBeenCalledTimes(2);
248+
expect(originalResize).toHaveBeenNthCalledWith(2, 1, 120, 40, false);
249+
expect(originalClear).toHaveBeenCalledTimes(2);
250+
expect(originalClear).toHaveBeenNthCalledWith(2, 1, false);
251+
expect(originalKill).toHaveBeenCalledTimes(2);
252+
expect(originalKill).toHaveBeenNthCalledWith(2, 1, false);
253+
});
254+
143255
test('ensureWindowsNodePtyCompat wraps loadNativeModule for the affected Windows build', () => {
144256
const originalStartProcess = jest.fn(() => ({ pty: 789 }));
145257
const originalConnect = jest.fn(() => ({ pid: 456 }));

0 commit comments

Comments
 (0)