Skip to content

Commit 7c4ab0e

Browse files
web3dev1337web3dev1337
authored
Merge pull request #980 from web3dev1337/fix/windows-regression-triage
fix(windows): make node-pty compat shape-based
2 parents b62d492 + 1b19865 commit 7c4ab0e

3 files changed

Lines changed: 152 additions & 55 deletions

File tree

‎CODEBASE_DOCUMENTATION.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ server/utils/processUtils.js - Shared spawn/env hardening helpers
8484
├─ Windows packaging guardrails: applies `windowsHide`/`CREATE_NO_WINDOW`, augments GUI-app PATH with Git/node/npm/common CLI locations, and builds hidden PowerShell argument lists
8585
└─ Cross-platform behavior: non-Windows platforms pass through unchanged so Linux/macOS launch behavior stays stable
8686
server/utils/nodePtyCompat.js - Runtime compatibility shim for the bundled `node-pty` Windows ConPTY loader
87-
└─ Windows PTY guard: wraps the known-bad `node-pty@1.2.0-beta.12` ConPTY calls in memory (`startProcess`, `connect`, `resize`, `clear`, `kill`) so packaged installs survive read-only app-resource layouts and stale native bindings
87+
└─ Windows PTY guard: wraps stale ConPTY calls in memory (`startProcess`, `connect`, `resize`, `clear`, `kill`) via `loadNativeModule` when available or direct `conpty.node` patching when package internals differ, so packaged installs survive read-only app-resource layouts and mixed node-pty variants
8888
server/utils/pathUtils.js - Shared slash-normalization + data-directory compatibility helpers for repo/worktree labels
8989
└─ Legacy migration: renames `~/.orchestrator` when possible, otherwise merges richer legacy state into `~/.agent-workspace` with conflict backups before falling back to the old directory
9090
server/tokenCounter.js - Token usage tracking (if applicable)

‎server/utils/nodePtyCompat.js‎

Lines changed: 71 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,10 @@ const CONNECT_PATCH_FLAG = '__agentWorkspaceConptyConnectPatched';
55
const RESIZE_PATCH_FLAG = '__agentWorkspaceConptyResizePatched';
66
const CLEAR_PATCH_FLAG = '__agentWorkspaceConptyClearPatched';
77
const KILL_PATCH_FLAG = '__agentWorkspaceConptyKillPatched';
8+
const DIRECT_CONPTY_NATIVE_CANDIDATES = [
9+
'node-pty/build/Release/conpty.node',
10+
'node-pty/build/Debug/conpty.node'
11+
];
812

