fix(opencode): address code review feedback — extract paths from error message, fix test imports
- Extract symlink source/target paths directly from the error message instead of reconstructing them from process.env (Codex P2 review) - Add extractSymlinkSourcePath and extractSymlinkTargetPath functions - Update ensureOpenCodeProfileNodeModulesJunction to accept optional errorMessage parameter and use extracted paths from it - Fix unused imports in test (remove 'os', replace 'beforeEach' with 'afterEach' per CodeRabbit review) - Widen fs.statSync mock signatures to use Parameters<typeof fs.statSync> per CodeRabbit review - Add tests for new extraction functions - Pass errorMessage to ensureOpenCodeProfileNodeModulesJunction calls in CLI client tests
This commit is contained in:
parent
597c690dbc
commit
cc3c9f7dc7
4 changed files with 111 additions and 34 deletions
|
|
@ -1092,7 +1092,7 @@ export class AgentTeamsRuntimeProviderManagementCliClient implements RuntimeProv
|
|||
if (process.platform === 'win32' && isOpenCodeNodeModulesSymlinkError(failure.message)) {
|
||||
const profileId = extractProfileIdFromSymlinkError(failure.message);
|
||||
if (profileId) {
|
||||
ensureOpenCodeProfileNodeModulesJunction(profileId);
|
||||
ensureOpenCodeProfileNodeModulesJunction(profileId, failure.message);
|
||||
try {
|
||||
const retryResult = await execCli(
|
||||
binaryPath,
|
||||
|
|
@ -1170,7 +1170,7 @@ export class AgentTeamsRuntimeProviderManagementCliClient implements RuntimeProv
|
|||
if (process.platform === 'win32' && isOpenCodeNodeModulesSymlinkError(failure.message)) {
|
||||
const profileId = extractProfileIdFromSymlinkError(failure.message);
|
||||
if (profileId) {
|
||||
ensureOpenCodeProfileNodeModulesJunction(profileId);
|
||||
ensureOpenCodeProfileNodeModulesJunction(profileId, failure.message);
|
||||
try {
|
||||
const retryResult = await execCli(
|
||||
binaryPath,
|
||||
|
|
|
|||
|
|
@ -54,13 +54,45 @@ export function extractProfileIdFromSymlinkError(message: string): string | null
|
|||
return match ? match[1] : null;
|
||||
}
|
||||
|
||||
export function ensureOpenCodeProfileNodeModulesJunction(profileId: string): boolean {
|
||||
const SYMLINK_SOURCE_PATTERN = /symlink\s+'([^']+)'/i;
|
||||
const SYMLINK_TARGET_PATTERN = /->\s+'([^']+)'/i;
|
||||
|
||||
export function extractSymlinkSourcePath(message: string): string | null {
|
||||
const match = SYMLINK_SOURCE_PATTERN.exec(message);
|
||||
return match ? match[1] : null;
|
||||
}
|
||||
|
||||
export function extractSymlinkTargetPath(message: string): string | null {
|
||||
const match = SYMLINK_TARGET_PATTERN.exec(message);
|
||||
return match ? match[1] : null;
|
||||
}
|
||||
|
||||
export function ensureOpenCodeProfileNodeModulesJunction(
|
||||
profileId: string,
|
||||
errorMessage?: string
|
||||
): boolean {
|
||||
if (process.platform !== 'win32') {
|
||||
return false;
|
||||
}
|
||||
|
||||
const source = getSharedCacheNodeModulesPath();
|
||||
const target = getProfileNodeModulesPath(profileId);
|
||||
let source: string;
|
||||
let target: string;
|
||||
|
||||
if (errorMessage) {
|
||||
const extractedSource = extractSymlinkSourcePath(errorMessage);
|
||||
const extractedTarget = extractSymlinkTargetPath(errorMessage);
|
||||
|
||||
if (extractedTarget) {
|
||||
target = extractedTarget;
|
||||
source = extractedSource ?? getSharedCacheNodeModulesPath();
|
||||
} else {
|
||||
target = getProfileNodeModulesPath(profileId);
|
||||
source = getSharedCacheNodeModulesPath();
|
||||
}
|
||||
} else {
|
||||
target = getProfileNodeModulesPath(profileId);
|
||||
source = getSharedCacheNodeModulesPath();
|
||||
}
|
||||
|
||||
try {
|
||||
const existingStat = fs.statSync(target, { throwIfNoEntry: false });
|
||||
|
|
|
|||
|
|
@ -958,7 +958,7 @@ describe('AgentTeamsRuntimeProviderManagementCliClient', () => {
|
|||
const client = new AgentTeamsRuntimeProviderManagementCliClient();
|
||||
const response = await client.loadView({ runtimeId: 'opencode' });
|
||||
|
||||
expect(ensureOpenCodeProfileNodeModulesJunctionMock).toHaveBeenCalledWith('abc123');
|
||||
expect(ensureOpenCodeProfileNodeModulesJunctionMock).toHaveBeenCalledWith('abc123', runtimeMessage);
|
||||
expect(execCliMock).toHaveBeenCalledTimes(2);
|
||||
expect(response.error).toBeUndefined();
|
||||
expect(response.view?.runtime?.state).toBe('ready');
|
||||
|
|
@ -998,7 +998,7 @@ describe('AgentTeamsRuntimeProviderManagementCliClient', () => {
|
|||
const client = new AgentTeamsRuntimeProviderManagementCliClient();
|
||||
const response = await client.loadView({ runtimeId: 'opencode' });
|
||||
|
||||
expect(ensureOpenCodeProfileNodeModulesJunctionMock).toHaveBeenCalledWith('abc123');
|
||||
expect(ensureOpenCodeProfileNodeModulesJunctionMock).toHaveBeenCalledWith('abc123', runtimeMessage);
|
||||
expect(execCliMock).toHaveBeenCalledTimes(2);
|
||||
expect(response.error?.message).toBe(runtimeMessage);
|
||||
} finally {
|
||||
|
|
@ -1089,7 +1089,7 @@ describe('AgentTeamsRuntimeProviderManagementCliClient', () => {
|
|||
const client = new AgentTeamsRuntimeProviderManagementCliClient();
|
||||
const response = await client.loadProviderDirectory({ runtimeId: 'opencode' });
|
||||
|
||||
expect(ensureOpenCodeProfileNodeModulesJunctionMock).toHaveBeenCalledWith('def456');
|
||||
expect(ensureOpenCodeProfileNodeModulesJunctionMock).toHaveBeenCalledWith('def456', runtimeMessage);
|
||||
expect(execCliMock).toHaveBeenCalledTimes(2);
|
||||
expect(response.directory?.entries).toEqual([]);
|
||||
} finally {
|
||||
|
|
|
|||
|
|
@ -1,12 +1,13 @@
|
|||
import fs from 'node:fs';
|
||||
import os from 'node:os';
|
||||
import path from 'node:path';
|
||||
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest';
|
||||
|
||||
import {
|
||||
ensureOpenCodeProfileNodeModulesJunction,
|
||||
extractProfileIdFromSymlinkError,
|
||||
extractSymlinkSourcePath,
|
||||
extractSymlinkTargetPath,
|
||||
getProfileNodeModulesPath,
|
||||
getSharedCacheNodeModulesPath,
|
||||
isOpenCodeNodeModulesSymlinkError,
|
||||
|
|
@ -75,6 +76,44 @@ describe('openCodeWindowsNodeModulesJunction', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('extractSymlinkSourcePath', () => {
|
||||
it('extracts the source path from a Windows error message', () => {
|
||||
const message =
|
||||
"EPERM: operation not permitted, symlink 'C:\\Users\\Swarog\\AppData\\Local\\claude-multimodel-nodejs\\Cache\\opencode\\shared-cache\\config-node_modules' -> 'C:\\Users\\Swarog\\AppData\\Local\\claude-multimodel-nodejs\\Data\\opencode\\profiles\\e8e2eadb00beea6c\\config\\opencode\\node_modules'";
|
||||
expect(extractSymlinkSourcePath(message)).toBe(
|
||||
'C:\\Users\\Swarog\\AppData\\Local\\claude-multimodel-nodejs\\Cache\\opencode\\shared-cache\\config-node_modules'
|
||||
);
|
||||
});
|
||||
|
||||
it('extracts the source path from a single-quoted error', () => {
|
||||
const message =
|
||||
"EPERM: operation not permitted, symlink '/home/user/.cache/opencode/shared-cache/config-node_modules' -> '/home/user/.data/opencode/profiles/abc123/config/opencode/node_modules'";
|
||||
expect(extractSymlinkSourcePath(message)).toBe(
|
||||
'/home/user/.cache/opencode/shared-cache/config-node_modules'
|
||||
);
|
||||
});
|
||||
|
||||
it('returns null when no source path is found', () => {
|
||||
const message = 'EPERM: some error without paths';
|
||||
expect(extractSymlinkSourcePath(message)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('extractSymlinkTargetPath', () => {
|
||||
it('extracts the target path from a Windows error message', () => {
|
||||
const message =
|
||||
"EPERM: operation not permitted, symlink 'C:\\Users\\Swarog\\AppData\\Local\\claude-multimodel-nodejs\\Cache\\opencode\\shared-cache\\config-node_modules' -> 'C:\\Users\\Swarog\\AppData\\Local\\claude-multimodel-nodejs\\Data\\opencode\\profiles\\e8e2eadb00beea6c\\config\\opencode\\node_modules'";
|
||||
expect(extractSymlinkTargetPath(message)).toBe(
|
||||
'C:\\Users\\Swarog\\AppData\\Local\\claude-multimodel-nodejs\\Data\\opencode\\profiles\\e8e2eadb00beea6c\\config\\opencode\\node_modules'
|
||||
);
|
||||
});
|
||||
|
||||
it('returns null when no target path is found', () => {
|
||||
const message = "EPERM: operation not permitted, symlink '/some/path'";
|
||||
expect(extractSymlinkTargetPath(message)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('getSharedCacheNodeModulesPath', () => {
|
||||
it('uses LOCALAPPDATA environment variable when set', () => {
|
||||
const originalEnv = process.env.LOCALAPPDATA;
|
||||
|
|
@ -143,9 +182,11 @@ describe('openCodeWindowsNodeModulesJunction', () => {
|
|||
|
||||
it('returns false on Windows when shared cache does not exist', () => {
|
||||
Object.defineProperty(process, 'platform', { value: 'win32' });
|
||||
const statSyncSpy = vi.spyOn(fs, 'statSync').mockImplementation(() => {
|
||||
throw new Error('ENOENT');
|
||||
});
|
||||
const statSyncSpy = vi.spyOn(fs, 'statSync').mockImplementation(
|
||||
(..._args: Parameters<typeof fs.statSync>) => {
|
||||
throw new Error('ENOENT');
|
||||
}
|
||||
);
|
||||
const result = ensureOpenCodeProfileNodeModulesJunction('abc123');
|
||||
expect(result).toBe(false);
|
||||
statSyncSpy.mockRestore();
|
||||
|
|
@ -153,9 +194,11 @@ describe('openCodeWindowsNodeModulesJunction', () => {
|
|||
|
||||
it('returns true on Windows when target node_modules already exists', () => {
|
||||
Object.defineProperty(process, 'platform', { value: 'win32' });
|
||||
const statSyncSpy = vi.spyOn(fs, 'statSync').mockImplementation(() => {
|
||||
return {} as fs.Stats;
|
||||
});
|
||||
const statSyncSpy = vi.spyOn(fs, 'statSync').mockImplementation(
|
||||
(..._args: Parameters<typeof fs.statSync>) => {
|
||||
return {} as fs.Stats;
|
||||
}
|
||||
);
|
||||
const result = ensureOpenCodeProfileNodeModulesJunction('abc123');
|
||||
expect(result).toBe(true);
|
||||
statSyncSpy.mockRestore();
|
||||
|
|
@ -164,17 +207,17 @@ describe('openCodeWindowsNodeModulesJunction', () => {
|
|||
it('creates junction on Windows when shared cache exists and target is missing', () => {
|
||||
Object.defineProperty(process, 'platform', { value: 'win32' });
|
||||
let callCount = 0;
|
||||
const statSyncSpy = vi.spyOn(fs, 'statSync').mockImplementation(() => {
|
||||
callCount++;
|
||||
// First call: target does not exist (throw)
|
||||
// Second call: source exists (return stats)
|
||||
if (callCount === 1) {
|
||||
const err = new Error('ENOENT') as NodeJS.ErrnoException;
|
||||
err.code = 'ENOENT';
|
||||
throw err;
|
||||
const statSyncSpy = vi.spyOn(fs, 'statSync').mockImplementation(
|
||||
(..._args: Parameters<typeof fs.statSync>) => {
|
||||
callCount++;
|
||||
if (callCount === 1) {
|
||||
const err = new Error('ENOENT') as NodeJS.ErrnoException;
|
||||
err.code = 'ENOENT';
|
||||
throw err;
|
||||
}
|
||||
return {} as fs.Stats;
|
||||
}
|
||||
return {} as fs.Stats;
|
||||
});
|
||||
);
|
||||
const mkdirSyncSpy = vi.spyOn(fs, 'mkdirSync').mockImplementation(() => '');
|
||||
const symlinkSyncSpy = vi.spyOn(fs, 'symlinkSync').mockImplementation(() => undefined);
|
||||
const result = ensureOpenCodeProfileNodeModulesJunction('abc123');
|
||||
|
|
@ -189,15 +232,17 @@ describe('openCodeWindowsNodeModulesJunction', () => {
|
|||
it('returns false when junction creation fails', () => {
|
||||
Object.defineProperty(process, 'platform', { value: 'win32' });
|
||||
let callCount2 = 0;
|
||||
const statSyncSpy = vi.spyOn(fs, 'statSync').mockImplementation(() => {
|
||||
callCount2++;
|
||||
if (callCount2 === 1) {
|
||||
const err = new Error('ENOENT') as NodeJS.ErrnoException;
|
||||
err.code = 'ENOENT';
|
||||
throw err;
|
||||
const statSyncSpy = vi.spyOn(fs, 'statSync').mockImplementation(
|
||||
(..._args: Parameters<typeof fs.statSync>) => {
|
||||
callCount2++;
|
||||
if (callCount2 === 1) {
|
||||
const err = new Error('ENOENT') as NodeJS.ErrnoException;
|
||||
err.code = 'ENOENT';
|
||||
throw err;
|
||||
}
|
||||
return {} as fs.Stats;
|
||||
}
|
||||
return {} as fs.Stats;
|
||||
});
|
||||
);
|
||||
const mkdirSyncSpy = vi.spyOn(fs, 'mkdirSync').mockImplementation(() => '');
|
||||
const symlinkSyncSpy = vi.spyOn(fs, 'symlinkSync').mockImplementation(() => {
|
||||
throw new Error('EPERM');
|
||||
|
|
|
|||
Loading…
Reference in a new issue