fix: prevent config overwrite, session history race, and streaming message loss (#663)
Co-authored-by: zuolingxuan <zuolingxuan@bytedance.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
8c3a6a5f7a
commit
83858fdf73
@@ -385,6 +385,33 @@ export async function removeProviderFromOpenClaw(provider: string): Promise<void
|
|||||||
console.log(`Removed OpenClaw provider config: ${provider}`);
|
console.log(`Removed OpenClaw provider config: ${provider}`);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Clean up agents.defaults.model references that point to the deleted provider.
|
||||||
|
// Model refs use the format "providerType/modelId", e.g. "openai/gpt-4".
|
||||||
|
// Leaving stale refs causes the Gateway to report "Unknown model" errors.
|
||||||
|
const agents = config.agents as Record<string, unknown> | undefined;
|
||||||
|
const agentDefaults = (agents?.defaults && typeof agents.defaults === 'object'
|
||||||
|
? agents.defaults as Record<string, unknown>
|
||||||
|
: null);
|
||||||
|
if (agentDefaults?.model && typeof agentDefaults.model === 'object') {
|
||||||
|
const modelCfg = agentDefaults.model as Record<string, unknown>;
|
||||||
|
const prefix = `${provider}/`;
|
||||||
|
|
||||||
|
if (typeof modelCfg.primary === 'string' && modelCfg.primary.startsWith(prefix)) {
|
||||||
|
delete modelCfg.primary;
|
||||||
|
modified = true;
|
||||||
|
console.log(`Removed deleted provider "${provider}" from agents.defaults.model.primary`);
|
||||||
|
}
|
||||||
|
|
||||||
|
if (Array.isArray(modelCfg.fallbacks)) {
|
||||||
|
const filtered = (modelCfg.fallbacks as string[]).filter((fb) => !fb.startsWith(prefix));
|
||||||
|
if (filtered.length !== modelCfg.fallbacks.length) {
|
||||||
|
modelCfg.fallbacks = filtered.length > 0 ? filtered : undefined;
|
||||||
|
modified = true;
|
||||||
|
console.log(`Removed deleted provider "${provider}" from agents.defaults.model.fallbacks`);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
if (modified) {
|
if (modified) {
|
||||||
await writeOpenClawJson(config);
|
await writeOpenClawJson(config);
|
||||||
}
|
}
|
||||||
@@ -1017,7 +1044,24 @@ export async function updateSingleAgentModelProvider(
|
|||||||
*/
|
*/
|
||||||
export async function sanitizeOpenClawConfig(): Promise<void> {
|
export async function sanitizeOpenClawConfig(): Promise<void> {
|
||||||
return withConfigLock(async () => {
|
return withConfigLock(async () => {
|
||||||
const config = await readOpenClawJson();
|
// Skip sanitization if the config file does not exist yet.
|
||||||
|
// Creating a skeleton config here would overwrite any data written
|
||||||
|
// by the Gateway on its first run.
|
||||||
|
if (!(await fileExists(OPENCLAW_CONFIG_PATH))) {
|
||||||
|
console.log('[sanitize] openclaw.json does not exist yet, skipping sanitization');
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Read the raw file directly instead of going through readOpenClawJson()
|
||||||
|
// which coalesces null → {}. We need to distinguish a genuinely empty
|
||||||
|
// file (valid, proceed normally) from a corrupt/unreadable file (null,
|
||||||
|
// bail out to avoid overwriting the user's data with a skeleton config).
|
||||||
|
const rawConfig = await readJsonFile<Record<string, unknown>>(OPENCLAW_CONFIG_PATH);
|
||||||
|
if (rawConfig === null) {
|
||||||
|
console.log('[sanitize] openclaw.json could not be parsed, skipping sanitization to preserve data');
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
const config: Record<string, unknown> = rawConfig;
|
||||||
let modified = false;
|
let modified = false;
|
||||||
|
|
||||||
// ── skills section ──────────────────────────────────────────────
|
// ── skills section ──────────────────────────────────────────────
|
||||||
|
|||||||
@@ -45,7 +45,16 @@ export function subscribeHostEvent<T = unknown>(
|
|||||||
const listener = (payload: unknown) => {
|
const listener = (payload: unknown) => {
|
||||||
handler(payload as T);
|
handler(payload as T);
|
||||||
};
|
};
|
||||||
ipc.on(ipcChannel, listener);
|
// preload's `on()` wraps the callback in an internal subscription function
|
||||||
|
// and returns a cleanup function that removes that exact wrapper. We MUST
|
||||||
|
// use the returned cleanup rather than calling `off(channel, listener)`,
|
||||||
|
// because `listener` !== the internal wrapper and removeListener would be
|
||||||
|
// a no-op, leaking the subscription.
|
||||||
|
const unsubscribe = ipc.on(ipcChannel, listener);
|
||||||
|
if (typeof unsubscribe === 'function') {
|
||||||
|
return unsubscribe;
|
||||||
|
}
|
||||||
|
// Fallback for environments where on() doesn't return cleanup
|
||||||
return () => {
|
return () => {
|
||||||
ipc.off(ipcChannel, listener);
|
ipc.off(ipcChannel, listener);
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -1141,6 +1141,10 @@ export const useChatStore = create<ChatState>((set, get) => ({
|
|||||||
|
|
||||||
switchSession: (key: string) => {
|
switchSession: (key: string) => {
|
||||||
if (key === get().currentSessionKey) return;
|
if (key === get().currentSessionKey) return;
|
||||||
|
// Stop any background polling for the old session before switching.
|
||||||
|
// This prevents the poll timer from firing after the switch and loading
|
||||||
|
// the wrong session's history into the new session's view.
|
||||||
|
clearHistoryPoll();
|
||||||
set((s) => buildSessionSwitchPatch(s, key));
|
set((s) => buildSessionSwitchPatch(s, key));
|
||||||
get().loadHistory();
|
get().loadHistory();
|
||||||
},
|
},
|
||||||
@@ -1303,6 +1307,11 @@ export const useChatStore = create<ChatState>((set, get) => ({
|
|||||||
|
|
||||||
const loadPromise = (async () => {
|
const loadPromise = (async () => {
|
||||||
const applyLoadedMessages = (rawMessages: RawMessage[], thinkingLevel: string | null) => {
|
const applyLoadedMessages = (rawMessages: RawMessage[], thinkingLevel: string | null) => {
|
||||||
|
// Guard: if the user switched sessions while this async load was in
|
||||||
|
// flight, discard the result to prevent overwriting the new session's
|
||||||
|
// messages with stale data from the old session.
|
||||||
|
if (get().currentSessionKey !== currentSessionKey) return;
|
||||||
|
|
||||||
// Before filtering: attach images/files from tool_result messages to the next assistant message
|
// Before filtering: attach images/files from tool_result messages to the next assistant message
|
||||||
const messagesWithToolImages = enrichWithToolResultFiles(rawMessages);
|
const messagesWithToolImages = enrichWithToolResultFiles(rawMessages);
|
||||||
const filteredMessages = messagesWithToolImages.filter((msg) => !isToolResultRole(msg.role));
|
const filteredMessages = messagesWithToolImages.filter((msg) => !isToolResultRole(msg.role));
|
||||||
|
|||||||
@@ -48,6 +48,22 @@ export function handleRuntimeEventState(
|
|||||||
if (event.message && typeof event.message === 'object') {
|
if (event.message && typeof event.message === 'object') {
|
||||||
const msgRole = (event.message as RawMessage).role;
|
const msgRole = (event.message as RawMessage).role;
|
||||||
if (isToolResultRole(msgRole)) return s.streamingMessage;
|
if (isToolResultRole(msgRole)) return s.streamingMessage;
|
||||||
|
// During multi-model fallback the Gateway may emit a delta with an
|
||||||
|
// empty or role-only message (e.g. `{}` or `{ role: 'assistant' }`)
|
||||||
|
// to signal a model switch. Accepting such a value would silently
|
||||||
|
// discard all content accumulated so far in streamingMessage.
|
||||||
|
// Only replace when the incoming message carries actual payload.
|
||||||
|
const msgObj = event.message as RawMessage;
|
||||||
|
// During multi-model fallback the Gateway may emit an empty or
|
||||||
|
// role-only delta (e.g. `{}` or `{ role: 'assistant' }`) to signal
|
||||||
|
// a model switch. If we already have accumulated streaming content,
|
||||||
|
// accepting such a message would silently discard it. Only guard
|
||||||
|
// when there IS existing content to protect; when streamingMessage
|
||||||
|
// is still null, let any delta through so the UI can start showing
|
||||||
|
// the typing indicator immediately.
|
||||||
|
if (s.streamingMessage && msgObj.content === undefined) {
|
||||||
|
return s.streamingMessage;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
return event.message ?? s.streamingMessage;
|
return event.message ?? s.streamingMessage;
|
||||||
})(),
|
})(),
|
||||||
|
|||||||
@@ -142,6 +142,46 @@ describe('chat runtime event handlers', () => {
|
|||||||
expect(next.streamingTools).toEqual([]);
|
expect(next.streamingTools).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('delta with empty object does not overwrite existing streamingMessage', async () => {
|
||||||
|
// Regression test for multi-model fallback: Gateway emits {} during model switch.
|
||||||
|
// The existing streamingMessage content must be preserved.
|
||||||
|
const { handleRuntimeEventState } = await import('@/stores/chat/runtime-event-handlers');
|
||||||
|
const existing = { role: 'assistant', content: [{ type: 'text', text: 'hello' }] };
|
||||||
|
const h = makeHarness({ streamingMessage: existing });
|
||||||
|
|
||||||
|
handleRuntimeEventState(h.set as never, h.get as never, { message: {} }, 'delta', 'run-x');
|
||||||
|
expect(h.read().streamingMessage).toEqual(existing);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('delta with role-only object does not overwrite existing streamingMessage', async () => {
|
||||||
|
const { handleRuntimeEventState } = await import('@/stores/chat/runtime-event-handlers');
|
||||||
|
const existing = { role: 'assistant', content: [{ type: 'text', text: 'partial' }] };
|
||||||
|
const h = makeHarness({ streamingMessage: existing });
|
||||||
|
|
||||||
|
handleRuntimeEventState(h.set as never, h.get as never, { message: { role: 'assistant' } }, 'delta', 'run-x');
|
||||||
|
expect(h.read().streamingMessage).toEqual(existing);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('delta with empty object is accepted when streamingMessage is null (initial state)', async () => {
|
||||||
|
// When streaming hasn't started yet, even an empty delta should be let
|
||||||
|
// through so the UI can show a typing indicator immediately.
|
||||||
|
const { handleRuntimeEventState } = await import('@/stores/chat/runtime-event-handlers');
|
||||||
|
const h = makeHarness({ streamingMessage: null });
|
||||||
|
|
||||||
|
handleRuntimeEventState(h.set as never, h.get as never, { message: { role: 'assistant' } }, 'delta', 'run-x');
|
||||||
|
expect(h.read().streamingMessage).toEqual({ role: 'assistant' });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('delta with actual content replaces streamingMessage', async () => {
|
||||||
|
const { handleRuntimeEventState } = await import('@/stores/chat/runtime-event-handlers');
|
||||||
|
const existing = { role: 'assistant', content: [{ type: 'text', text: 'old' }] };
|
||||||
|
const incoming = { role: 'assistant', content: [{ type: 'text', text: 'new' }] };
|
||||||
|
const h = makeHarness({ streamingMessage: existing });
|
||||||
|
|
||||||
|
handleRuntimeEventState(h.set as never, h.get as never, { message: incoming }, 'delta', 'run-x');
|
||||||
|
expect(h.read().streamingMessage).toEqual(incoming);
|
||||||
|
});
|
||||||
|
|
||||||
it('clears runtime state on aborted event', async () => {
|
it('clears runtime state on aborted event', async () => {
|
||||||
const { handleRuntimeEventState } = await import('@/stores/chat/runtime-event-handlers');
|
const { handleRuntimeEventState } = await import('@/stores/chat/runtime-event-handlers');
|
||||||
const h = makeHarness({
|
const h = makeHarness({
|
||||||
|
|||||||
@@ -21,11 +21,11 @@ describe('host-events', () => {
|
|||||||
|
|
||||||
it('subscribes through IPC for mapped host events', async () => {
|
it('subscribes through IPC for mapped host events', async () => {
|
||||||
const onMock = vi.mocked(window.electron.ipcRenderer.on);
|
const onMock = vi.mocked(window.electron.ipcRenderer.on);
|
||||||
const offMock = vi.mocked(window.electron.ipcRenderer.off);
|
|
||||||
const captured: Array<(...args: unknown[]) => void> = [];
|
const captured: Array<(...args: unknown[]) => void> = [];
|
||||||
|
const cleanupSpy = vi.fn();
|
||||||
onMock.mockImplementation((_, cb: (...args: unknown[]) => void) => {
|
onMock.mockImplementation((_, cb: (...args: unknown[]) => void) => {
|
||||||
captured.push(cb);
|
captured.push(cb);
|
||||||
return () => {};
|
return cleanupSpy;
|
||||||
});
|
});
|
||||||
|
|
||||||
const { subscribeHostEvent } = await import('@/lib/host-events');
|
const { subscribeHostEvent } = await import('@/lib/host-events');
|
||||||
@@ -38,8 +38,10 @@ describe('host-events', () => {
|
|||||||
captured[0]({ state: 'running' });
|
captured[0]({ state: 'running' });
|
||||||
expect(handler).toHaveBeenCalledWith({ state: 'running' });
|
expect(handler).toHaveBeenCalledWith({ state: 'running' });
|
||||||
|
|
||||||
|
// unsubscribe should use the cleanup returned by ipc.on() — NOT ipc.off()
|
||||||
|
// which would pass the wrong function reference (see preload wrapper mismatch)
|
||||||
unsubscribe();
|
unsubscribe();
|
||||||
expect(offMock).toHaveBeenCalledWith('gateway:status-changed', expect.any(Function));
|
expect(cleanupSpy).toHaveBeenCalledTimes(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('does not use SSE fallback by default for unknown events', async () => {
|
it('does not use SSE fallback by default for unknown events', async () => {
|
||||||
|
|||||||
@@ -111,3 +111,93 @@ describe('saveProviderKeyToOpenClaw', () => {
|
|||||||
logSpy.mockRestore();
|
logSpy.mockRestore();
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('sanitizeOpenClawConfig', () => {
|
||||||
|
beforeEach(async () => {
|
||||||
|
vi.resetModules();
|
||||||
|
vi.restoreAllMocks();
|
||||||
|
await rm(testHome, { recursive: true, force: true });
|
||||||
|
await rm(testUserData, { recursive: true, force: true });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('skips sanitization when openclaw.json does not exist', async () => {
|
||||||
|
// Ensure the .openclaw dir doesn't exist at all
|
||||||
|
const { sanitizeOpenClawConfig } = await import('@electron/utils/openclaw-auth');
|
||||||
|
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
|
||||||
|
|
||||||
|
// Should not throw and should not create the file
|
||||||
|
await expect(sanitizeOpenClawConfig()).resolves.toBeUndefined();
|
||||||
|
|
||||||
|
const configPath = join(testHome, '.openclaw', 'openclaw.json');
|
||||||
|
await expect(readFile(configPath, 'utf8')).rejects.toThrow();
|
||||||
|
|
||||||
|
logSpy.mockRestore();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('skips sanitization when openclaw.json contains invalid JSON', async () => {
|
||||||
|
// Simulate a corrupted file: readJsonFile returns null, sanitize must bail out
|
||||||
|
const openclawDir = join(testHome, '.openclaw');
|
||||||
|
await mkdir(openclawDir, { recursive: true });
|
||||||
|
const configPath = join(openclawDir, 'openclaw.json');
|
||||||
|
await writeFile(configPath, 'NOT VALID JSON {{{', 'utf8');
|
||||||
|
const before = await readFile(configPath, 'utf8');
|
||||||
|
|
||||||
|
const { sanitizeOpenClawConfig } = await import('@electron/utils/openclaw-auth');
|
||||||
|
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
|
||||||
|
|
||||||
|
await sanitizeOpenClawConfig();
|
||||||
|
|
||||||
|
const after = await readFile(configPath, 'utf8');
|
||||||
|
// Corrupt file must not be overwritten
|
||||||
|
expect(after).toBe(before);
|
||||||
|
|
||||||
|
logSpy.mockRestore();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('properly sanitizes a genuinely empty {} config (fresh install)', async () => {
|
||||||
|
// A fresh install with {} is a valid config — sanitize should proceed
|
||||||
|
// and enforce tools.profile, commands.restart, etc.
|
||||||
|
await writeOpenClawJson({});
|
||||||
|
|
||||||
|
const { sanitizeOpenClawConfig } = await import('@electron/utils/openclaw-auth');
|
||||||
|
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
|
||||||
|
|
||||||
|
await sanitizeOpenClawConfig();
|
||||||
|
|
||||||
|
const configPath = join(testHome, '.openclaw', 'openclaw.json');
|
||||||
|
const result = JSON.parse(await readFile(configPath, 'utf8')) as Record<string, unknown>;
|
||||||
|
// Fresh install should get tools settings enforced
|
||||||
|
const tools = result.tools as Record<string, unknown>;
|
||||||
|
expect(tools.profile).toBe('full');
|
||||||
|
|
||||||
|
logSpy.mockRestore();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('preserves user config (memory, agents, channels) when enforcing tools settings', async () => {
|
||||||
|
await writeOpenClawJson({
|
||||||
|
agents: { defaults: { model: { primary: 'openai/gpt-4' } } },
|
||||||
|
channels: { discord: { token: 'tok', enabled: true } },
|
||||||
|
memory: { enabled: true, limit: 100 },
|
||||||
|
});
|
||||||
|
|
||||||
|
const { sanitizeOpenClawConfig } = await import('@electron/utils/openclaw-auth');
|
||||||
|
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
|
||||||
|
|
||||||
|
await sanitizeOpenClawConfig();
|
||||||
|
|
||||||
|
const configPath = join(testHome, '.openclaw', 'openclaw.json');
|
||||||
|
const result = JSON.parse(await readFile(configPath, 'utf8')) as Record<string, unknown>;
|
||||||
|
|
||||||
|
// User-owned sections must survive the sanitize pass
|
||||||
|
expect(result.memory).toEqual({ enabled: true, limit: 100 });
|
||||||
|
expect(result.channels).toEqual({ discord: { token: 'tok', enabled: true } });
|
||||||
|
expect((result.agents as Record<string, unknown>).defaults).toEqual({
|
||||||
|
model: { primary: 'openai/gpt-4' },
|
||||||
|
});
|
||||||
|
// tools settings should now be enforced
|
||||||
|
const tools = result.tools as Record<string, unknown>;
|
||||||
|
expect(tools.profile).toBe('full');
|
||||||
|
|
||||||
|
logSpy.mockRestore();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user