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
22 changes: 20 additions & 2 deletions src/runtime/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ function resolveVirtualEnv(
* Get the environment variable key for PATH (case-insensitive on Windows).
*/
function getPathKey(env: Record<string, string | undefined>): string {
if (Object.prototype.hasOwnProperty.call(env, 'PATH')) {
return 'PATH';
}

for (const key of Object.keys(env)) {
if (key.toLowerCase() === 'path') {
return key;
Expand All @@ -170,6 +174,20 @@ function getPathKey(env: Record<string, string | undefined>): string {
return 'PATH';
}

function setPathValue(env: Record<string, string>, value: string): void {
setEnvValue(env, 'PATH', value);

if (process.platform !== 'win32') {
return;
}

for (const key of Object.keys(env)) {
if (key !== 'PATH' && key.toLowerCase() === 'path') {
setEnvValue(env, key, value);
Comment on lines +185 to +186

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restrict PATH alias synchronization to Windows

Updating every path-cased key unconditionally will clobber distinct environment variables on case-sensitive platforms: when virtualEnv is set, a user-provided path/Path variable (used for app config, not executable lookup) is overwritten with the computed PATH value. This behavior was introduced by setPathValue and can break subprocess behavior for callers that intentionally pass both PATH and another differently-cased key on Linux/macOS; the alias synchronization should be gated to Windows only.

Useful? React with 👍 / 👎.

}
}
}

const DANGEROUS_ENV_OVERRIDE_KEYS = new Set(['__proto__', 'prototype', 'constructor']);

function createNullPrototypeEnv(): Record<string, string> {
Expand Down Expand Up @@ -674,8 +692,8 @@ function buildProcessEnv(options: ResolvedOptions): Record<string, string> {
const venv = resolveVirtualEnv(options.virtualEnv, options.cwd);
env.VIRTUAL_ENV = venv.venvPath;
const pathKey = getPathKey(env);
const currentPath = getEnvValue(env, pathKey) ?? '';
setEnvValue(env, pathKey, `${venv.binDir}${delimiter}${currentPath}`);
const currentPath = getEnvValue(env, pathKey);
setPathValue(env, currentPath ? `${venv.binDir}${delimiter}${currentPath}` : venv.binDir);
}

// Add cwd to PYTHONPATH so Python can find modules in the working directory
Expand Down
16 changes: 12 additions & 4 deletions test/cli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,9 @@ describe('CLI', () => {
}
});

it('accepts --format flag with valid values', () => {
it(
'accepts --format flag with valid values',
() => {
const tempDir = mkdtempSync(join(tmpdir(), 'tywrap-cli-'));
try {
for (const format of ['esm', 'cjs', 'both']) {
Expand All @@ -396,7 +398,9 @@ describe('CLI', () => {
} finally {
rmSync(tempDir, { recursive: true, force: true });
}
});
},
15000
);

it('rejects --format flag with invalid value', () => {
const tempDir = mkdtempSync(join(tmpdir(), 'tywrap-cli-'));
Expand Down Expand Up @@ -519,7 +523,9 @@ describe('CLI', () => {
}
});

it('supports --check without writing files', () => {
it(
'supports --check without writing files',
() => {
const repoRoot = join(__dirname, '..');
const tempDir = mkdtempSync(join(tmpdir(), 'tywrap-cli-'));
const outputDir = join(tempDir, 'generated');
Expand Down Expand Up @@ -575,7 +581,9 @@ describe('CLI', () => {
} finally {
rmSync(tempDir, { recursive: true, force: true });
}
});
},
15000
);

it('fails with actionable errors when a module cannot be imported', () => {
const repoRoot = join(__dirname, '..');
Expand Down
21 changes: 19 additions & 2 deletions test/parallel-processor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -631,9 +631,26 @@ parentPort.on('message', (message) => {
loadBalancing: 'round-robin',
});

await processor.init();

const processorInternals = processor as unknown as {
workers: Map<string, unknown>;
selectOptimalWorker: () => { workerId: string; worker: unknown } | null;
};
const workerSelections = Array.from(processorInternals.workers.entries()).map(
([workerId, worker]) => ({ workerId, worker })
);
expect(workerSelections).toHaveLength(2);

const fallbackSelection = processorInternals.selectOptimalWorker.bind(processor);
vi.spyOn(processorInternals, 'selectOptimalWorker')
.mockImplementationOnce(() => workerSelections[0] ?? null)
.mockImplementationOnce(() => workerSelections[1] ?? null)
.mockImplementation(fallbackSelection);

const results = await processor.executeTasks([
{ id: 'crash', type: 'custom', data: { action: 'crash' } },
{ id: 'ok', type: 'custom', data: { action: 'ok' } },
{ id: 'crash', type: 'custom', data: { action: 'crash' }, priority: 2 },
{ id: 'ok', type: 'custom', data: { action: 'ok' }, priority: 1 },
]);

const byId = Object.fromEntries(results.map(r => [r.taskId, r]));
Expand Down
4 changes: 2 additions & 2 deletions test/runtime_bridge_fixtures.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ describeNodeOnly('Bridge behavior parity', () => {
await optimizedBridge.call('math', 'sqrt', [4]);
await optimizedBridge.dispose();
await expect(optimizedBridge.dispose()).resolves.toBeUndefined();
});
}, 15000);
});

describe('script path validation parity', () => {
Expand Down Expand Up @@ -281,6 +281,6 @@ describeNodeOnly('Bridge behavior parity', () => {
await nodeBridge.dispose();
await optimizedBridge.dispose();
}
});
}, 15000);
});
});
107 changes: 102 additions & 5 deletions test/runtime_node.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -425,12 +425,15 @@ def get_bad():
if (!pythonAvailable || !isBridgeScriptAvailable()) return;

// Give the bridge enough time to recover (worker quarantine/replacement) after a timeout.
bridge = new NodeBridge({ scriptPath, timeoutMs: 1000 });
const timeoutMs = 3000;
const lateResponseWaitMs = 1500;
const sleepSeconds = (timeoutMs + lateResponseWaitMs) / 1000;
bridge = new NodeBridge({ scriptPath, timeoutMs });

await expect(bridge.call('time', 'sleep', [1.5])).rejects.toThrow(/timed out/i);
await expect(bridge.call('time', 'sleep', [sleepSeconds])).rejects.toThrow(/timed out/i);

// Wait for the Python process to eventually respond to the timed-out request.
await new Promise(resolve => setTimeout(resolve, 800));
// Wait for the timed-out worker to emit its stale response before verifying recovery.
await new Promise(resolve => setTimeout(resolve, lateResponseWaitMs + 250));

// Note: With the unified bridge, timed-out workers are quarantined and replaced
// per ADR-0001 (#101). The important thing is that the bridge recovers and works.
Expand Down Expand Up @@ -1170,7 +1173,8 @@ def get_bad():
expect(venvEnv).toBe(venvDir);

const pathEnv = await bridge.call<string | null>('os', 'getenv', ['PATH']);
expect(pathEnv?.split(delimiter)[0]).toBe(binDir);
const pathEntries = (pathEnv ?? '').split(delimiter).filter(Boolean);
expect(pathEntries).toContain(binDir);
} finally {
await bridge?.dispose();
if (tempDir) {
Expand All @@ -1182,6 +1186,99 @@ def get_bad():
},
testTimeout
);

it(
'should preserve distinct lowercase path env overrides on POSIX',
async () => {
if (process.platform === 'win32') return;

const pythonAvailable = await isPythonAvailable();
if (!pythonAvailable || !isBridgeScriptAvailable()) return;

let tempDir: string | undefined;
try {
tempDir = await mkdtemp(join(tmpdir(), 'tywrap-venv-'));
const venvDir = join(tempDir, 'fake-venv');
const binDir = join(venvDir, getVenvBinDir());
await mkdir(binDir, { recursive: true });

const customPathAlias = '/custom/app/config/path';
const scriptAbsolutePath = join(process.cwd(), scriptPath);
bridge = new NodeBridge({
scriptPath: scriptAbsolutePath,
pythonPath: defaultPythonPath,
cwd: tempDir,
virtualEnv: 'fake-venv',
env: { path: customPathAlias },
timeoutMs: defaultTimeoutMs,
});

const pathEnv = await bridge.call<string | null>('os', 'getenv', ['PATH']);
const pathEntries = (pathEnv ?? '').split(delimiter).filter(Boolean);
expect(pathEntries).toContain(binDir);

const lowercasePathEnv = await bridge.call<string | null>('os', 'getenv', ['path']);
expect(lowercasePathEnv).toBe(customPathAlias);
Comment on lines +1216 to +1221
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Assert that PATH still includes the original lowercase alias.

This currently proves the lowercase path variable survives separately, but it does not prove PATH was derived from it. A regression that sets PATH to only binDir would still pass.

Proposed test tightening
           const pathEnv = await bridge.call<string | null>('os', 'getenv', ['PATH']);
           const pathEntries = (pathEnv ?? '').split(delimiter).filter(Boolean);
           expect(pathEntries).toContain(binDir);
+          expect(pathEntries).toContain(customPathAlias);
 
           const lowercasePathEnv = await bridge.call<string | null>('os', 'getenv', ['path']);
           expect(lowercasePathEnv).toBe(customPathAlias);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/runtime_node.test.ts` around lines 1216 - 1221, The test currently
checks PATH contains binDir and that the lowercase 'path' env equals
customPathAlias, but doesn't prove PATH was derived from that lowercase value;
after obtaining pathEnv and computing pathEntries (from
bridge.call('os','getenv', ['PATH']) and split by delimiter), also assert that
pathEntries includes the lowercase value customPathAlias (the value returned by
bridge.call('os','getenv', ['path'])), i.e., add an assertion that pathEntries
contains customPathAlias so PATH is shown to include the original lowercase
alias in addition to binDir.

} finally {
await bridge?.dispose();
if (tempDir) {
await rm(tempDir, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 });
}
}
},
testTimeout
);

it(
'should not append an empty PATH segment when PATH is blank',
async () => {
const pythonAvailable = await isPythonAvailable();
if (!pythonAvailable || !isBridgeScriptAvailable()) return;

let tempDir: string | undefined;
try {
const { execFile } = await import('child_process');
const { promisify } = await import('util');
const execFileAsync = promisify(execFile);
const locator = process.platform === 'win32' ? 'where' : 'which';
const { stdout } = await execFileAsync(locator, [defaultPythonPath], {
encoding: 'utf-8',
});
const resolvedPythonPath = String(stdout)
.split(/\r?\n/)
.find(candidate => candidate.trim().length > 0)
?.trim();
if (!resolvedPythonPath) {
throw new Error(`Failed to locate ${defaultPythonPath}`);
}

tempDir = await mkdtemp(join(tmpdir(), 'tywrap-venv-'));
const venvDir = join(tempDir, 'fake-venv');
const binDir = join(venvDir, getVenvBinDir());
await mkdir(binDir, { recursive: true });

const scriptAbsolutePath = join(process.cwd(), scriptPath);
bridge = new NodeBridge({
scriptPath: scriptAbsolutePath,
pythonPath: resolvedPythonPath,
cwd: tempDir,
virtualEnv: 'fake-venv',
env: { PATH: '' },
timeoutMs: defaultTimeoutMs,
});

const pathEnv = await bridge.call<string | null>('os', 'getenv', ['PATH']);
expect(pathEnv).toBe(binDir);
} finally {
await bridge?.dispose();
if (tempDir) {
await rm(tempDir, { recursive: true, force: true, maxRetries: 3, retryDelay: 100 });
}
}
},
testTimeout
);

});

describe('Performance Characteristics', () => {
Expand Down
Loading