913
function isConptyUsageError(error, methodName) {
1014
const message = String(error?.message || '').trim();
@@ -115,6 +119,33 @@ function wrapConptyCompatMethods(nativeModule) {
115119
return patchedMethods;
116120
}
117121

122+
function patchConptyNativeModuleDirectly({
123+
requireModule = require
124+
} = {}) {
125+
let lastError = null;
126+
127+
for (const candidate of DIRECT_CONPTY_NATIVE_CANDIDATES) {
128+
try {
129+
const nativeModule = requireModule(candidate);
130+
const patchedMethods = wrapConptyCompatMethods(nativeModule);
131+
return {
132+
applied: patchedMethods.length > 0,
133+
reason: patchedMethods.length > 0 ? 'patched-direct-conpty-native' : 'already-patched',
134+
patchedMethods,
135+
candidate
136+
};
137+
} catch (error) {
138+
lastError = error;
139+
}
140+
}
141+
142+
return {
143+
applied: false,
144+
reason: 'missing-conpty-native-module',
145+
error: lastError
146+
};
147+
}
148+
118149
function ensureWindowsNodePtyCompat({
119150
platform = process.platform,
120151
utilsModule = null,
@@ -129,75 +160,59 @@ function ensureWindowsNodePtyCompat({
129160
if (!resolvedPackageInfo) {
130161
try {
131162
resolvedPackageInfo = requireModule('node-pty/package.json');
132-
} catch (error) {
133-
return {
134-
applied: false,
135-
reason: 'missing-package-info',
136-
error: error
137-
};
163+
} catch {
164+
resolvedPackageInfo = null;
138165
}
139166
}
140167

141-
const version = String(resolvedPackageInfo?.version || '').trim();
142-
if (version !== KNOWN_BROKEN_NODE_PTY_VERSION) {
143-
return {
144-
applied: false,
145-
reason: 'version-not-affected',
146-
version
147-
};
148-
}
168+
const version = String(resolvedPackageInfo?.version || '').trim() || null;
149169

150170
let utils = utilsModule;
151171
if (!utils) {
152172
try {
153173
utils = requireModule('node-pty/lib/utils');
154-
} catch (error) {
174+
} catch {
175+
utils = null;
176+
}
177+
}
178+
179+
if (utils && typeof utils.loadNativeModule === 'function') {
180+
if (utils.loadNativeModule[LOAD_NATIVE_MODULE_PATCH_FLAG]) {
155181
return {
156182
applied: false,
157-
reason: 'missing-utils-module',
158-
version,
159-
error
183+
reason: 'already-patched',
184+
version
160185
};
161186
}
162-
}
163187

164-
if (!utils || typeof utils.loadNativeModule !== 'function') {
165-
return {
166-
applied: false,
167-
reason: 'missing-utils-module',
168-
version
188+
const originalLoadNativeModule = utils.loadNativeModule.bind(utils);
189+
const wrappedLoadNativeModule = function loadNativeModuleCompat(name) {
190+
const result = originalLoadNativeModule(name);
191+
if (name === 'conpty' && result?.module) {
192+
const patchedMethods = wrapConptyCompatMethods(result.module);
193+
if (patchedMethods.length > 0) {
194+
result.compatPatchedMethods = patchedMethods;
195+
}
196+
}
197+
return result;
169198
};
170-
}
171199

172-
if (utils.loadNativeModule[LOAD_NATIVE_MODULE_PATCH_FLAG]) {
200+
Object.defineProperty(wrappedLoadNativeModule, LOAD_NATIVE_MODULE_PATCH_FLAG, {
201+
value: true,
202+
enumerable: false
203+
});
204+
205+
utils.loadNativeModule = wrappedLoadNativeModule;
173206
return {
174-
applied: false,
175-
reason: 'already-patched',
207+
applied: true,
208+
reason: 'patched-load-native-module',
176209
version
177210
};
178211
}
179212

180-
const originalLoadNativeModule = utils.loadNativeModule.bind(utils);
181-
const wrappedLoadNativeModule = function loadNativeModuleCompat(name) {
182-
const result = originalLoadNativeModule(name);
183-
if (name === 'conpty' && result?.module) {
184-
const patchedMethods = wrapConptyCompatMethods(result.module);
185-
if (patchedMethods.length > 0) {
186-
result.compatPatchedMethods = patchedMethods;
187-
}
188-
}
189-
return result;
190-
};
191-
192-
Object.defineProperty(wrappedLoadNativeModule, LOAD_NATIVE_MODULE_PATCH_FLAG, {
193-
value: true,
194-
enumerable: false
195-
});
196-
197-
utils.loadNativeModule = wrappedLoadNativeModule;
213+
const directPatch = patchConptyNativeModuleDirectly({ requireModule });
198214
return {
199-
applied: true,
200-
reason: 'patched-load-native-module',
215+
...directPatch,
201216
version
202217
};
203218
}
@@ -220,8 +235,14 @@ function loadNodePty({
220235
logger.info('Applied node-pty ConPTY runtime compatibility patch', {
221236
version: compat.version
222237
});
223-
} else if (platform === 'win32' && compat.reason === 'missing-utils-module' && logger?.warn) {
238+
} else if (
239+
platform === 'win32'
240+
&& !compat.applied
241+
&& !['already-patched'].includes(String(compat.reason || '').trim())
242+
&& logger?.warn
243+
) {
224244
logger.warn('Could not apply node-pty ConPTY runtime compatibility patch', {
245+
reason: compat.reason || null,
225246
version: compat.version || null
226247
});
227248
}
@@ -236,6 +257,7 @@ module.exports = {
236257
isConptyUsageError,
237258
isStartProcessUsageError,
238259
loadNodePty,
260+
patchConptyNativeModuleDirectly,
239261
wrapConptyCompatMethods,
240262
wrapConptyConnect,
241263
wrapConptyStartProcess

‎tests/unit/nodePtyCompat.test.js‎

Lines changed: 80 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -211,15 +211,78 @@ describe('nodePtyCompat', () => {
211211
});
212212
});
213213

214-
test('loadNodePty skips the patch when node-pty package metadata is unavailable', () => {
214+
test('ensureWindowsNodePtyCompat patches the direct conpty native module when loadNativeModule is unavailable', () => {
215+
const originalStartProcess = jest.fn((...args) => {
216+
if (args.length >= 7) {
217+
throw new Error('Usage: pty.startProcess(file, cols, rows, debug, pipeName, inheritCursor)');
218+
}
219+
return { pty: 654 };
220+
});
221+
const originalConnect = jest.fn((...args) => {
222+
if (args.length >= 6) {
223+
throw new Error('Usage: pty.connect(id, cmdline, cwd, env, exitCallback)');
224+
}
225+
return { pid: 987 };
226+
});
227+
const nativeModule = {
228+
startProcess: originalStartProcess,
229+
connect: originalConnect,
230+
resize: jest.fn(),
231+
clear: jest.fn(),
232+
kill: jest.fn()
233+
};
234+
const requireModule = jest.fn((specifier) => {
235+
if (specifier === 'node-pty/lib/utils') {
236+
throw new Error('missing utils');
237+
}
238+
if (specifier === 'node-pty/build/Release/conpty.node') {
239+
return nativeModule;
240+
}
241+
throw new Error(`unexpected module: ${specifier}`);
242+
});
243+
244+
const result = ensureWindowsNodePtyCompat({
245+
platform: 'win32',
246+
packageInfo: { version: '1.0.0' },
247+
requireModule
248+
});
249+
250+
expect(result).toMatchObject({
251+
applied: true,
252+
reason: 'patched-direct-conpty-native',
253+
version: '1.0.0',
254+
candidate: 'node-pty/build/Release/conpty.node',
255+
patchedMethods: ['startProcess', 'connect', 'resize', 'clear', 'kill']
256+
});
257+
258+
expect(nativeModule.startProcess('powershell.exe', 120, 40, false, 'pipe-2', true, 'unexpected-extra-flag')).toEqual({ pty: 654 });
259+
expect(nativeModule.connect(7, 'powershell.exe', 'C:\\repo', { PATH: 'x' }, true, jest.fn())).toEqual({ pid: 987 });
260+
expect(originalStartProcess).toHaveBeenCalledTimes(2);
261+
expect(originalConnect).toHaveBeenCalledTimes(2);
262+
});
263+
264+
test('loadNodePty applies the direct native patch when node-pty package metadata is unavailable', () => {
215265
const logger = {
216266
info: jest.fn(),
217267
warn: jest.fn()
218268
};
269+
const nativeModule = {
270+
startProcess: jest.fn(() => ({ pty: 111 })),
271+
connect: jest.fn(() => ({ pid: 222 })),
272+
resize: jest.fn(),
273+
clear: jest.fn(),
274+
kill: jest.fn()
275+
};
219276
const requireModule = jest.fn((specifier) => {
220277
if (specifier === 'node-pty/package.json') {
221278
throw new Error('missing package info');
222279
}
280+
if (specifier === 'node-pty/lib/utils') {
281+
throw new Error('missing utils');
282+
}
283+
if (specifier === 'node-pty/build/Release/conpty.node') {
284+
return nativeModule;
285+
}
223286
if (specifier === 'node-pty') {
224287
return { spawn: jest.fn() };
225288
}
@@ -234,12 +297,19 @@ describe('nodePtyCompat', () => {
234297

235298
expect(pty).toEqual({ spawn: expect.any(Function) });
236299
expect(requireModule).toHaveBeenCalledWith('node-pty/package.json');
300+
expect(requireModule).toHaveBeenCalledWith('node-pty/lib/utils');
301+
expect(requireModule).toHaveBeenCalledWith('node-pty/build/Release/conpty.node');
237302
expect(requireModule).toHaveBeenCalledWith('node-pty');
238-
expect(logger.info).not.toHaveBeenCalled();
303+
expect(logger.info).toHaveBeenCalledWith(
304+
'Applied node-pty ConPTY runtime compatibility patch',
305+
expect.objectContaining({
306+
version: null
307+
})
308+
);
239309
expect(logger.warn).not.toHaveBeenCalled();
240310
});
241311

242-
test('loadNodePty skips the patch for unaffected versions', () => {
312+
test('loadNodePty applies the patch even when node-pty reports an unexpected version string', () => {
243313
const logger = {
244314
info: jest.fn(),
245315
warn: jest.fn()
@@ -253,13 +323,18 @@ describe('nodePtyCompat', () => {
253323
platform: 'win32',
254324
logger,
255325
utilsModule,
256-
packageInfo: { version: '1.2.0' },
326+
packageInfo: { version: '1.0.0' },
257327
requireModule
258328
});
259329

260330
expect(pty).toEqual({ spawn: expect.any(Function) });
261331
expect(requireModule).toHaveBeenCalledWith('node-pty');
262-
expect(logger.info).not.toHaveBeenCalled();
332+
expect(logger.info).toHaveBeenCalledWith(
333+
'Applied node-pty ConPTY runtime compatibility patch',
334+
expect.objectContaining({
335+
version: '1.0.0'
336+
})
337+
);
263338
expect(logger.warn).not.toHaveBeenCalled();
264339
});
265340

0 commit comments

Comments
 (0)