fix: bound doctor output buffer and normalize credential values (#497)
This commit is contained in:
committed by
GitHub
Unverified
parent
1dbe4a8466
commit
89bda3c7af
@@ -49,6 +49,10 @@ async function fileExists(p: string): Promise<boolean> {
|
|||||||
try { await access(p, constants.F_OK); return true; } catch { return false; }
|
try { await access(p, constants.F_OK); return true; } catch { return false; }
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function normalizeCredentialValue(value: string): string {
|
||||||
|
return value.trim();
|
||||||
|
}
|
||||||
|
|
||||||
// ── Types ────────────────────────────────────────────────────────
|
// ── Types ────────────────────────────────────────────────────────
|
||||||
|
|
||||||
export interface ChannelConfigData {
|
export interface ChannelConfigData {
|
||||||
@@ -350,7 +354,16 @@ function assertNoDuplicateCredential(
|
|||||||
if (!uniqueKey) return;
|
if (!uniqueKey) return;
|
||||||
|
|
||||||
const incomingValue = config[uniqueKey];
|
const incomingValue = config[uniqueKey];
|
||||||
if (!incomingValue || typeof incomingValue !== 'string') return;
|
if (typeof incomingValue !== 'string') return;
|
||||||
|
const normalizedIncomingValue = normalizeCredentialValue(incomingValue);
|
||||||
|
if (!normalizedIncomingValue) return;
|
||||||
|
if (normalizedIncomingValue !== incomingValue) {
|
||||||
|
logger.warn('Normalized channel credential value for duplicate check', {
|
||||||
|
channelType,
|
||||||
|
accountId: resolvedAccountId,
|
||||||
|
key: uniqueKey,
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
const accounts = channelSection.accounts as Record<string, ChannelConfigData> | undefined;
|
const accounts = channelSection.accounts as Record<string, ChannelConfigData> | undefined;
|
||||||
if (!accounts) return;
|
if (!accounts) return;
|
||||||
@@ -359,9 +372,12 @@ function assertNoDuplicateCredential(
|
|||||||
if (existingAccountId === resolvedAccountId) continue;
|
if (existingAccountId === resolvedAccountId) continue;
|
||||||
if (!accountCfg || typeof accountCfg !== 'object') continue;
|
if (!accountCfg || typeof accountCfg !== 'object') continue;
|
||||||
const existingValue = accountCfg[uniqueKey];
|
const existingValue = accountCfg[uniqueKey];
|
||||||
if (typeof existingValue === 'string' && existingValue === incomingValue) {
|
if (
|
||||||
|
typeof existingValue === 'string'
|
||||||
|
&& normalizeCredentialValue(existingValue) === normalizedIncomingValue
|
||||||
|
) {
|
||||||
throw new Error(
|
throw new Error(
|
||||||
`The ${channelType} bot (${uniqueKey}: ${incomingValue}) is already bound to another agent (account: ${existingAccountId}). ` +
|
`The ${channelType} bot (${uniqueKey}: ${normalizedIncomingValue}) is already bound to another agent (account: ${existingAccountId}). ` +
|
||||||
`Each agent must use a unique bot.`,
|
`Each agent must use a unique bot.`,
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
@@ -416,6 +432,19 @@ export async function saveChannelConfig(
|
|||||||
|
|
||||||
const existingAccountConfig = resolveAccountConfig(channelSection, resolvedAccountId);
|
const existingAccountConfig = resolveAccountConfig(channelSection, resolvedAccountId);
|
||||||
const transformedConfig = transformChannelConfig(channelType, config, existingAccountConfig);
|
const transformedConfig = transformChannelConfig(channelType, config, existingAccountConfig);
|
||||||
|
const uniqueKey = CHANNEL_UNIQUE_CREDENTIAL_KEY[channelType];
|
||||||
|
if (uniqueKey && typeof transformedConfig[uniqueKey] === 'string') {
|
||||||
|
const rawCredentialValue = transformedConfig[uniqueKey] as string;
|
||||||
|
const normalizedCredentialValue = normalizeCredentialValue(rawCredentialValue);
|
||||||
|
if (normalizedCredentialValue !== rawCredentialValue) {
|
||||||
|
logger.warn('Normalizing channel credential value before save', {
|
||||||
|
channelType,
|
||||||
|
accountId: resolvedAccountId,
|
||||||
|
key: uniqueKey,
|
||||||
|
});
|
||||||
|
transformedConfig[uniqueKey] = normalizedCredentialValue;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Write credentials into accounts.<accountId>
|
// Write credentials into accounts.<accountId>
|
||||||
if (!channelSection.accounts || typeof channelSection.accounts !== 'object') {
|
if (!channelSection.accounts || typeof channelSection.accounts !== 'object') {
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ import { logger } from './logger';
|
|||||||
import { getUvMirrorEnv } from './uv-env';
|
import { getUvMirrorEnv } from './uv-env';
|
||||||
|
|
||||||
const OPENCLAW_DOCTOR_TIMEOUT_MS = 60_000;
|
const OPENCLAW_DOCTOR_TIMEOUT_MS = 60_000;
|
||||||
|
const MAX_DOCTOR_OUTPUT_BYTES = 10 * 1024 * 1024;
|
||||||
const OPENCLAW_DOCTOR_ARGS = ['doctor', '--json'];
|
const OPENCLAW_DOCTOR_ARGS = ['doctor', '--json'];
|
||||||
const OPENCLAW_DOCTOR_FIX_ARGS = ['doctor', '--fix', '--yes', '--non-interactive'];
|
const OPENCLAW_DOCTOR_FIX_ARGS = ['doctor', '--fix', '--yes', '--non-interactive'];
|
||||||
|
|
||||||
@@ -24,6 +25,39 @@ export interface OpenClawDoctorResult {
|
|||||||
error?: string;
|
error?: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function appendDoctorOutput(
|
||||||
|
current: string,
|
||||||
|
currentBytes: number,
|
||||||
|
data: Buffer | string,
|
||||||
|
stream: 'stdout' | 'stderr',
|
||||||
|
alreadyTruncated: boolean,
|
||||||
|
): { output: string; bytes: number; truncated: boolean } {
|
||||||
|
if (alreadyTruncated) {
|
||||||
|
return { output: current, bytes: currentBytes, truncated: true };
|
||||||
|
}
|
||||||
|
|
||||||
|
const chunk = typeof data === 'string' ? Buffer.from(data) : data;
|
||||||
|
if (currentBytes + chunk.length <= MAX_DOCTOR_OUTPUT_BYTES) {
|
||||||
|
return {
|
||||||
|
output: current + chunk.toString(),
|
||||||
|
bytes: currentBytes + chunk.length,
|
||||||
|
truncated: false,
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
const remaining = Math.max(0, MAX_DOCTOR_OUTPUT_BYTES - currentBytes);
|
||||||
|
const appended = remaining > 0 ? chunk.subarray(0, remaining).toString() : '';
|
||||||
|
logger.warn(
|
||||||
|
`OpenClaw doctor ${stream} exceeded ${MAX_DOCTOR_OUTPUT_BYTES} bytes; truncating additional output`,
|
||||||
|
);
|
||||||
|
|
||||||
|
return {
|
||||||
|
output: current + appended,
|
||||||
|
bytes: MAX_DOCTOR_OUTPUT_BYTES,
|
||||||
|
truncated: true,
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
function getBundledBinPath(): string {
|
function getBundledBinPath(): string {
|
||||||
const target = `${process.platform}-${process.arch}`;
|
const target = `${process.platform}-${process.arch}`;
|
||||||
return app.isPackaged
|
return app.isPackaged
|
||||||
@@ -79,6 +113,10 @@ async function runDoctorCommand(mode: OpenClawDoctorMode): Promise<OpenClawDocto
|
|||||||
|
|
||||||
let stdout = '';
|
let stdout = '';
|
||||||
let stderr = '';
|
let stderr = '';
|
||||||
|
let stdoutBytes = 0;
|
||||||
|
let stderrBytes = 0;
|
||||||
|
let stdoutTruncated = false;
|
||||||
|
let stderrTruncated = false;
|
||||||
let settled = false;
|
let settled = false;
|
||||||
|
|
||||||
const finish = (result: Omit<OpenClawDoctorResult, 'durationMs'>) => {
|
const finish = (result: Omit<OpenClawDoctorResult, 'durationMs'>) => {
|
||||||
@@ -111,11 +149,17 @@ async function runDoctorCommand(mode: OpenClawDoctorMode): Promise<OpenClawDocto
|
|||||||
}, OPENCLAW_DOCTOR_TIMEOUT_MS);
|
}, OPENCLAW_DOCTOR_TIMEOUT_MS);
|
||||||
|
|
||||||
child.stdout?.on('data', (data) => {
|
child.stdout?.on('data', (data) => {
|
||||||
stdout += data.toString();
|
const next = appendDoctorOutput(stdout, stdoutBytes, data, 'stdout', stdoutTruncated);
|
||||||
|
stdout = next.output;
|
||||||
|
stdoutBytes = next.bytes;
|
||||||
|
stdoutTruncated = next.truncated;
|
||||||
});
|
});
|
||||||
|
|
||||||
child.stderr?.on('data', (data) => {
|
child.stderr?.on('data', (data) => {
|
||||||
stderr += data.toString();
|
const next = appendDoctorOutput(stderr, stderrBytes, data, 'stderr', stderrTruncated);
|
||||||
|
stderr = next.output;
|
||||||
|
stderrBytes = next.bytes;
|
||||||
|
stderrTruncated = next.truncated;
|
||||||
});
|
});
|
||||||
|
|
||||||
child.on('error', (error) => {
|
child.on('error', (error) => {
|
||||||
|
|||||||
104
tests/unit/channel-config.test.ts
Normal file
104
tests/unit/channel-config.test.ts
Normal file
@@ -0,0 +1,104 @@
|
|||||||
|
import { readFile, rm } from 'fs/promises';
|
||||||
|
import { join } from 'path';
|
||||||
|
import { beforeEach, describe, expect, it, vi } from 'vitest';
|
||||||
|
|
||||||
|
const { testHome, testUserData, mockLoggerWarn, mockLoggerInfo, mockLoggerError } = vi.hoisted(() => {
|
||||||
|
const suffix = Math.random().toString(36).slice(2);
|
||||||
|
return {
|
||||||
|
testHome: `/tmp/clawx-channel-config-${suffix}`,
|
||||||
|
testUserData: `/tmp/clawx-channel-config-user-data-${suffix}`,
|
||||||
|
mockLoggerWarn: vi.fn(),
|
||||||
|
mockLoggerInfo: vi.fn(),
|
||||||
|
mockLoggerError: vi.fn(),
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
|
vi.mock('os', async () => {
|
||||||
|
const actual = await vi.importActual<typeof import('os')>('os');
|
||||||
|
const mocked = {
|
||||||
|
...actual,
|
||||||
|
homedir: () => testHome,
|
||||||
|
};
|
||||||
|
return {
|
||||||
|
...mocked,
|
||||||
|
default: mocked,
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
|
vi.mock('electron', () => ({
|
||||||
|
app: {
|
||||||
|
isPackaged: false,
|
||||||
|
getPath: () => testUserData,
|
||||||
|
getVersion: () => '0.0.0-test',
|
||||||
|
getAppPath: () => '/tmp',
|
||||||
|
},
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock('@electron/utils/logger', () => ({
|
||||||
|
warn: mockLoggerWarn,
|
||||||
|
info: mockLoggerInfo,
|
||||||
|
error: mockLoggerError,
|
||||||
|
}));
|
||||||
|
|
||||||
|
async function readOpenClawJson(): Promise<Record<string, unknown>> {
|
||||||
|
const content = await readFile(join(testHome, '.openclaw', 'openclaw.json'), 'utf8');
|
||||||
|
return JSON.parse(content) as Record<string, unknown>;
|
||||||
|
}
|
||||||
|
|
||||||
|
describe('channel credential normalization and duplicate checks', () => {
|
||||||
|
beforeEach(async () => {
|
||||||
|
vi.resetAllMocks();
|
||||||
|
vi.resetModules();
|
||||||
|
await rm(testHome, { recursive: true, force: true });
|
||||||
|
await rm(testUserData, { recursive: true, force: true });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('assertNoDuplicateCredential detects duplicates with different whitespace', async () => {
|
||||||
|
const { saveChannelConfig } = await import('@electron/utils/channel-config');
|
||||||
|
|
||||||
|
await saveChannelConfig('feishu', { appId: 'bot-123', appSecret: 'secret-a' }, 'agent-a');
|
||||||
|
|
||||||
|
await expect(
|
||||||
|
saveChannelConfig('feishu', { appId: ' bot-123 ', appSecret: 'secret-b' }, 'agent-b'),
|
||||||
|
).rejects.toThrow('already bound to another agent');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('assertNoDuplicateCredential does NOT detect duplicates with different case', async () => {
|
||||||
|
// Case-sensitive credentials (like tokens) should NOT be normalized to lowercase
|
||||||
|
// to avoid false positives where different tokens become the same after lowercasing
|
||||||
|
const { saveChannelConfig } = await import('@electron/utils/channel-config');
|
||||||
|
|
||||||
|
await saveChannelConfig('feishu', { appId: 'Bot-ABC', appSecret: 'secret-a' }, 'agent-a');
|
||||||
|
|
||||||
|
// Should NOT throw - different case is considered a different credential
|
||||||
|
await expect(
|
||||||
|
saveChannelConfig('feishu', { appId: 'bot-abc', appSecret: 'secret-b' }, 'agent-b'),
|
||||||
|
).resolves.not.toThrow();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('normalizes credential values when saving (trim only, preserve case)', async () => {
|
||||||
|
const { saveChannelConfig } = await import('@electron/utils/channel-config');
|
||||||
|
|
||||||
|
await saveChannelConfig('feishu', { appId: ' BoT-XyZ ', appSecret: 'secret' }, 'agent-a');
|
||||||
|
|
||||||
|
const config = await readOpenClawJson();
|
||||||
|
const channels = config.channels as Record<string, { accounts: Record<string, { appId?: string }> }>;
|
||||||
|
// Should trim whitespace but preserve original case
|
||||||
|
expect(channels.feishu.accounts['agent-a'].appId).toBe('BoT-XyZ');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('emits warning logs when credential normalization (trim) occurs', async () => {
|
||||||
|
const { saveChannelConfig } = await import('@electron/utils/channel-config');
|
||||||
|
|
||||||
|
await saveChannelConfig('feishu', { appId: ' BoT-Log ', appSecret: 'secret' }, 'agent-a');
|
||||||
|
|
||||||
|
expect(mockLoggerWarn).toHaveBeenCalledWith(
|
||||||
|
'Normalized channel credential value for duplicate check',
|
||||||
|
expect.objectContaining({ channelType: 'feishu', accountId: 'agent-a', key: 'appId' }),
|
||||||
|
);
|
||||||
|
expect(mockLoggerWarn).toHaveBeenCalledWith(
|
||||||
|
'Normalizing channel credential value before save',
|
||||||
|
expect.objectContaining({ channelType: 'feishu', accountId: 'agent-a', key: 'appId' }),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
158
tests/unit/openclaw-doctor.test.ts
Normal file
158
tests/unit/openclaw-doctor.test.ts
Normal file
@@ -0,0 +1,158 @@
|
|||||||
|
import { EventEmitter } from 'node:events';
|
||||||
|
import { beforeEach, describe, expect, it, vi } from 'vitest';
|
||||||
|
|
||||||
|
const MAX_DOCTOR_OUTPUT_BYTES = 10 * 1024 * 1024;
|
||||||
|
|
||||||
|
const {
|
||||||
|
mockExistsSync,
|
||||||
|
mockFork,
|
||||||
|
mockGetUvMirrorEnv,
|
||||||
|
mockLoggerWarn,
|
||||||
|
mockLoggerInfo,
|
||||||
|
mockLoggerError,
|
||||||
|
} = vi.hoisted(() => ({
|
||||||
|
mockExistsSync: vi.fn(),
|
||||||
|
mockFork: vi.fn(),
|
||||||
|
mockGetUvMirrorEnv: vi.fn(),
|
||||||
|
mockLoggerWarn: vi.fn(),
|
||||||
|
mockLoggerInfo: vi.fn(),
|
||||||
|
mockLoggerError: vi.fn(),
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock('node:fs', async () => {
|
||||||
|
const actual = await vi.importActual<typeof import('node:fs')>('node:fs');
|
||||||
|
return {
|
||||||
|
...actual,
|
||||||
|
existsSync: mockExistsSync,
|
||||||
|
default: {
|
||||||
|
...actual,
|
||||||
|
existsSync: mockExistsSync,
|
||||||
|
},
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
|
vi.mock('electron', () => ({
|
||||||
|
app: {
|
||||||
|
isPackaged: false,
|
||||||
|
},
|
||||||
|
utilityProcess: {
|
||||||
|
fork: mockFork,
|
||||||
|
},
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock('@electron/utils/paths', () => ({
|
||||||
|
getOpenClawDir: () => '/tmp/openclaw',
|
||||||
|
getOpenClawEntryPath: () => '/tmp/openclaw/openclaw-entry.js',
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock('@electron/utils/uv-env', () => ({
|
||||||
|
getUvMirrorEnv: mockGetUvMirrorEnv,
|
||||||
|
}));
|
||||||
|
|
||||||
|
vi.mock('@electron/utils/logger', () => ({
|
||||||
|
logger: {
|
||||||
|
warn: mockLoggerWarn,
|
||||||
|
info: mockLoggerInfo,
|
||||||
|
error: mockLoggerError,
|
||||||
|
},
|
||||||
|
}));
|
||||||
|
|
||||||
|
class MockUtilityChild extends EventEmitter {
|
||||||
|
stdout = new EventEmitter();
|
||||||
|
stderr = new EventEmitter();
|
||||||
|
kill = vi.fn();
|
||||||
|
}
|
||||||
|
|
||||||
|
describe('openclaw doctor output handling', () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
vi.resetAllMocks();
|
||||||
|
vi.resetModules();
|
||||||
|
|
||||||
|
mockExistsSync.mockReturnValue(true);
|
||||||
|
mockGetUvMirrorEnv.mockResolvedValue({});
|
||||||
|
});
|
||||||
|
|
||||||
|
it('collects normal output under the buffer limit', async () => {
|
||||||
|
const child = new MockUtilityChild();
|
||||||
|
mockFork.mockReturnValue(child);
|
||||||
|
|
||||||
|
const { runOpenClawDoctor } = await import('@electron/utils/openclaw-doctor');
|
||||||
|
const resultPromise = runOpenClawDoctor();
|
||||||
|
|
||||||
|
await vi.waitFor(() => {
|
||||||
|
expect(mockFork).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
|
child.stdout.emit('data', Buffer.from('doctor ok\n'));
|
||||||
|
child.emit('exit', 0);
|
||||||
|
|
||||||
|
const result = await resultPromise;
|
||||||
|
expect(result.success).toBe(true);
|
||||||
|
expect(result.exitCode).toBe(0);
|
||||||
|
expect(result.stdout).toBe('doctor ok\n');
|
||||||
|
expect(result.stderr).toBe('');
|
||||||
|
expect(mockLoggerWarn).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('truncates output when stdout exceeds MAX_DOCTOR_OUTPUT_BYTES', async () => {
|
||||||
|
const child = new MockUtilityChild();
|
||||||
|
mockFork.mockReturnValue(child);
|
||||||
|
|
||||||
|
const { runOpenClawDoctor } = await import('@electron/utils/openclaw-doctor');
|
||||||
|
const resultPromise = runOpenClawDoctor();
|
||||||
|
|
||||||
|
await vi.waitFor(() => {
|
||||||
|
expect(mockFork).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
|
child.stdout.emit('data', Buffer.from('a'.repeat(MAX_DOCTOR_OUTPUT_BYTES - 5)));
|
||||||
|
child.stdout.emit('data', Buffer.from('b'.repeat(10)));
|
||||||
|
child.stdout.emit('data', Buffer.from('c'.repeat(1000)));
|
||||||
|
child.emit('exit', 0);
|
||||||
|
|
||||||
|
const result = await resultPromise;
|
||||||
|
expect(result.stdout.length).toBe(MAX_DOCTOR_OUTPUT_BYTES);
|
||||||
|
expect(result.stdout.endsWith('bbbbb')).toBe(true);
|
||||||
|
expect(result.stdout.includes('c')).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('logs a warning when truncation occurs', async () => {
|
||||||
|
const child = new MockUtilityChild();
|
||||||
|
mockFork.mockReturnValue(child);
|
||||||
|
|
||||||
|
const { runOpenClawDoctor } = await import('@electron/utils/openclaw-doctor');
|
||||||
|
const resultPromise = runOpenClawDoctor();
|
||||||
|
|
||||||
|
await vi.waitFor(() => {
|
||||||
|
expect(mockFork).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
|
child.stdout.emit('data', Buffer.from('x'.repeat(MAX_DOCTOR_OUTPUT_BYTES + 1)));
|
||||||
|
child.emit('exit', 0);
|
||||||
|
|
||||||
|
await resultPromise;
|
||||||
|
expect(mockLoggerWarn).toHaveBeenCalledWith(
|
||||||
|
`OpenClaw doctor stdout exceeded ${MAX_DOCTOR_OUTPUT_BYTES} bytes; truncating additional output`,
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('collects stdout and stderr independently', async () => {
|
||||||
|
const child = new MockUtilityChild();
|
||||||
|
mockFork.mockReturnValue(child);
|
||||||
|
|
||||||
|
const { runOpenClawDoctor } = await import('@electron/utils/openclaw-doctor');
|
||||||
|
const resultPromise = runOpenClawDoctor();
|
||||||
|
|
||||||
|
await vi.waitFor(() => {
|
||||||
|
expect(mockFork).toHaveBeenCalledTimes(1);
|
||||||
|
});
|
||||||
|
child.stdout.emit('data', Buffer.from('line-1\n'));
|
||||||
|
child.stderr.emit('data', Buffer.from('warn-1\n'));
|
||||||
|
child.stdout.emit('data', Buffer.from('line-2\n'));
|
||||||
|
child.stderr.emit('data', Buffer.from('warn-2\n'));
|
||||||
|
child.emit('exit', 1);
|
||||||
|
|
||||||
|
const result = await resultPromise;
|
||||||
|
expect(result.success).toBe(false);
|
||||||
|
expect(result.exitCode).toBe(1);
|
||||||
|
expect(result.stdout).toBe('line-1\nline-2\n');
|
||||||
|
expect(result.stderr).toBe('warn-1\nwarn-2\n');
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user