Skip to content

Commit 941f68c

Browse files
web3dev1337web3dev1337
authored
Merge pull request #975 from web3dev1337/fix/windows-pty-compat-main
fix(windows): make node-pty compat shim adaptive
2 parents b90a86b + 59e9399 commit 941f68c

4 files changed

Lines changed: 177 additions & 85 deletions

File tree

‎server/utils/nodePtyCompat.js‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,11 @@ const KNOWN_BROKEN_NODE_PTY_VERSION = '1.2.0-beta.12';
22
const LOAD_NATIVE_MODULE_PATCH_FLAG = '__agentWorkspaceConptyCompatPatched';
33
const START_PROCESS_PATCH_FLAG = '__agentWorkspaceConptyStartProcessPatched';
44

5+
function isStartProcessUsageError(error) {
6+
const message = String(error?.message || '').trim();
7+
return message.includes('Usage: pty.startProcess(');
8+
}
9+
510
function wrapConptyStartProcess(nativeModule) {
611
if (!nativeModule || typeof nativeModule.startProcess !== 'function') {
712
return false;
@@ -12,8 +17,15 @@ function wrapConptyStartProcess(nativeModule) {
1217
}
1318

1419
const originalStartProcess = nativeModule.startProcess.bind(nativeModule);
15-
const wrappedStartProcess = function startProcessCompat(file, cols, rows, debug, pipeName, inheritCursor) {
16-
return originalStartProcess(file, cols, rows, debug, pipeName, inheritCursor);
20+
const wrappedStartProcess = function startProcessCompat(...args) {
21+
try {
22+
return originalStartProcess(...args);
23+
} catch (error) {
24+
if (args.length >= 7 && isStartProcessUsageError(error)) {
25+
return originalStartProcess(...args.slice(0, 6));
26+
}
27+
throw error;
28+
}
1729
};
1830

1931
Object.defineProperty(wrappedStartProcess, START_PROCESS_PATCH_FLAG, {
@@ -139,6 +151,7 @@ function loadNodePty({
139151
module.exports = {
140152
KNOWN_BROKEN_NODE_PTY_VERSION,
141153
ensureWindowsNodePtyCompat,
154+
isStartProcessUsageError,
142155
loadNodePty,
143156
wrapConptyStartProcess
144157
};

‎tests/unit/commanderService.windowsShellArgs.test.js‎

Lines changed: 35 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,39 @@
11
const { EventEmitter } = require('events');
22

3-
jest.mock('winston', () => ({
4-
createLogger: jest.fn(() => ({
5-
info: jest.fn(),
6-
warn: jest.fn(),
7-
error: jest.fn(),
8-
debug: jest.fn()
9-
})),
10-
format: {
11-
combine: jest.fn(() => ({})),
12-
timestamp: jest.fn(() => ({})),
13-
json: jest.fn(() => ({})),
14-
simple: jest.fn(() => ({}))
15-
},
16-
transports: {
17-
File: jest.fn(),
18-
Console: jest.fn()
19-
}
20-
}), { virtual: true });
3+
function mockWinston() {
4+
return {
5+
createLogger: jest.fn(() => ({
6+
info: jest.fn(),
7+
warn: jest.fn(),
8+
error: jest.fn(),
9+
debug: jest.fn()
10+
})),
11+
format: {
12+
combine: jest.fn(() => ({})),
13+
timestamp: jest.fn(() => ({})),
14+
json: jest.fn(() => ({})),
15+
simple: jest.fn(() => ({}))
16+
},
17+
transports: {
18+
File: jest.fn(),
19+
Console: jest.fn()
20+
}
21+
};
22+
}
23+
24+
function loadCommanderServiceForWindows(spawn) {
25+
jest.resetModules();
26+
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' });
27+
jest.doMock('node-pty', () => ({ spawn }));
28+
jest.doMock('winston', () => mockWinston(), { virtual: true });
29+
30+
let CommanderService = null;
31+
jest.isolateModules(() => {
32+
({ CommanderService } = require('../../server/commanderService'));
33+
});
34+
CommanderService.instance = null;
35+
return CommanderService;
36+
}
2137

2238
describe('CommanderService Windows shell args', () => {
2339
const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform');
@@ -29,8 +45,6 @@ describe('CommanderService Windows shell args', () => {
2945
});
3046

3147
test('does not request a hidden PowerShell window for ConPTY terminals', async () => {
32-
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' });
33-
3448
const spawn = jest.fn(() => {
3549
const pty = new EventEmitter();
3650
pty.onData = jest.fn();
@@ -40,10 +54,8 @@ describe('CommanderService Windows shell args', () => {
4054
pty.kill = jest.fn();
4155
return pty;
4256
});
43-
jest.doMock('node-pty', () => ({ spawn }), { virtual: true });
4457

45-
const { CommanderService } = require('../../server/commanderService');
46-
CommanderService.instance = null;
58+
const CommanderService = loadCommanderServiceForWindows(spawn);
4759
const service = CommanderService.getInstance({ io: null, sessionManager: null });
4860

4961
const result = await service.start();

‎tests/unit/nodePtyCompat.test.js‎

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
const {
22
KNOWN_BROKEN_NODE_PTY_VERSION,
33
ensureWindowsNodePtyCompat,
4+
isStartProcessUsageError,
45
loadNodePty,
56
wrapConptyStartProcess
67
} = require('../../server/utils/nodePtyCompat');
78

89
describe('nodePtyCompat', () => {
9-
test('wrapConptyStartProcess truncates the incompatible seventh argument', () => {
10+
test('wrapConptyStartProcess preserves the seventh argument when the native binding accepts it', () => {
1011
const originalStartProcess = jest.fn(() => ({ pty: 123 }));
1112
const nativeModule = { startProcess: originalStartProcess };
1213

@@ -20,13 +21,47 @@ describe('nodePtyCompat', () => {
2021
40,
2122
false,
2223
'pipe-1',
23-
true
24+
true,
25+
'unexpected-extra-flag'
2426
);
2527
expect(wrapConptyStartProcess(nativeModule)).toBe(false);
2628
});
2729

30+
test('wrapConptyStartProcess retries without the seventh argument on the native usage error', () => {
31+
const originalStartProcess = jest.fn((...args) => {
32+
if (args.length >= 7) {
33+
throw new Error('Usage: pty.startProcess(file, cols, rows, debug, pipeName, inheritCursor)');
34+
}
35+
return { pty: 456 };
36+
});
37+
const nativeModule = { startProcess: originalStartProcess };
38+
39+
expect(wrapConptyStartProcess(nativeModule)).toBe(true);
40+
41+
expect(nativeModule.startProcess('powershell.exe', 120, 40, false, 'pipe-1', true, 'unexpected-extra-flag')).toEqual({ pty: 456 });
42+
expect(originalStartProcess).toHaveBeenNthCalledWith(
43+
1,
44+
'powershell.exe',
45+
120,
46+
40,
47+
false,
48+
'pipe-1',
49+
true,
50+
'unexpected-extra-flag'
51+
);
52+
expect(originalStartProcess).toHaveBeenNthCalledWith(
53+
2,
54+
'powershell.exe',
55+
120,
56+
40,
57+
false,
58+
'pipe-1',
59+
true
60+
);
61+
});
62+
2863
test('ensureWindowsNodePtyCompat wraps loadNativeModule for the affected Windows build', () => {
29-
const originalStartProcess = jest.fn(() => ({ pty: 456 }));
64+
const originalStartProcess = jest.fn(() => ({ pty: 789 }));
3065
const conptyModule = { startProcess: originalStartProcess };
3166
const originalLoadNativeModule = jest.fn((name) => ({
3267
dir: `mock/${name}`,
@@ -58,7 +93,8 @@ describe('nodePtyCompat', () => {
5893
40,
5994
false,
6095
'pipe-2',
61-
true
96+
true,
97+
'unexpected-extra-flag'
6298
);
6399

64100
expect(ensureWindowsNodePtyCompat({
@@ -123,4 +159,10 @@ describe('nodePtyCompat', () => {
123159
expect(logger.info).not.toHaveBeenCalled();
124160
expect(logger.warn).not.toHaveBeenCalled();
125161
});
162+
163+
test('isStartProcessUsageError only matches the native usage message', () => {
164+
expect(isStartProcessUsageError(new Error('Usage: pty.startProcess(file, cols, rows, debug, pipeName, inheritCursor)'))).toBe(true);
165+
expect(isStartProcessUsageError(new Error('other failure'))).toBe(false);
166+
expect(isStartProcessUsageError(null)).toBe(false);
167+
});
126168
});

‎tests/unit/sessionManager.windowsPty.test.js‎

Lines changed: 81 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,67 +1,93 @@
1-
jest.mock('node-pty', () => ({
2-
spawn: jest.fn(() => ({
3-
pid: 1234,
4-
onData: jest.fn(),
5-
onExit: jest.fn(),
6-
kill: jest.fn()
7-
}))
8-
}), { virtual: true });
9-
10-
jest.mock('winston', () => ({
11-
createLogger: jest.fn(() => ({
12-
info: jest.fn(),
13-
warn: jest.fn(),
14-
error: jest.fn(),
15-
debug: jest.fn()
16-
})),
17-
format: {
18-
combine: jest.fn(() => ({})),
19-
timestamp: jest.fn(() => ({})),
20-
json: jest.fn(() => ({})),
21-
simple: jest.fn(() => ({})),
22-
errors: jest.fn(() => ({}))
23-
},
24-
transports: {
25-
File: jest.fn(),
26-
Console: jest.fn()
27-
}
28-
}), { virtual: true });
29-
30-
jest.mock('../../server/sessionRecoveryService', () => ({
31-
clearSession: jest.fn(),
32-
updateSession: jest.fn(),
33-
updateAgent: jest.fn(),
34-
updateCwd: jest.fn(),
35-
updateConversation: jest.fn(),
36-
updateServer: jest.fn(),
37-
getSession: jest.fn(),
38-
getAllSessions: jest.fn(),
39-
init: jest.fn(),
40-
loadWorkspaceState: jest.fn(),
41-
getRecoveryInfo: jest.fn(),
42-
clearWorkspace: jest.fn(),
43-
markAgentInactive: jest.fn()
44-
}));
45-
46-
const nodePty = require('node-pty');
47-
const { SessionManager } = require('../../server/sessionManager');
481
const fs = require('fs');
492
const path = require('path');
503
const { patchNodePtyStartProcessCompat } = require('../../server/utils/processUtils');
514

52-
describe('SessionManager Windows PTY options', () => {
53-
const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform');
5+
const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform');
6+
7+
function createNodePtyMock() {
8+
return {
9+
spawn: jest.fn(() => ({
10+
pid: 1234,
11+
onData: jest.fn(),
12+
onExit: jest.fn(),
13+
kill: jest.fn()
14+
}))
15+
};
16+
}
17+
18+
function mockWinston() {
19+
return {
20+
createLogger: jest.fn(() => ({
21+
info: jest.fn(),
22+
warn: jest.fn(),
23+
error: jest.fn(),
24+
debug: jest.fn()
25+
})),
26+
format: {
27+
combine: jest.fn(() => ({})),
28+
timestamp: jest.fn(() => ({})),
29+
json: jest.fn(() => ({})),
30+
simple: jest.fn(() => ({})),
31+
errors: jest.fn(() => ({}))
32+
},
33+
transports: {
34+
File: jest.fn(),
35+
Console: jest.fn()
36+
}
37+
};
38+
}
39+
40+
function mockSessionRecoveryService() {
41+
return {
42+
clearSession: jest.fn(),
43+
updateSession: jest.fn(),
44+
updateAgent: jest.fn(),
45+
updateCwd: jest.fn(),
46+
updateConversation: jest.fn(),
47+
updateServer: jest.fn(),
48+
getSession: jest.fn(),
49+
getAllSessions: jest.fn(),
50+
init: jest.fn(),
51+
loadWorkspaceState: jest.fn(),
52+
getRecoveryInfo: jest.fn(),
53+
clearWorkspace: jest.fn(),
54+
markAgentInactive: jest.fn()
55+
};
56+
}
57+
58+
function loadSessionManagerForWindows() {
59+
jest.resetModules();
60+
Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' });
61+
62+
let nodePty = null;
63+
let SessionManager = null;
64+
65+
jest.doMock('node-pty', () => {
66+
nodePty = createNodePtyMock();
67+
return nodePty;
68+
});
69+
jest.doMock('winston', () => mockWinston(), { virtual: true });
70+
jest.doMock('../../server/sessionRecoveryService', () => mockSessionRecoveryService());
5471

55-
beforeEach(() => {
56-
jest.clearAllMocks();
72+
jest.isolateModules(() => {
73+
({ SessionManager } = require('../../server/sessionManager'));
5774
});
5875

76+
return {
77+
SessionManager,
78+
nodePty
79+
};
80+
}
81+
82+
describe('SessionManager Windows PTY options', () => {
5983
afterEach(() => {
84+
jest.resetModules();
85+
jest.clearAllMocks();
6086
Object.defineProperty(process, 'platform', platformDescriptor);
6187
});
6288

6389
it('enables ConPTY for Windows sessions and cleans up workspace-specific sessions', () => {
64-
Object.defineProperty(process, 'platform', { value: 'win32' });
90+
const { SessionManager, nodePty } = loadSessionManagerForWindows();
6591

6692
const io = { emit: jest.fn() };
6793
const sessionManager = new SessionManager(io, null);
@@ -82,7 +108,6 @@ describe('SessionManager Windows PTY options', () => {
82108
cwd: 'C:\\repo'
83109
})
84110
);
85-
// useConpty must NOT be passed — it causes native arg-count mismatches
86111
const passedOpts = nodePty.spawn.mock.calls[0][2];
87112
expect(passedOpts).not.toHaveProperty('useConpty');
88113

@@ -105,9 +130,9 @@ describe('SessionManager Windows PTY options', () => {
105130
});
106131

107132
it('patches the broken Windows node-pty startProcess wrapper', () => {
108-
const existsSpy = jest.spyOn(fs, 'existsSync').mockImplementation((targetPath) => {
109-
return targetPath === path.join('C:\\node_modules\\node-pty', 'lib', 'windowsPtyAgent.js');
110-
});
133+
const existsSpy = jest.spyOn(fs, 'existsSync').mockImplementation((targetPath) => (
134+
targetPath === path.join('C:\\node_modules\\node-pty', 'lib', 'windowsPtyAgent.js')
135+
));
111136
const readSpy = jest.spyOn(fs, 'readFileSync').mockReturnValue(
112137
'before conptyNative.startProcess(file, cols, rows, debug, this._generatePipeName(), conptyInheritCursor, this._useConptyDll) after'
113138
);

0 commit comments

Comments
 (0)