From 2474bb0e6c4b608414c3a1c61b928b5463496d9d Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Mon, 14 Sep 2026 14:23:19 +0800 Subject: [PATCH 1/8] fix(storage): back up corrupt settings before restoring defaults Recover only JSON parse failures after preserving the original bytes in an exclusive owner-only backup. Keep read, normalization and migration errors outside recovery, and distinguish failures before publication from an unconfirmed default-settings publication without replaying the mutation. Report recovery through localized desktop notifications and the existing settings effects queue. Track renderer delivery separately from applied settings so silent recovery cannot consume the pending change event. Cover byte preservation, permissions, fault boundaries, callback failures, queued mutations and desktop refresh behavior. Refs #4285 Generated-by: OpenAI Codex --- .../__tests__/client-settings-effects.test.ts | 34 +- .../main/__tests__/settings-recovery.test.ts | 228 ++++++++++ .../__tests__/startup-storage-repair.test.ts | 2 + .../src/main/client-settings-effects.ts | 14 +- apps/desktop/src/main/early-window.ts | 21 +- apps/desktop/src/main/runtime-host-boot.ts | 2 + apps/desktop/src/main/settings-recovery.ts | 155 +++++++ docs/windows-test-inventory.md | 9 +- .../settings-store-onboarding.test.ts | 17 +- .../__tests__/settings-store-recovery.test.ts | 426 ++++++++++++++++++ packages/storage/src/settings-store.ts | 173 ++++++- 11 files changed, 1045 insertions(+), 36 deletions(-) create mode 100644 apps/desktop/src/main/__tests__/settings-recovery.test.ts create mode 100644 apps/desktop/src/main/settings-recovery.ts create mode 100644 packages/storage/src/__tests__/settings-store-recovery.test.ts diff --git a/apps/desktop/src/main/__tests__/client-settings-effects.test.ts b/apps/desktop/src/main/__tests__/client-settings-effects.test.ts index 0dcdeda897..5c89e31c44 100644 --- a/apps/desktop/src/main/__tests__/client-settings-effects.test.ts +++ b/apps/desktop/src/main/__tests__/client-settings-effects.test.ts @@ -48,6 +48,7 @@ test('applies each client settings snapshot once across local writes and file wa }); assert.equal(await effects.refresh(false), true); + assert.equal(await effects.refresh(true), true); // First renderer delivery. assert.equal(await effects.refresh(true), false); current = { @@ -59,12 +60,43 @@ test('applies each client settings snapshot once across local writes and file wa assert.deepEqual(keepAwake, [false, true]); assert.equal(botApplications, 1); - assert.equal(rendererEvents, 1); + assert.equal(rendererEvents, 2); // The shipped default is already on screen before the first snapshot is // read, so a run that never leaves it must not touch the OS icon at all. assert.deepEqual(appIcons, []); }); +test('silent refreshes retain an undelivered renderer change without repeating effects', async () => { + let current = createDefaultSettings(); + const keepAwake: boolean[] = []; + const deliveredLocales: string[] = []; + const effects = createClientSettingsEffects({ + settingsStore: { get: async () => current }, + applyWorkHub: async () => undefined, + applyKeepSystemAwake: async (enabled) => { keepAwake.push(enabled); }, + applyBotSettings: async () => undefined, + applyAppIcon: async () => undefined, + systemPrefersDark: () => false, + observeLocale: () => undefined, + emitExternalChanged: () => { deliveredLocales.push(current.personalization.uiLocale); }, + }); + await effects.refresh(false); + current = { + ...current, + system: { keepSystemAwake: true }, + personalization: { ...current.personalization, uiLocale: 'zh-CN' }, + }; + assert.equal(await effects.refresh(false), true); + assert.equal(await effects.refresh(false), false); + assert.deepEqual(deliveredLocales, []); + // A later write supersedes the silently applied snapshot before delivery. + current = { ...current, personalization: { ...current.personalization, uiLocale: 'zh-TW' } }; + assert.equal(await effects.refresh(true), true); + assert.equal(await effects.refresh(true), false); + assert.deepEqual(deliveredLocales, ['zh-TW']); + assert.deepEqual(keepAwake, [false, true]); +}); + test('applies a chosen app icon once, and again only when the choice changes', async () => { let current = createDefaultSettings(); const appIcons: string[] = []; diff --git a/apps/desktop/src/main/__tests__/settings-recovery.test.ts b/apps/desktop/src/main/__tests__/settings-recovery.test.ts new file mode 100644 index 0000000000..645812cd55 --- /dev/null +++ b/apps/desktop/src/main/__tests__/settings-recovery.test.ts @@ -0,0 +1,228 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import assert from 'node:assert/strict'; +import fs, { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { syncBuiltinESMExports } from 'node:module'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { test, type TestContext } from 'node:test'; +import { UI_LOCALES } from '@maka/core/ui-locale'; +import { createDefaultSettings } from '@maka/core/settings'; +import { + createSettingsStore, + SettingsRecoveryCommitUnknownError, + type CorruptSettingsRecovery, +} from '@maka/storage/settings-store'; +import { createClientSettingsEffects } from '../client-settings-effects.js'; +import { createSettingsRecoveryReporter, settingsRecoveryCopy } from '../settings-recovery.js'; + +const event: CorruptSettingsRecovery = { + settingsPath: '/profile/settings.json', + backupPath: '/profile/settings.json.corrupt-1000-fixture', + outcome: 'recovered', +}; +const turn = () => new Promise((resolve) => setImmediate(resolve)); + +function harness(options: { e2e?: boolean; supported?: boolean; throws?: 'support' | 'create' | 'show' } = {}) { + const notices: { title: string; body: string }[] = []; + const logs: string[] = []; + const failures: (() => void)[] = []; + let supports = 0; + const reporter = createSettingsRecoveryReporter({ + e2e: options.e2e ?? false, + locale: () => 'en', + log: (message) => { logs.push(message); }, + notifications: { + isSupported() { + supports += 1; + if (options.throws === 'support') throw new Error('unsupported'); + return options.supported ?? true; + }, + create(copy, failed) { + if (options.throws === 'create') throw new Error('create failed'); + failures.push(failed); + return { show() { + if (options.throws === 'show') throw new Error('show failed'); + notices.push(copy); + } }; + }, + }, + }); + return { reporter, notices, logs, failures, supports: () => supports }; +} + +test('localized recovery copy names the backup, privacy review and uncertain outcome', () => { + for (const locale of UI_LOCALES) { + const recovered = settingsRecoveryCopy(event, locale); + const unknown = settingsRecoveryCopy({ ...event, outcome: 'commit-unknown' }, locale); + assert.ok(recovered.body.includes(event.backupPath)); + assert.ok(unknown.body.includes(event.backupPath)); + assert.notEqual(recovered.title, unknown.title); + assert.match(recovered.body, /Incognito|隐身|無痕/u); + assert.doesNotMatch(recovered.body + unknown.body, /now off|已关闭|已關閉/u); + assert.match(unknown.body, /unconfirmed|未确认|未確認/u); + const failed = settingsRecoveryCopy({ ...event, outcome: 'commit-unknown' }, locale, true); + assert.match(failed.body, /unconfirmed|未确认|未確認/u); + assert.match(failed.body, /Restart|重启|重新啟動/u); + } +}); + +test('early recovery is logged and reported immediately, then reread when effects become ready', async () => { + const h = harness(); + let refreshes = 0; + h.reporter.onRecovery(event); + assert.equal(h.notices.length, 1); + assert.ok(h.logs[0].includes(event.backupPath)); + assert.equal(refreshes, 0); + const effects = { refresh: async (notify: boolean) => { assert.equal(notify, true); refreshes += 1; return true; } }; + h.reporter.setEffects(effects); + await turn(); + assert.equal(refreshes, 1); + h.reporter.setEffects(effects); + await turn(); + assert.equal(refreshes, 1); +}); + +for (const options of [{ e2e: true }, { supported: false }]) { + test('notification suppression does not suppress recovery diagnostics or effects', async () => { + const h = harness(options); + let refreshed = false; + h.reporter.setEffects({ refresh: async () => { refreshed = true; return false; } }); + h.reporter.onRecovery(event); + await turn(); + assert.equal(h.notices.length, 0); + assert.equal(refreshed, true); + assert.ok(h.logs.some((line) => line.includes(event.backupPath))); + if (options.e2e) assert.equal(h.supports(), 0); + }); +} + +for (const phase of ['support', 'create', 'show'] as const) { + test(`notification ${phase} failure is isolated`, async () => { + const h = harness({ throws: phase }); + h.reporter.onRecovery(event); + await turn(); + assert.ok(h.logs.some((line) => line.includes('notification failed'))); + }); +} + +test('asynchronous native notification failure is logged without throwing', () => { + const h = harness(); + h.reporter.onRecovery(event); + assert.doesNotThrow(() => h.failures[0]()); + assert.ok(h.logs.some((line) => line.includes('notification failed'))); +}); + +test('refresh failure keeps the publication warning and does not leak the failing effect error', async () => { + const h = harness(); + h.reporter.setEffects({ refresh: async () => { throw new Error('secret effect detail'); } }); + h.reporter.onRecovery({ ...event, outcome: 'commit-unknown' }); + await turn(); + assert.equal(h.notices.length, 2); + assert.match(h.notices[1].body, /unconfirmed/u); + assert.match(h.notices[1].body, /Restart/u); + assert.equal(JSON.stringify(h).includes('secret effect detail'), false); + assert.ok(h.logs.some((line) => line.includes('refresh failed'))); +}); + +async function realStore(t: TestContext) { + const root = await mkdtemp(join(tmpdir(), 'maka-desktop-settings-recovery-')); + t.after(async () => { + t.mock.restoreAll(); + syncBuiltinESMExports(); + await rm(root, { recursive: true, force: true }); + }); + const h = harness(); + const store = createSettingsStore(root, { onCorruptRecovery: h.reporter.onRecovery }); + const observed: string[] = []; + const bots: unknown[] = []; + const keepAwake: boolean[] = []; + let changes = 0; + const effects = createClientSettingsEffects({ + settingsStore: store, + systemPrefersDark: () => false, + applyWorkHub: async () => {}, + applyKeepSystemAwake: async (value) => { keepAwake.push(value); }, + applyBotSettings: async (value) => { bots.push(value); }, + applyAppIcon: async () => {}, + observeLocale: (settings) => { observed.push(settings.personalization.uiLocale); }, + emitExternalChanged: () => { changes += 1; }, + }); + return { root, path: join(root, 'settings.json'), h, store, effects, observed, bots, keepAwake, changes: () => changes }; +} + +for (const notifyRenderer of [false, true]) { + test(`recovery during effects.refresh(${notifyRenderer}) releases both queues and notifies the renderer once`, { timeout: 5_000 }, async (t) => { + const { path, h, effects, observed, bots, keepAwake, changes, store } = await realStore(t); + await store.update({ personalization: { uiLocale: 'zh-CN' }, system: { keepSystemAwake: true } }); + await effects.refresh(false); + h.reporter.setEffects(effects); + await writeFile(path, '{"secret":"never print this"'); + await effects.refresh(notifyRenderer); + await turn(); + await effects.refresh(true); // Barrier behind the callback's queued refresh. + assert.deepEqual(observed, ['zh-CN', 'auto', 'auto', 'auto']); + assert.deepEqual(keepAwake, [true, false]); + assert.equal(bots.length, 1); // Bots did not change, so effects deduplicate them. + assert.equal(changes(), 1); + assert.equal(h.notices.length, 1); + assert.equal(h.logs.join('').includes('never print this'), false); + }); +} + +test('recovery before effects initialization refreshes the latest file including a subsequent mutation', async (t) => { + const { path, h, effects, store, observed } = await realStore(t); + await writeFile(path, ''); + await store.update({ personalization: { uiLocale: 'zh-TW' } }); + h.reporter.setEffects(effects); + await turn(); + await effects.refresh(true); + assert.ok(observed.every((locale) => locale === 'zh-TW')); + assert.equal(h.notices.length, 1); +}); + +test('published reset failure is independently reported and consumers reread without replaying a mutation', { + skip: process.platform === 'win32', timeout: 5_000, +}, async (t) => { + const { root, path, h, effects, store, observed } = await realStore(t); + await store.update({ personalization: { uiLocale: 'zh-CN' } }); + await effects.refresh(false); + h.reporter.setEffects(effects); + await writeFile(path, '{bad'); + const originalOpen = fs.open; + let directoryCount = 0; + t.mock.method(fs, 'open', async (...args: Parameters) => { + const handle = await originalOpen(...args); + if (args[0] === root && ++directoryCount === 2) { + t.mock.method(handle, 'sync', async () => { throw new Error('injected reset fence failure'); }); + } + return handle; + }); + syncBuiltinESMExports(); + let patched = false; + await assert.rejects(store.updateIf(() => { patched = true; return true; }, { personalization: { uiLocale: 'en' } }), SettingsRecoveryCommitUnknownError); + await turn(); + await effects.refresh(true); + assert.equal(patched, false); + assert.equal(observed.at(-1), 'auto'); + assert.match(h.notices[0].body, /unconfirmed/u); + assert.equal(h.notices.length, 1); + assert.deepEqual(JSON.parse(await readFile(path, 'utf8')), createDefaultSettings()); +}); diff --git a/apps/desktop/src/main/__tests__/startup-storage-repair.test.ts b/apps/desktop/src/main/__tests__/startup-storage-repair.test.ts index 59ead83c04..324b6e5e6a 100644 --- a/apps/desktop/src/main/__tests__/startup-storage-repair.test.ts +++ b/apps/desktop/src/main/__tests__/startup-storage-repair.test.ts @@ -33,6 +33,7 @@ import type { BrowserMessageBoxAppearance } from '../browser-message-box.js'; import { showMessageBoxWithDiagnostics } from '../native-diagnostic-dialog.js'; import { getNativeDiagnosticDialogCopy } from '../native-diagnostic-dialog-copy.js'; import { resolveDesktopStorageRoot } from '../storage-root-startup.js'; +import { createSettingsRecoveryReporter } from '../settings-recovery.js'; import { startupStep } from '../startup-step.js'; import { resolveWindowRevealMode } from '../window-reveal.js'; @@ -118,6 +119,7 @@ for (const accept of [false, true]) { assert.equal(await readFile(markerPath, 'utf8'), staleMarker); return { response: accept ? 0 : 1, checkboxChecked: false }; }, + createSettingsRecoveryReporter, createSettingsStore: () => { settingsOpened = true; throw stopped; }, }; const completion = runInNewContext(`${boot}\nmodule.exports.default()`, { diff --git a/apps/desktop/src/main/client-settings-effects.ts b/apps/desktop/src/main/client-settings-effects.ts index 82a1795f80..75e744616e 100644 --- a/apps/desktop/src/main/client-settings-effects.ts +++ b/apps/desktop/src/main/client-settings-effects.ts @@ -49,6 +49,7 @@ interface ClientSettingsEffectDependencies { export function createClientSettingsEffects( dependencies: ClientSettingsEffectDependencies, ): ClientSettingsEffects { + let appliedSettingsFingerprint: string | undefined; let rendererFingerprint: string | undefined; let botFingerprint: string | undefined; let keepSystemAwake: boolean | undefined; @@ -67,6 +68,7 @@ export function createClientSettingsEffects( const settings = await load(); const nextRendererFingerprint = JSON.stringify(settings); const nextBotFingerprint = JSON.stringify(settings.botChat); + const settingsChanged = nextRendererFingerprint !== appliedSettingsFingerprint; const rendererChanged = nextRendererFingerprint !== rendererFingerprint; const keepAwakeChanged = settings.system.keepSystemAwake !== keepSystemAwake; const botChanged = nextBotFingerprint !== botFingerprint; @@ -99,9 +101,15 @@ export function createClientSettingsEffects( await dependencies.applyAppIcon(nextAppIcon); appIcon = nextAppIcon; } - rendererFingerprint = nextRendererFingerprint; - if (notifyRenderer && rendererChanged) dependencies.emitExternalChanged(); - return rendererChanged || keepAwakeChanged || botChanged || appIconChanged; + appliedSettingsFingerprint = nextRendererFingerprint; + const rendererNotified = notifyRenderer && rendererChanged; + // A silent refresh may apply recovered settings before the recovery + // callback runs. Only an actual delivery consumes the renderer change. + if (rendererNotified) { + dependencies.emitExternalChanged(); + rendererFingerprint = nextRendererFingerprint; + } + return settingsChanged || rendererNotified || keepAwakeChanged || botChanged || appIconChanged; }); tail = run.then( () => undefined, diff --git a/apps/desktop/src/main/early-window.ts b/apps/desktop/src/main/early-window.ts index 6bdd04df4f..858d515c7a 100644 --- a/apps/desktop/src/main/early-window.ts +++ b/apps/desktop/src/main/early-window.ts @@ -31,6 +31,7 @@ import { type MessageBoxOptions, type MessageBoxReturnValue, nativeTheme, + Notification, } from "electron"; import { resolveSystemUiLocale } from "@maka/core/ui-locale"; import { resolveStorageRoot } from "@maka/storage/root-authority"; @@ -57,7 +58,8 @@ import { showMessageBoxWithDiagnostics, } from "./native-diagnostic-dialog.js"; import { resolveShellEnv } from "./shell-env.js"; -import { revealMode } from "./startup-context.js"; +import { createSettingsRecoveryReporter } from "./settings-recovery.js"; +import { isIsolatedE2e, revealMode } from "./startup-context.js"; import { resolveDesktopStorageRoot } from "./storage-root-startup.js"; import { startupStep } from "./startup-step.js"; import { isDarkAppearance } from "./theme-source.js"; @@ -174,7 +176,22 @@ if (!resolvedLocalStorageRoot) { throw new Error("Desktop storage root resolution did not complete"); } export const startupLocalStorageRoot = resolvedLocalStorageRoot; -export const settingsStore = createSettingsStore(workspaceRoot); +export const settingsRecovery = createSettingsRecoveryReporter({ + e2e: isIsolatedE2e, + locale: () => resolveSystemUiLocale(app.getPreferredSystemLanguages()), + notifications: { + isSupported: () => Notification.isSupported(), + create: (copy, failed) => { + const notification = new Notification(copy); + notification.on('failed', failed); + return notification; + }, + }, + log: (message) => console.warn(message), +}); +export const settingsStore = createSettingsStore(workspaceRoot, { + onCorruptRecovery: settingsRecovery.onRecovery, +}); export const desktopLocale = createDesktopLocaleAuthority({ readSettings: () => settingsStore.get(), preferredSystemLanguages: () => app.getPreferredSystemLanguages(), diff --git a/apps/desktop/src/main/runtime-host-boot.ts b/apps/desktop/src/main/runtime-host-boot.ts index 18d81b452a..2b987831aa 100644 --- a/apps/desktop/src/main/runtime-host-boot.ts +++ b/apps/desktop/src/main/runtime-host-boot.ts @@ -92,6 +92,7 @@ import { mainWindowController, mainWindowDelegates, quitCoordinator, + settingsRecovery, settingsStore, shellEnvReady, showDesktopMessageBox, @@ -852,6 +853,7 @@ const clientSettingsEffects = createClientSettingsEffects({ sendActiveRuntimeHostEvent("settings:externalChanged", { ts: Date.now() }); }, }); +settingsRecovery.setEffects(clientSettingsEffects); // An OS appearance flip changes no setting, so nothing else would notice it. // Only the icon depends on the answer, and `refresh` re-resolves it and // no-ops when the resolved tile is the one already applied — which is the diff --git a/apps/desktop/src/main/settings-recovery.ts b/apps/desktop/src/main/settings-recovery.ts new file mode 100644 index 0000000000..65e61239af --- /dev/null +++ b/apps/desktop/src/main/settings-recovery.ts @@ -0,0 +1,155 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import type { UiCatalog, UiLocale } from '@maka/core/ui-locale'; +import type { CorruptSettingsRecovery } from '@maka/storage/settings-store'; +import type { ClientSettingsEffects } from './client-settings-effects.js'; + +interface RecoveryCopy { + title: string; + body: string; +} + +type RecoveryStatus = CorruptSettingsRecovery['outcome'] | 'refresh-failed'; + +const COPY = { + 'zh-CN': { + recovered: { + title: '设置已恢复为默认值', + body: '设置文件损坏，已恢复默认设置。请检查隐身模式、隐私和其他偏好。', + }, + 'commit-unknown': { + title: '设置已重置，保存状态待确认', + body: '默认设置已写入，但磁盘同步失败，持久保存状态尚未确认。请检查隐身模式和隐私设置。', + }, + 'refresh-failed': { + title: '运行中的设置刷新失败', + body: '设置文件已重置，但运行中的设置未能全部刷新。请重启应用并检查隐私设置。', + }, + backup: '原始文件备份：', + }, + 'zh-TW': { + recovered: { + title: '設定已還原為預設值', + body: '設定檔案損毀，已還原預設設定。請檢查無痕模式、隱私與其他偏好。', + }, + 'commit-unknown': { + title: '設定已重設，儲存狀態待確認', + body: '預設設定已寫入，但磁碟同步失敗，尚未確認是否持久儲存。請檢查無痕模式與隱私設定。', + }, + 'refresh-failed': { + title: '執行中的設定更新失敗', + body: '設定檔案已重設，但執行中的設定未能全部更新。請重新啟動應用程式並檢查隱私設定。', + }, + backup: '原始檔案備份：', + }, + en: { + recovered: { + title: 'Settings restored to defaults', + body: 'The settings file was invalid and has been reset. Please review Incognito, privacy and other preferences.', + }, + 'commit-unknown': { + title: 'Settings reset; save status uncertain', + body: 'Default settings were written, but disk synchronization failed and durability is unconfirmed. Please review Incognito and privacy settings.', + }, + 'refresh-failed': { + title: 'Running settings could not be refreshed', + body: 'The settings file was reset, but some running settings could not be refreshed. Restart the app and review your privacy settings.', + }, + backup: 'Original file backup: ', + }, +} satisfies UiCatalog & { backup: string }>; + +export function settingsRecoveryCopy( + recovery: CorruptSettingsRecovery, + locale: UiLocale, + refreshFailed = false, +): RecoveryCopy { + const copy = COPY[locale]; + const message = copy[refreshFailed ? 'refresh-failed' : recovery.outcome]; + // Preserve the durability warning when reporting a separate refresh failure. + const warning = refreshFailed && recovery.outcome === 'commit-unknown' + ? ` ${copy['commit-unknown'].body}` + : ''; + return { + title: message.title, + body: `${message.body}${warning} ${copy.backup}${recovery.backupPath}`, + }; +} + +interface SettingsRecoveryReporterDependencies { + readonly e2e: boolean; + readonly locale: () => UiLocale; + readonly notifications: { + isSupported(): boolean; + create(copy: RecoveryCopy, failed: () => void): { show(): void }; + }; + /** Receives only the result and paths, never JSON contents or parser errors. */ + readonly log: (message: string) => void; +} + +/** Observes recovery without becoming a second settings authority. Notification + * delivery is best-effort; refresh reuses the existing effects queue and rereads + * the file after the storage operation releases its own queue. */ +export function createSettingsRecoveryReporter(deps: SettingsRecoveryReporterDependencies) { + let effects: Pick | undefined; + let pending: CorruptSettingsRecovery | undefined; + + const log = (message: string): void => { + try { deps.log(message); } catch { /* Diagnostics must not block recovery. */ } + }; + const notify = (recovery: CorruptSettingsRecovery, refreshFailed = false): void => { + if (deps.e2e) return; + const failed = () => log(`[settings-recovery] notification failed; backup=${recovery.backupPath}`); + try { + if (!deps.notifications.isSupported()) { + log(`[settings-recovery] notifications unavailable; backup=${recovery.backupPath}`); + return; + } + deps.notifications.create(settingsRecoveryCopy(recovery, deps.locale(), refreshFailed), failed).show(); + } catch { + failed(); + } + }; + const refresh = (): void => { + const target = effects; + const recovery = pending; + if (!target || !recovery) return; + pending = undefined; + // Never await this from onRecovery: refresh can itself be the read that + // discovers corruption, and both storage and effects serialize operations. + void Promise.resolve().then(() => target.refresh(true)).catch(() => { + log(`[settings-recovery] refresh failed; outcome=${recovery.outcome}; backup=${recovery.backupPath}; restart the app`); + notify(recovery, true); + }); + }; + + return { + onRecovery(recovery: CorruptSettingsRecovery): void { + log(`[settings-recovery] outcome=${recovery.outcome}; settings=${recovery.settingsPath}; backup=${recovery.backupPath}`); + notify(recovery); + pending = recovery; + refresh(); + }, + setEffects(value: Pick): void { + effects = value; + refresh(); + }, + }; +} diff --git a/docs/windows-test-inventory.md b/docs/windows-test-inventory.md index 9c0649dd5a..a0007444e1 100644 --- a/docs/windows-test-inventory.md +++ b/docs/windows-test-inventory.md @@ -16,10 +16,10 @@ Locations intentionally omit line numbers so unrelated edits do not invalidate t | Classification | Count | |---|---:| | windows-backend-gap | 27 | -| portable-candidate | 40 | +| portable-candidate | 45 | | platform-contract | 38 | -Total Windows-excluded declarations: **105** +Total Windows-excluded declarations: **110** ## Inventory @@ -34,6 +34,7 @@ Total Windows-excluded declarations: **105** | portable-candidate | `apps/desktop/src/main/__tests__/opencli-chrome.test.ts` Windows opens the store page in installed Chrome, not the default browser | `process.platform === 'win32'` | | platform-contract | `apps/desktop/src/main/__tests__/project-context-root.test.ts` rejects a session cwd without read and traversal access | `process.platform === 'win32' ? 'POSIX permissions are required to make the session cwd inaccessible' : process.getuid?.() === 0` | | platform-contract | `apps/desktop/src/main/__tests__/runtime-host-skills-ipc-main.test.ts` reports create_failed without opening when a Skill directory parent is not writable | `process.platform === 'win32' ? 'POSIX permissions are required to make the Skill directory parent read-only' : process.getuid?.() === 0` | +| portable-candidate | `apps/desktop/src/main/__tests__/settings-recovery.test.ts` published reset failure is independently reported and consumers reread without replaying a mutation | `process.platform === 'win32'` | | platform-contract | `apps/desktop/src/main/__tests__/shell-env.test.ts` imports the login PATH without importing application control variables | `process.platform === 'win32'` | | platform-contract | `apps/desktop/src/main/__tests__/shell-env.test.ts` keeps the inherited PATH and does not log shell stderr when capture fails | `process.platform === 'win32'` | | platform-contract | `apps/desktop/src/main/__tests__/shell-env.test.ts` kills login-shell descendants when capture times out | `process.platform === 'win32'` | @@ -121,6 +122,10 @@ Total Windows-excluded declarations: **105** | platform-contract | `packages/storage/src/__tests__/runtime-policy-stores.test.ts` successor recovery removes credentials orphaned by an interrupted connection removal | `process.platform === 'win32' ? 'POSIX permissions are required to inject a persistence failure' : false` | | platform-contract | `packages/storage/src/__tests__/runtime-policy-stores.test.ts` fails closed on final symlinks, FIFOs, and oversized documents without changing bytes | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/settings-store-onboarding.test.ts` preserves a restrictive umask-derived settings.json mode and leaves no temp file behind | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/settings-store-recovery.test.ts` private backups do not change the normal settings umask policy | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/settings-store-recovery.test.ts` backup ${phase} failure preserves source and reports the original cause | `process.platform === 'win32' && (phase === 'chmod' \|\| phase === 'directory')` | +| portable-candidate | `packages/storage/src/__tests__/settings-store-recovery.test.ts` refuses a preexisting backup ${planted} without deleting it | `process.platform === 'win32' && planted === 'symlink'` | +| portable-candidate | `packages/storage/src/__tests__/settings-store-recovery.test.ts` post-publication failure remains commit-unknown through ${mutation} | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/stable-storage.test.ts` rejects a symlink instead of following it | `process.platform === 'win32' ? 'POSIX no-follow semantics are required' : false` | | portable-candidate | `packages/storage/src/__tests__/stable-storage.test.ts` hardenDirectory creates a 0700 directory chain | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/stable-storage.test.ts` hardenDirectory re-chmods a pre-existing world-accessible directory to 0700 | `process.platform === 'win32'` | diff --git a/packages/storage/src/__tests__/settings-store-onboarding.test.ts b/packages/storage/src/__tests__/settings-store-onboarding.test.ts index 7a99587d7e..d15fbd00be 100644 --- a/packages/storage/src/__tests__/settings-store-onboarding.test.ts +++ b/packages/storage/src/__tests__/settings-store-onboarding.test.ts @@ -330,7 +330,7 @@ describe('SettingsStore.get file recovery', () => { } }); - it('creates defaults only when settings.json is missing', async () => { + it('creates defaults without a backup when settings.json is missing', async () => { const workspaceRoot = await mkdtemp(join(tmpdir(), 'maka-settings-defaults-')); try { const store = createSettingsStore(workspaceRoot); @@ -345,21 +345,6 @@ describe('SettingsStore.get file recovery', () => { } }); - it('rejects corrupt settings.json without overwriting user settings bytes', async () => { - const workspaceRoot = await mkdtemp(join(tmpdir(), 'maka-settings-corrupt-')); - try { - const store = createSettingsStore(workspaceRoot); - const settingsPath = join(workspaceRoot, 'settings.json'); - const corrupt = '{"appearance":{"theme":"dark"}'; - await writeFile(settingsPath, corrupt, 'utf8'); - - await assert.rejects(() => store.get(), SyntaxError); - assert.equal(await readFile(settingsPath, 'utf8'), corrupt); - } finally { - await rm(workspaceRoot, { recursive: true, force: true }); - } - }); - it('preserves a restrictive umask-derived settings.json mode and leaves no temp file behind', { skip: process.platform === 'win32', }, async () => { diff --git a/packages/storage/src/__tests__/settings-store-recovery.test.ts b/packages/storage/src/__tests__/settings-store-recovery.test.ts new file mode 100644 index 0000000000..e6ed242a4b --- /dev/null +++ b/packages/storage/src/__tests__/settings-store-recovery.test.ts @@ -0,0 +1,426 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import assert from 'node:assert/strict'; +import fs, { + chmod, + mkdtemp, + readFile, + readdir, + rm, + stat, + symlink, + writeFile, +} from 'node:fs/promises'; +import { syncBuiltinESMExports } from 'node:module'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { test, type TestContext } from 'node:test'; +import { createDefaultSettings } from '@maka/core/settings'; +import { + createSettingsStore, + SettingsRecoveryError, + SettingsRecoveryCommitUnknownError, + type CorruptSettingsRecovery, + type SettingsStoreOptions, +} from '../settings-store.js'; +import { AtomicFileWriteCommitUnknownError } from '../atomic-file-write.js'; + +const corrupt = Buffer.from('{"appearance":{"theme":"dark"},"secret":"do-not-log"'); +const turn = () => new Promise((resolve) => setImmediate(resolve)); + +async function fixture(t: TestContext, options?: SettingsStoreOptions) { + const root = await mkdtemp(join(tmpdir(), 'maka-settings-recovery-')); + t.after(async () => { + t.mock.restoreAll(); + syncBuiltinESMExports(); + await rm(root, { recursive: true, force: true }); + }); + const path = join(root, 'settings.json'); + await writeFile(path, corrupt); + const events: CorruptSettingsRecovery[] = []; + const store = createSettingsStore( + root, + options ?? { + onCorruptRecovery: (event) => { + events.push(event); + }, + }, + ); + return { root, path, events, store }; +} + +for (const [name, bytes] of [ + ['empty', Buffer.alloc(0)], + ['truncated', corrupt], + ['invalid UTF-8 and truncated', Buffer.concat([corrupt, Buffer.from([0xff, 0xc3])])], +] as const) { + test(`recovers ${name} settings with an exact byte backup before reporting`, async (t) => { + const { root, path, store, events } = await fixture(t); + await writeFile(path, bytes); + assert.deepEqual(await store.get(), createDefaultSettings()); + assert.equal(events.length, 1); + assert.equal(events[0].settingsPath, path); + assert.equal(events[0].outcome, 'recovered'); + assert.match(events[0].backupPath, /settings\.json\.corrupt-\d+-[a-f0-9-]+$/u); + assert.deepEqual(await readFile(events[0].backupPath), bytes); + assert.deepEqual(JSON.parse(await readFile(path, 'utf8')), createDefaultSettings()); + await store.get(); + assert.equal(events.length, 1); + assert.equal((await readdir(root)).length, 2); + }); +} + +test('concurrent reads recover once and later reads observe external changes', async (t) => { + const { path, store, events } = await fixture(t); + const values = await Promise.all(Array.from({ length: 16 }, () => store.get())); + assert.equal(events.length, 1); + for (const value of values) + assert.deepEqual(JSON.parse(JSON.stringify(value)), createDefaultSettings()); + const changed = createDefaultSettings(); + changed.appearance.theme = 'dark'; + await writeFile(path, JSON.stringify(changed)); + assert.equal((await store.get()).appearance.theme, 'dark'); + assert.equal(events.length, 1); +}); + +test('missing and valid JSON keep their normal behavior without a recovery report', async (t) => { + const { path, root, store, events } = await fixture(t); + await rm(path); + await store.get(); + for (const text of ['null', '{}', '{"appearance":{"theme":"dark"}}']) { + await writeFile(path, text); + await store.get(); + assert.equal(await readFile(path, 'utf8'), text); + } + assert.deepEqual(events, []); + assert.deepEqual(await readdir(root), ['settings.json']); +}); + +for (const callback of [ + undefined, + () => { + throw new Error('observer failed'); + }, + async () => { + throw new Error('observer rejected'); + }, +]) { + test('optional or failing observer cannot change a successful recovery', async (t) => { + const { store } = await fixture(t, { onCorruptRecovery: callback }); + assert.deepEqual(await store.get(), createDefaultSettings()); + await turn(); // Any unhandled observer rejection would fail this test. + }); +} + +test('an async observer may reenter the store without deadlocking its queue', { + timeout: 5_000, +}, async (t) => { + const { root } = await fixture(t); + let observed: Promise | undefined; + const store = createSettingsStore(root, { + onCorruptRecovery: () => { + observed = store.get(); + return observed.then(() => {}); + }, + }); + await store.get(); + assert.deepEqual(JSON.parse(JSON.stringify(await observed)), createDefaultSettings()); +}); + +test('mutations use the recovered defaults and execute their own work once', async (t) => { + const { path, store, events } = await fixture(t); + assert.equal((await store.update({ appearance: { theme: 'dark' } })).appearance.theme, 'dark'); + await writeFile(path, corrupt); + let predicates = 0; + let patches = 0; + const result = await store.updateIf( + (current) => { + predicates += 1; + assert.deepEqual(current, createDefaultSettings()); + return true; + }, + () => { + patches += 1; + return { appearance: { theme: 'light' } }; + }, + ); + assert.equal(result.applied, true); + assert.equal(result.settings.appearance.theme, 'light'); + assert.equal(predicates, 1); + assert.equal(patches, 1); + await writeFile(path, corrupt); + assert.equal((await store.upsertOnboardingMilestone('first_chat_sent', 'completed')).length, 1); + await writeFile(path, corrupt); + assert.deepEqual(await store.clearOnboardingMilestone('first_chat_sent'), []); + assert.equal(events.length, 4); + assert.equal(new Set(events.map((event) => event.backupPath)).size, 4); +}); + +test('private backups do not change the normal settings umask policy', { + skip: process.platform === 'win32', +}, async (t) => { + const { path, store, events } = await fixture(t); + await chmod(path, 0o644); + const previous = process.umask(0o027); + try { + await store.get(); + } finally { + process.umask(previous); + } + assert.equal((await stat(events[0].backupPath)).mode & 0o777, 0o600); + assert.equal((await stat(path)).mode & 0o777, 0o640); +}); + +for (const code of ['EACCES', 'EIO']) { + test(`read ${code} does not reset or create backups`, async (t) => { + const { path, root, store, events } = await fixture(t); + const failure = Object.assign(new Error('read failed'), { code }); + const read = fs.readFile; + t.mock.method(fs, 'readFile', async (...args: Parameters) => { + if (args[0] === path) throw failure; + return read(...args); + }); + syncBuiltinESMExports(); + await assert.rejects(store.get(), (error) => error === failure); + assert.deepEqual(await read(path), corrupt); + assert.deepEqual(await readdir(root), ['settings.json']); + assert.deepEqual(events, []); + }); +} + +for (const phase of ['open', 'writeFile', 'chmod', 'sync', 'close', 'directory'] as const) { + test(`backup ${phase} failure preserves source and reports the original cause`, { + skip: process.platform === 'win32' && (phase === 'chmod' || phase === 'directory'), + }, async (t) => { + const { path, root, store, events } = await fixture(t); + const failure = Object.assign(new Error(`backup ${phase} failed`), { code: 'EIO' }); + const originalOpen = fs.open; + t.mock.method(fs, 'open', async (...args: Parameters) => { + const backup = String(args[0]).startsWith(`${path}.corrupt-`); + if (backup && phase === 'open') throw failure; + const handle = await originalOpen(...args); + if (backup && phase !== 'open' && phase !== 'directory') { + t.mock.method( + handle, + phase, + async () => { + throw failure; + }, + { times: 1 }, + ); + } + if (args[0] === root && phase === 'directory') { + t.mock.method(handle, 'sync', async () => { + throw failure; + }); + } + return handle; + }); + syncBuiltinESMExports(); + await assert.rejects(store.get(), (error) => { + assert.ok(error instanceof SettingsRecoveryError); + assert.equal(error.phase, 'backup'); + assert.equal(error.cause, failure); + assert.equal(error.settingsPath, path); + assert.equal(error.backupPath, undefined); + assert.equal(error.incompleteBackupPath, undefined); + assert.equal(error.message.includes('do-not-log'), false); + return true; + }); + assert.deepEqual(await readFile(path), corrupt); + assert.deepEqual(await readdir(root), ['settings.json']); + assert.deepEqual(events, []); + }); +} + +test('backup cleanup failure retains the original cause and identifies an incomplete file', async (t) => { + const { path, store } = await fixture(t); + const failure = new Error('backup write failed'); + const originalOpen = fs.open; + t.mock.method(fs, 'open', async (...args: Parameters) => { + const handle = await originalOpen(...args); + if (String(args[0]).startsWith(`${path}.corrupt-`)) { + t.mock.method(handle, 'writeFile', async () => { + throw failure; + }); + } + return handle; + }); + t.mock.method(fs, 'rm', async () => { + throw new Error('cleanup failed'); + }); + syncBuiltinESMExports(); + await assert.rejects(store.get(), (error) => { + assert.ok(error instanceof SettingsRecoveryError); + assert.equal(error.cause, failure); + assert.equal(error.backupPath, undefined); + assert.ok(error.incompleteBackupPath?.startsWith(`${path}.corrupt-`)); + return true; + }); + assert.deepEqual(await readFile(path), corrupt); +}); + +for (const planted of ['file', 'symlink'] as const) { + test(`refuses a preexisting backup ${planted} without deleting it`, { + skip: process.platform === 'win32' && planted === 'symlink', + }, async (t) => { + const { path, root, store } = await fixture(t); + const target = join(root, 'unrelated'); + await writeFile(target, 'keep me'); + const originalOpen = fs.open; + let collision = ''; + t.mock.method(fs, 'open', async (...args: Parameters) => { + if (String(args[0]).startsWith(`${path}.corrupt-`)) { + collision = String(args[0]); + if (planted === 'file') await writeFile(collision, 'keep me'); + else await symlink(target, collision); + } + return originalOpen(...args); + }); + syncBuiltinESMExports(); + await assert.rejects( + store.get(), + (error) => + error instanceof SettingsRecoveryError && + (error.cause as NodeJS.ErrnoException).code === 'EEXIST', + ); + assert.equal(await readFile(collision, 'utf8'), 'keep me'); + assert.equal(await readFile(target, 'utf8'), 'keep me'); + assert.deepEqual(await readFile(path), corrupt); + }); +} + +test('reset publication failure retains the complete backup and never reports success', async (t) => { + const { path, root, store, events } = await fixture(t); + const failure = Object.assign(new Error('rename failed'), { code: 'EACCES' }); + const rename = fs.rename; + t.mock.method(fs, 'rename', async (...args: Parameters) => { + if (args[1] === path) throw failure; + return rename(...args); + }); + syncBuiltinESMExports(); + let backupPath = ''; + await assert.rejects(store.get(), (error) => { + assert.ok(error instanceof SettingsRecoveryError); + assert.equal(error.phase, 'reset'); + assert.equal(error.cause, failure); + assert.ok(error.backupPath); + backupPath = error.backupPath; + return true; + }); + assert.deepEqual(await readFile(backupPath), corrupt); + assert.deepEqual(await readFile(path), corrupt); + assert.equal((await readdir(root)).length, 2); + assert.deepEqual(events, []); +}); + +for (const code of ['ENOENT', 'syntax']) { + test(`a migration write failure (${code}) cannot trigger creation or recovery`, async (t) => { + const { path, root, store, events } = await fixture(t); + const text = '{"network":{"proxy":{"password":"legacy"}},"appearance":{"theme":"dark"}}'; + await writeFile(path, text); + const failure = + code === 'syntax' + ? new SyntaxError('migration failed') + : Object.assign(new Error('migration failed'), { code }); + t.mock.method(fs, 'rename', async () => { + throw failure; + }); + syncBuiltinESMExports(); + await assert.rejects(store.get(), (error) => error === failure); + assert.equal(await readFile(path, 'utf8'), text); + assert.deepEqual(await readdir(root), ['settings.json']); + assert.deepEqual(events, []); + }); +} + +test('normalization errors are not interpreted as invalid JSON', async (t) => { + const { path, store, events } = await fixture(t); + await writeFile(path, '{}'); + const failure = new SyntaxError('normalizer failed'); + t.mock.method(JSON, 'parse', () => + Object.defineProperty({}, 'network', { + get() { + throw failure; + }, + }), + ); + await assert.rejects(store.get(), (error) => error === failure); + assert.deepEqual(events, []); + assert.equal(await readFile(path, 'utf8'), '{}'); +}); + +for (const mutation of ['get', 'update', 'updateIf', 'milestone'] as const) { + test(`post-publication failure remains commit-unknown through ${mutation}`, { + skip: process.platform === 'win32', + }, async (t) => { + const { root, path, store, events } = await fixture(t); + const failure = Object.assign(new Error('reset directory sync failed'), { code: 'EIO' }); + const originalOpen = fs.open; + let directories = 0; + let syncs = 0; + t.mock.method(fs, 'open', async (...args: Parameters) => { + const handle = await originalOpen(...args); + if (args[0] === root && ++directories === 2) { + t.mock.method(handle, 'sync', async () => { + syncs += 1; + throw failure; + }); + } + return handle; + }); + syncBuiltinESMExports(); + let predicateCalls = 0; + const operation = + mutation === 'get' + ? store.get() + : mutation === 'update' + ? store.update({ appearance: { theme: 'dark' } }) + : mutation === 'milestone' + ? store.upsertOnboardingMilestone('first_chat_sent', 'completed') + : store.updateIf( + () => { + predicateCalls += 1; + return true; + }, + { appearance: { theme: 'dark' } }, + ); + await assert.rejects(operation, (error) => { + assert.ok(error instanceof SettingsRecoveryCommitUnknownError); + assert.ok(error instanceof AtomicFileWriteCommitUnknownError); + assert.equal(error.published, true); + assert.ok(error.cause instanceof AtomicFileWriteCommitUnknownError); + assert.equal(error.cause.cause, failure); + assert.equal(error.settingsPath, path); + assert.equal(error.backupPath, events[0]?.backupPath); + assert.equal(error.message.includes('do-not-log'), false); + return true; + }); + assert.equal(syncs, 1); + assert.equal(directories, 2); + assert.equal(predicateCalls, 0); + assert.equal(events.length, 1); + assert.equal(events[0].outcome, 'commit-unknown'); + assert.deepEqual(await readFile(events[0].backupPath), corrupt); + assert.deepEqual(JSON.parse(await readFile(path, 'utf8')), createDefaultSettings()); + assert.deepEqual(JSON.parse(JSON.stringify(await store.get())), createDefaultSettings()); + assert.equal(events.length, 1); + }); +} diff --git a/packages/storage/src/settings-store.ts b/packages/storage/src/settings-store.ts index 94f549e310..90f0e4aa29 100644 --- a/packages/storage/src/settings-store.ts +++ b/packages/storage/src/settings-store.ts @@ -17,13 +17,76 @@ * under the License. */ -import { mkdir, readFile } from 'node:fs/promises'; +import { randomUUID } from 'node:crypto'; +import { mkdir, open, readFile, rm } from 'node:fs/promises'; import { dirname, join } from 'node:path'; import type { AppSettings, UpdateAppSettingsInput } from '@maka/core/settings'; import type { OnboardingMilestone, OnboardingMilestoneId } from '@maka/core/onboarding'; import { createDefaultSettings, mergeSettings, normalizeSettings } from '@maka/core/settings'; import { sanitizeOnboardingMilestones } from '@maka/core/onboarding'; -import { writeAtomicFile } from './atomic-file-write.js'; +import { AtomicFileWriteCommitUnknownError, writeAtomicFile } from './atomic-file-write.js'; +import { syncDirectory } from './stable-storage.js'; + +export interface CorruptSettingsRecovery { + readonly settingsPath: string; + readonly backupPath: string; + readonly outcome: 'recovered' | 'commit-unknown'; +} + +export interface SettingsStoreOptions { + /** Observes publication, not delivery of a notification. Never awaited while + * holding the store queue; callback failures cannot change the write result. */ + onCorruptRecovery?: (recovery: CorruptSettingsRecovery) => void | Promise; +} + +export class SettingsRecoveryError extends Error { + readonly settingsPath: string; + readonly phase: 'backup' | 'reset'; + /** Present only when a complete backup passed its synchronization steps. */ + readonly backupPath?: string; + /** A file this attempt created but could not finish or remove. */ + readonly incompleteBackupPath?: string; + + constructor(options: { + settingsPath: string; + phase: 'backup' | 'reset'; + backupPath?: string; + incompleteBackupPath?: string; + cause: unknown; + }) { + const detail = options.backupPath + ? `Original bytes are backed up at ${options.backupPath}.` + : 'No complete backup was confirmed.'; + super( + `Cannot recover invalid JSON settings at ${options.settingsPath}: ${options.phase} failed. ` + + `The original settings file was not replaced. ${detail}` + + (options.incompleteBackupPath + ? ` An incomplete backup may remain at ${options.incompleteBackupPath}.` + : '') + + ' Close the app and check file access and disk space before retrying.', + { cause: options.cause }, + ); + this.name = 'SettingsRecoveryError'; + this.settingsPath = options.settingsPath; + this.phase = options.phase; + this.backupPath = options.backupPath; + this.incompleteBackupPath = options.incompleteBackupPath; + } +} + +export class SettingsRecoveryCommitUnknownError extends AtomicFileWriteCommitUnknownError { + constructor( + readonly settingsPath: string, + readonly backupPath: string, + cause: AtomicFileWriteCommitUnknownError, + ) { + super({ cause }); + this.name = 'SettingsRecoveryCommitUnknownError'; + this.message = + `Default settings were published at ${settingsPath}, but durability is unconfirmed. ` + + `Original bytes are backed up at ${backupPath}. Reload before retrying; do not replay the update automatically.`; + } +} /** * A conditional write's patch, either fixed or derived from the state the @@ -68,15 +131,21 @@ export interface SettingsStore { clearOnboardingMilestone(id: OnboardingMilestoneId): Promise; } -export function createSettingsStore(workspaceRoot: string): SettingsStore { - return new FileSettingsStore(workspaceRoot); +export function createSettingsStore( + workspaceRoot: string, + options: SettingsStoreOptions = {}, +): SettingsStore { + return new FileSettingsStore(workspaceRoot, options); } class FileSettingsStore implements SettingsStore { private readonly settingsPath: string; private queue: Promise = Promise.resolve(); - constructor(workspaceRoot: string) { + constructor( + workspaceRoot: string, + private readonly options: SettingsStoreOptions, + ) { this.settingsPath = join(workspaceRoot, 'settings.json'); } @@ -90,20 +159,100 @@ class FileSettingsStore implements SettingsStore { } private async readOrCreate(): Promise { + let bytes: Buffer; try { - const text = await readFile(this.settingsPath, 'utf8'); - const persisted: unknown = JSON.parse(text); - const settings = normalizeSettings(persisted); - if (hasLegacyProxyCredentialFields(persisted)) { - await this.write(settings); - } - return settings; + bytes = await readFile(this.settingsPath); } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; const settings = createDefaultSettings(); await this.write(settings); return settings; } + + let persisted: unknown; + try { + persisted = JSON.parse(bytes.toString('utf8')); + } catch (error) { + if (!(error instanceof SyntaxError)) throw error; + // Never attach the parse error: its message can quote stored secrets. + return this.recoverCorruptSettings(bytes); + } + const settings = normalizeSettings(persisted); + if (hasLegacyProxyCredentialFields(persisted)) { + await this.write(settings); + } + return settings; + } + + private async recoverCorruptSettings(bytes: Buffer): Promise { + const backupPath = await this.backupCorruptSettings(bytes); + const settings = createDefaultSettings(); + try { + await this.write(settings); + } catch (error) { + if (error instanceof AtomicFileWriteCommitUnknownError) { + this.reportRecovery(backupPath, 'commit-unknown'); + throw new SettingsRecoveryCommitUnknownError(this.settingsPath, backupPath, error); + } + throw new SettingsRecoveryError({ + settingsPath: this.settingsPath, + phase: 'reset', + backupPath, + cause: error, + }); + } + this.reportRecovery(backupPath, 'recovered'); + return settings; + } + + /** An exclusive byte-for-byte backup, completed before replacing settings. + * Keeping the source in place avoids turning a failed recovery into ENOENT. + * This has the same platform fsync limits as the shared atomic writer. */ + private async backupCorruptSettings(bytes: Buffer): Promise { + const backupPath = `${this.settingsPath}.corrupt-${Date.now()}-${randomUUID()}`; + let created = false; + try { + const handle = await open(backupPath, 'wx', 0o600); + created = true; + try { + await handle.writeFile(bytes); + if (process.platform !== 'win32') await handle.chmod(0o600); + await handle.sync(); + await handle.close(); + } catch (error) { + await handle.close().catch(() => {}); + throw error; + } + await syncDirectory(dirname(this.settingsPath)); + return backupPath; + } catch (error) { + let incompleteBackupPath: string | undefined; + if (created) { + await rm(backupPath, { force: true }).catch(() => { + incompleteBackupPath = backupPath; + }); + } + throw new SettingsRecoveryError({ + settingsPath: this.settingsPath, + phase: 'backup', + incompleteBackupPath, + cause: error, + }); + } + } + + private reportRecovery(backupPath: string, outcome: CorruptSettingsRecovery['outcome']): void { + try { + void Promise.resolve( + this.options.onCorruptRecovery?.({ + settingsPath: this.settingsPath, + backupPath, + outcome, + }), + ).catch(() => {}); + } catch { + // Notification failure must not undo or misclassify a published reset. + } } async update(patch: UpdateAppSettingsInput): Promise { From c4e8d7b1f0d3d33a800d5aee34640699bb94a7f4 Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Mon, 14 Sep 2026 14:30:30 +0800 Subject: [PATCH 2/8] fix(mcp): surface repair guidance for corrupt config files Return a typed invalid-JSON error naming the persisted file while preserving its bytes and omitting parser messages that may contain credentials. Show localized repair guidance in Desktop and TUI, including mutations after a successful TUI startup. Keep live MCP state intact and make error details scrollable in small terminals, with Escape returning to the server list. Cover read and mutation refusal, localized rendering, scrolling, existing connections and explicit operations after an external file repair. Refs #4285 Generated-by: OpenAI Codex --- .../__tests__/mcp-ipc-commit-unknown.test.ts | 34 ++++- .../src/main/__tests__/mcp-page-model.test.ts | 10 +- .../module-hub/model/mcp-page-model.ts | 8 +- .../renderer/features/module-hub/testing.ts | 2 +- .../features/module-hub/ui/mcp-page.tsx | 4 +- apps/desktop/src/renderer/locales/mcp-copy.ts | 5 +- .../src/__tests__/pi-tui-mcp-status.test.ts | 120 ++++++++++++++++++ .../src/__tests__/tui-copy-catalog.test.ts | 1 + .../cli/src/__tests__/tui-mcp-control.test.ts | 111 +++++++++++++++- packages/cli/src/pi-tui-mcp-status.ts | 63 ++++++++- packages/cli/src/tui-copy-catalog.ts | 9 ++ packages/cli/src/tui-mcp-control.ts | 25 +++- .../src/__tests__/mcp-config-store.test.ts | 42 ++++++ packages/storage/src/mcp-config-store.ts | 18 ++- 14 files changed, 427 insertions(+), 25 deletions(-) diff --git a/apps/desktop/src/main/__tests__/mcp-ipc-commit-unknown.test.ts b/apps/desktop/src/main/__tests__/mcp-ipc-commit-unknown.test.ts index 4844a17d51..102171a1cd 100644 --- a/apps/desktop/src/main/__tests__/mcp-ipc-commit-unknown.test.ts +++ b/apps/desktop/src/main/__tests__/mcp-ipc-commit-unknown.test.ts @@ -18,7 +18,7 @@ */ import assert from 'node:assert/strict'; -import fs, { mkdtemp, readFile, rm } from 'node:fs/promises'; +import fs, { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; import { syncBuiltinESMExports } from 'node:module'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -29,11 +29,12 @@ import { McpClientManager } from '@maka/mcp'; import { AtomicFileWriteCommitUnknownError, createMcpConfigStore, + McpConfigSourceError, type McpConfigStore, } from '@maka/storage/mcp-config-store'; import { registerMcpIpcMain, type McpIpcMainDeps } from '../mcp-ipc-main.js'; import { getMcpCopy } from '../../renderer/locales/mcp-copy.js'; -import { mcpWriteFailureMessage } from '../../renderer/features/module-hub/testing.js'; +import { mcpConfigFailureMessage } from '../../renderer/features/module-hub/testing.js'; test('MCP remove reconciles a live manager after the real store publishes then fails directory sync', { skip: process.platform === 'win32', @@ -66,7 +67,7 @@ test('MCP remove reconciles a live manager after the real store publishes then f assert.ok(error instanceof AtomicFileWriteCommitUnknownError); assert.equal(error.published, true); assert.equal(error.cause, fault.error); - assert.equal(mcpWriteFailureMessage(error, getMcpCopy('en')), getMcpCopy('en').errors.writeDurabilityUnknown); + assert.equal(mcpConfigFailureMessage(error, getMcpCopy('en')), getMcpCopy('en').errors.writeDurabilityUnknown); return true; }); assert.deepEqual(await diskConfig(root), { version: MCP_CONFIG_VERSION, mcpServers: {} }); @@ -126,7 +127,7 @@ for (const phase of ['read', 'sync', 'emit'] as const) { assert.match(error.message, /out of sync/u); assert.equal(error.cause, tracked.error()); assert.deepEqual(error.errors, [tracked.error(), reconciliationError]); - assert.equal(mcpWriteFailureMessage(error, getMcpCopy('en')), getMcpCopy('en').errors.writeOutOfSync); + assert.equal(mcpConfigFailureMessage(error, getMcpCopy('en')), getMcpCopy('en').errors.writeOutOfSync); return true; }); assert.ok((await diskConfig(root)).mcpServers.fixture); @@ -243,3 +244,28 @@ function mutationHarness(t: TestContext, store: McpConfigStore, overrides: Parti }, }; } + +test('MCP IPC preserves repair guidance for a corrupt persisted file and does not import over it', async (t) => { + const { root, store } = await fixtureStore(t); + const path = join(root, 'mcp.json'); + const source = '{"secret":"must-not-appear"'; + await writeFile(path, source); + const ipc = mutationHarness(store); + for (const call of [() => ipc.invoke('mcp:getConfig'), () => ipc.invoke('mcp:importConfig', '{"new":{"command":"unused"}}')]) { + await assert.rejects(call(), (error) => { + assert.ok(error instanceof McpConfigSourceError); + assert.equal(error.path, path); + assert.ok(error.message.includes(path)); + assert.match(error.message, /back up and repair/u); + for (const locale of ['en', 'zh-CN', 'zh-TW'] as const) { + const copy = getMcpCopy(locale); + const ipcError: Error = new Error(`Error invoking remote method 'mcp:getConfig': ${error.name}: ${error.message}`); + assert.equal(mcpConfigFailureMessage(ipcError, copy), copy.errors.invalidConfigFile(path)); + } + assert.equal(error.message.includes('must-not-appear'), false); + return true; + }); + } + assert.deepEqual(ipc.synced, []); + assert.equal(await readFile(path, 'utf8'), source); +}); diff --git a/apps/desktop/src/main/__tests__/mcp-page-model.test.ts b/apps/desktop/src/main/__tests__/mcp-page-model.test.ts index c2ae3c5d45..c225165df1 100644 --- a/apps/desktop/src/main/__tests__/mcp-page-model.test.ts +++ b/apps/desktop/src/main/__tests__/mcp-page-model.test.ts @@ -27,7 +27,7 @@ import { mcpConfigFromDraft, mcpDraftProtocolPreference, mcpDraftFromConfig, - mcpWriteFailureMessage, + mcpConfigFailureMessage, } from '../../renderer/features/module-hub/testing.js'; const copy = getMcpCopy('en'); @@ -41,14 +41,14 @@ test('MCP write errors retain actionable localized meaning across Electron seria [durabilityError.message, localized.errors.writeDurabilityUnknown], [outOfSync, localized.errors.writeOutOfSync], ]) { - assert.equal(mcpWriteFailureMessage(message, localized), expected); + assert.equal(mcpConfigFailureMessage(message, localized), expected); assert.equal( - mcpWriteFailureMessage(new Error(`Error invoking remote method 'mcp:remove': Error: ${message}`), localized), + mcpConfigFailureMessage(new Error(`Error invoking remote method 'mcp:remove': Error: ${message}`), localized), expected, ); } - assert.equal(mcpWriteFailureMessage(new Error('unrelated private details'), localized), undefined); - assert.equal(mcpWriteFailureMessage(undefined, localized), undefined); + assert.equal(mcpConfigFailureMessage(new Error('unrelated private details'), localized), undefined); + assert.equal(mcpConfigFailureMessage(undefined, localized), undefined); } }); diff --git a/apps/desktop/src/renderer/features/module-hub/model/mcp-page-model.ts b/apps/desktop/src/renderer/features/module-hub/model/mcp-page-model.ts index e62284aaf0..faffec5b5f 100644 --- a/apps/desktop/src/renderer/features/module-hub/model/mcp-page-model.ts +++ b/apps/desktop/src/renderer/features/module-hub/model/mcp-page-model.ts @@ -27,9 +27,13 @@ import type { McpCopy } from '../../../locales/mcp-copy.js'; import { formatCommandLine, parseCommandLine } from './mcp-command-line.js'; /** Electron preserves error messages, but not custom error fields. Map only - * the fixed publication-error messages to safe, localized presentation. */ -export function mcpWriteFailureMessage(error: unknown, copy: McpCopy): string | undefined { + * the fixed config-error messages to safe, localized presentation. */ +export function mcpConfigFailureMessage(error: unknown, copy: McpCopy): string | undefined { const message = error instanceof Error ? error.message : typeof error === 'string' ? error : ''; + const invalidFile = /MCP config at ([^\r\n]+) contains invalid JSON\. The file was not modified\. Close the app, back up and repair this file before retrying\.$/u.exec(message); + if (invalidFile) { + return copy.errors.invalidConfigFile(invalidFile[1].replace(/[\u0000-\u001f\u007f-\u009f]/gu, '')); + } if (message.includes('MCP write durability is uncertain and runtime state is out of sync')) { return copy.errors.writeOutOfSync; } diff --git a/apps/desktop/src/renderer/features/module-hub/testing.ts b/apps/desktop/src/renderer/features/module-hub/testing.ts index dd1cd69b60..9ebd412e15 100644 --- a/apps/desktop/src/renderer/features/module-hub/testing.ts +++ b/apps/desktop/src/renderer/features/module-hub/testing.ts @@ -39,7 +39,7 @@ export { mcpConfigFromDraft, mcpDraftProtocolPreference, mcpDraftFromConfig, - mcpWriteFailureMessage, + mcpConfigFailureMessage, } from "./model/mcp-page-model.js"; export { useModuleHubController } from "./controller/use-module-hub-controller.js"; export { diff --git a/apps/desktop/src/renderer/features/module-hub/ui/mcp-page.tsx b/apps/desktop/src/renderer/features/module-hub/ui/mcp-page.tsx index e94f1c0218..b8673f32bc 100644 --- a/apps/desktop/src/renderer/features/module-hub/ui/mcp-page.tsx +++ b/apps/desktop/src/renderer/features/module-hub/ui/mcp-page.tsx @@ -81,7 +81,7 @@ import { mcpConfigFromDraft, mcpDraftProtocolPreference, mcpDraftFromConfig, - mcpWriteFailureMessage, + mcpConfigFailureMessage, type McpEditorDraft, } from '../model/mcp-page-model.js'; import { classifiedErrorFallback } from '../../../application/contracts/operation-diagnostics.js'; @@ -137,7 +137,7 @@ export function McpPage(props: { hubHeader?: ModuleHubHeader }) { const mounted = useMountedRef(); const toast = useToast(); useEffect(() => { - if (error) toast.error(copy.errors.update, mcpWriteFailureMessage(error, copy) ?? classifiedErrorFallback(error, getSettingsSharedCopy(locale).unknownError, locale, 'mcp'), undefined, defaultRuntimeHostDiagnosticTarget(error)); + if (error) toast.error(copy.errors.update, mcpConfigFailureMessage(error, copy) ?? classifiedErrorFallback(error, getSettingsSharedCopy(locale).unknownError, locale, 'mcp'), undefined, defaultRuntimeHostDiagnosticTarget(error)); }, [error, locale, copy, toast]); // Set when a remove starts, consumed once the row has actually left the // list — which only happens when the config write lands. diff --git a/apps/desktop/src/renderer/locales/mcp-copy.ts b/apps/desktop/src/renderer/locales/mcp-copy.ts index 144102239c..d8e525777f 100644 --- a/apps/desktop/src/renderer/locales/mcp-copy.ts +++ b/apps/desktop/src/renderer/locales/mcp-copy.ts @@ -24,7 +24,7 @@ export type McpCopy = { load: string; save: string; import: string; update: string; test: string; remove: string; unavailableStatus: string; mapLine(line: number): string; importJson: string; importObject: string; importVersion(version: string): string; importServersObject: string; importProtocolVersion: string; - writeDurabilityUnknown: string; writeOutOfSync: string; + writeDurabilityUnknown: string; invalidConfigFile: (path: string) => string; writeOutOfSync: string; }; toast: { saved: string; savedDetail: string; @@ -67,6 +67,7 @@ const MCP_COPY = { 'zh-CN': { errors: { load: '载入 MCP 失败', save: '保存 MCP 失败', + invalidConfigFile: (path) => `${path} 中的 JSON 无效，文件未被修改。请关闭应用，备份并修复此文件后重试。`, writeDurabilityUnknown: '写入已发布，但无法确认断电后是否保留。请检查刷新后的配置再决定是否重试。', writeOutOfSync: '写入的持久性尚未确认，MCP 运行状态也未能与配置同步。请检查配置并重新同步后再重试。', import: '导入 MCP 失败', update: '更新 MCP 失败', test: 'MCP 测试失败', remove: '删除 MCP 失败', unavailableStatus: 'Server 没有返回可用状态。', @@ -128,6 +129,7 @@ const MCP_COPY = { 'zh-TW': { errors: { load: '載入 MCP 失敗', save: '儲存 MCP 失敗', + invalidConfigFile: (path) => `${path} 中的 JSON 無效，檔案未被修改。請關閉應用程式，備份並修復此檔案後重試。`, writeDurabilityUnknown: '寫入已發布，但無法確認斷電後是否保留。請檢查重新整理後的設定再決定是否重試。', writeOutOfSync: '寫入的持久性尚未確認，MCP 執行狀態也未能與設定同步。請檢查設定並重新同步後再重試。', import: '匯入 MCP 失敗', update: '更新 MCP 失敗', test: 'MCP 測試失敗', remove: '刪除 MCP 失敗', unavailableStatus: 'Server 沒有返回可用狀態。', @@ -189,6 +191,7 @@ const MCP_COPY = { en: { errors: { load: 'Failed to load MCP', save: 'Failed to save MCP', + invalidConfigFile: (path) => `Invalid JSON in ${path}. The file is unchanged. Close the app, back up and repair this file before retrying.`, writeDurabilityUnknown: 'The write was published, but survival after power loss could not be confirmed. Check the refreshed configuration before retrying.', writeOutOfSync: 'Write durability could not be confirmed, and MCP runtime state is out of sync with the configuration. Check the configuration and resynchronize before retrying.', import: 'Failed to import MCP', update: 'Failed to update MCP', test: 'MCP test failed', remove: 'Failed to delete MCP', unavailableStatus: 'The server did not return an available status.', diff --git a/packages/cli/src/__tests__/pi-tui-mcp-status.test.ts b/packages/cli/src/__tests__/pi-tui-mcp-status.test.ts index 73b8936200..54a78d70b0 100644 --- a/packages/cli/src/__tests__/pi-tui-mcp-status.test.ts +++ b/packages/cli/src/__tests__/pi-tui-mcp-status.test.ts @@ -553,3 +553,123 @@ function surface( execute: async () => ({ status: 'failed', reason: 'manager-failed' }), }; } + +test('invalid persisted MCP JSON renders its location and repair guidance in every locale', () => { + for (const locale of ['en', 'zh-CN', 'zh-TW'] as const) { + const overlay = new McpManagementOverlay({ + locale, + surface: surface({ + initialization: 'error', + invalidConfigPath: '/profile/mcp.json', + configuration: 'synchronizing', + publication: 'not_published', + toolCount: 0, + servers: [], + }), + viewportRows: () => 20, + onClose: () => {}, + onChange: () => {}, + }); + const text = overlay.render(160).map(stripAnsi).join('\n'); + assert.match(text, /\/profile\/mcp\.json/u); + assert.match(text, /back up and repair|备份并修复|備份並修復/u); + assert.match(text, /unchanged|未被修改/u); + } +}); + +for (const locale of ['en', 'zh-CN', 'zh-TW'] as const) { + test(`runtime MCP file errors show ${locale} repair guidance and return to the live server list`, async () => { + const snapshot = listSnapshot(); + const mcp = surface(snapshot); + mcp.execute = async () => ({ + status: 'failed', + reason: 'invalid-config-file', + path: '/profile/\u0000mcp.json', + }); + let closed = false; + const overlay = new McpManagementOverlay({ + locale, + surface: mcp, + viewportRows: () => 20, + onClose: () => { + closed = true; + }, + onChange: () => {}, + }); + const render = () => overlay.render(160).map(stripAnsi).join('\n'); + render(); + overlay.handleInput(' '); + await new Promise((resolve) => setImmediate(resolve)); + const text = render(); + assert.ok(text.includes('/profile/mcp.json')); + assert.match(text, /back up and repair|备份并修复|備份並修復/u); + assert.ok(text.includes(TUI_COPY_RESOURCES['mcp-status'][locale].footer.diagnostic)); + assert.doesNotMatch(text, /\u0000/u); + assert.equal(mcp.snapshot().initialization, 'ready'); + overlay.handleInput('\u001b'); + assert.equal(closed, false); + assert.ok(render().includes('filesystem')); + assert.equal(render().includes('/profile/mcp.json'), false); + mcp.execute = async () => ({ status: 'applied', effect: 'published' }); + overlay.handleInput(' '); + await new Promise((resolve) => setImmediate(resolve)); + assert.ok(render().includes(TUI_COPY_RESOURCES['mcp-status'][locale].editor.results.published)); + }); +} + +for (const phase of ['initialization', 'mutation'] as const) { + test(`MCP ${phase} repair details scroll in a small terminal and clamp after resizing`, async () => { + const path = '/Users/example/Library/Application Support/Maka/workspaces/default/mcp.json'; + const mcp = surface( + phase === 'mutation' + ? listSnapshot() + : { + initialization: 'error', + configuration: 'synchronizing', + publication: 'not_published', + invalidConfigPath: path, + toolCount: 0, + servers: [], + }, + ); + mcp.execute = async () => ({ status: 'failed', reason: 'invalid-config-file', path }); + let rows = 4; + const overlay = new McpManagementOverlay({ + locale: 'en', + surface: mcp, + viewportRows: () => rows, + onClose: () => {}, + onChange: () => {}, + }); + const render = () => overlay.render(50).map(stripAnsi).join('\n'); + render(); + if (phase === 'mutation') { + overlay.handleInput(' '); + await new Promise((resolve) => setImmediate(resolve)); + } + const first = render(); + assert.equal(first.includes('retrying.'), false); + overlay.handleInput('\u001b[B'); + assert.notEqual(render(), first); + overlay.handleInput('\u001b[A'); + assert.equal(render(), first); + overlay.handleInput('\u001b[6~'); + assert.notEqual(render(), first); + overlay.handleInput('\u001b[5~'); + assert.equal(render(), first); + overlay.handleInput('\u001b[F'); + const last = render(); + assert.ok(last.includes('retrying.')); + overlay.handleInput('\u001b[6~'); + assert.equal(render(), last); + overlay.handleInput('\u001b[H'); + assert.equal(render(), first); + overlay.handleInput('\u001b[F'); + render(); + rows = 30; + const expanded = render(); + assert.match(expanded, /1-\d+ \/ \d+/u); + assert.ok(expanded.includes('mcp.json')); + assert.ok(expanded.includes('retrying.')); + }); +} diff --git a/packages/cli/src/__tests__/tui-copy-catalog.test.ts b/packages/cli/src/__tests__/tui-copy-catalog.test.ts index 7cc2b7a0ba..a328810b39 100644 --- a/packages/cli/src/__tests__/tui-copy-catalog.test.ts +++ b/packages/cli/src/__tests__/tui-copy-catalog.test.ts @@ -37,6 +37,7 @@ const MESSAGE_VALUES = { state: 'ready', count: 2, detail: 'HTTP 401', + path: '/profile/mcp.json', hasDetail: true, bytes: 40_000, serverId: 'filesystem', diff --git a/packages/cli/src/__tests__/tui-mcp-control.test.ts b/packages/cli/src/__tests__/tui-mcp-control.test.ts index 9d423db4c6..d7c1e6fcf4 100644 --- a/packages/cli/src/__tests__/tui-mcp-control.test.ts +++ b/packages/cli/src/__tests__/tui-mcp-control.test.ts @@ -19,7 +19,7 @@ import { deferred } from '@maka/core/test-only/async-primitives'; import assert from 'node:assert/strict'; -import fs, { mkdtemp, readFile, rm } from 'node:fs/promises'; +import fs, { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; import { syncBuiltinESMExports } from 'node:module'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; @@ -34,7 +34,11 @@ import { AtomicFileWriteCommitUnknownError, createMcpConfigStore, } from '@maka/storage/mcp-config-store'; -import { createTuiMcpController, type TuiMcpPublicationAvailability } from '../tui-mcp-control.js'; +import { + createTuiMcpController, + type TuiMcpAction, + type TuiMcpPublicationAvailability, +} from '../tui-mcp-control.js'; import { waitFor } from './tui-terminal-mock.js'; test('TUI MCP startup stays backgrounded and publishes the discovered snapshot', async () => { @@ -1411,3 +1415,106 @@ function deferredValue() { }); return { promise, resolve }; } + +test('TUI MCP retains only the invalid persisted config path for repair guidance', async () => { + const root = await mkdtemp(join(tmpdir(), 'tui-mcp-invalid-json-')); + const manager = managerHarness(0, []); + const connection = connectionHarness(); + const path = join(root, 'mcp.json'); + const bytes = '{"secret":"do-not-display-this"'; + await writeFile(path, bytes); + const controller = createTuiMcpController( + { workspaceRoot: root, connection: connection.connection }, + { + configStore: createMcpConfigStore(root), + manager: manager.manager, + createProvider: () => provider('unused'), + }, + ); + try { + await waitFor( + () => controller.snapshot().initialization === 'error', + 'invalid MCP file to fail initialization', + ); + assert.equal(controller.snapshot().invalidConfigPath, path); + assert.equal(JSON.stringify(controller.snapshot()).includes('do-not-display-this'), false); + assert.equal(connection.replacements.length, 0); + assert.equal(await readFile(path, 'utf8'), bytes); + } finally { + await controller.close(); + await rm(root, { recursive: true, force: true }); + } +}); + +for (const kind of ['add', 'edit', 'set_enabled', 'remove', 'commit_import'] as const) { + test(`TUI MCP ${kind} retains a corrupt file diagnostic without disturbing live state`, async (t) => { + const root = await mkdtemp(join(tmpdir(), 'tui-mcp-runtime-corrupt-')); + const path = join(root, 'mcp.json'); + const store = createMcpConfigStore(root); + const initial: McpConfigFile = { + version: 3, + mcpServers: { docs: { enabled: false, url: 'https://docs.example/mcp' } }, + }; + await store.transform(() => initial); + const order: string[] = []; + const connection = connectionHarness(); + const controller = createTuiMcpController( + { workspaceRoot: root, connection: connection.connection }, + { + configStore: store, + manager: managementManager(order).manager, + createProvider: () => undefined, + }, + ); + t.after(async () => { + await controller.close(); + await rm(root, { recursive: true, force: true }); + }); + await waitFor( + () => + controller.snapshot().initialization === 'ready' && + controller.snapshot().publication === 'not_published', + 'MCP initialization without a capability provider', + ); + const edit = controller.configForEdit('docs'); + assert.ok(edit); + const preview = controller.previewImport('{"new":{"command":"unused"}}'); + assert.equal(preview.status, 'ready'); + const actions: Record = { + add: { kind: 'add', serverId: 'new', config: { command: 'unused' } }, + edit: { + kind: 'edit', + serverId: 'docs', + expectedRevision: edit.revision, + config: { command: 'unused' }, + }, + set_enabled: { kind: 'set_enabled', serverId: 'docs', enabled: true }, + remove: { kind: 'remove', serverId: 'docs' }, + commit_import: { kind: 'commit_import', previewId: preview.preview.previewId }, + }; + const before = controller.snapshot(); + order.length = 0; + const bytes = '{"secret":"never-display-this"'; + await writeFile(path, bytes); + const result = await controller.execute(actions[kind]); + assert.deepEqual(result, { status: 'failed', reason: 'invalid-config-file', path }); + assert.deepEqual(controller.snapshot(), before); + assert.deepEqual(controller.configForEdit('docs'), edit); + assert.deepEqual(order, []); + assert.equal(connection.unregisters, 0); + assert.equal(await readFile(path, 'utf8'), bytes); + assert.equal(JSON.stringify(result).includes('never-display-this'), false); + + // An external repair makes the next explicit operation usable without a + // controller restart or a stale initialization-error flag. + await writeFile(path, JSON.stringify(initial)); + const repaired = await controller.execute({ + kind: 'add', + serverId: 'repaired', + config: { command: 'unused' }, + }); + assert.equal(repaired.status, 'applied'); + assert.equal(controller.snapshot().initialization, 'ready'); + assert.ok((await store.get()).mcpServers.repaired); + }); +} diff --git a/packages/cli/src/pi-tui-mcp-status.ts b/packages/cli/src/pi-tui-mcp-status.ts index 9ee6f6808f..4101343e95 100644 --- a/packages/cli/src/pi-tui-mcp-status.ts +++ b/packages/cli/src/pi-tui-mcp-status.ts @@ -24,6 +24,7 @@ import { matchesKey, truncateToWidth, visibleWidth, + wrapTextWithAnsi, type Component, type TUI, } from '@earendil-works/pi-tui'; @@ -52,6 +53,7 @@ interface TuiMcpStatusCopy { readonly title: string; readonly footer: { readonly back: string; + readonly diagnostic: string; readonly readOnly: string; readonly manage: string; readonly managePublication: string; @@ -60,6 +62,7 @@ interface TuiMcpStatusCopy { readonly unavailableDetail: string; readonly loading: string; readonly loadError: string; + readonly invalidConfigFile: string; readonly noServers: string; readonly publication: Readonly< Record['publication'], string> @@ -88,8 +91,10 @@ interface TuiMcpStatusCopy { }; } +type TuiMcpNoticeResult = Exclude; + type TuiMcpResultCode = - | Extract['reason'] + | Extract['reason'] | Extract['effect'] | 'turn_active' | 'invalid' @@ -136,6 +141,7 @@ type InputKind = type McpOverlayPhase = | { kind: 'list' } + | { kind: 'config_error'; path: string } | { kind: 'add_choice' } | { kind: 'transport'; draft: GuidedDraft } | { kind: 'protocol'; draft: GuidedDraft } @@ -210,7 +216,8 @@ export class McpManagementOverlay implements Component { else this.backToList(); return; } - if (this.phase.kind === 'list') this.handleListInput(data); + if (this.phase.kind === 'config_error') this.handleTextScroll(data); + else if (this.phase.kind === 'list') this.handleListInput(data); else if (this.phase.kind === 'add_choice') this.handleAddChoice(data); else if (this.phase.kind === 'transport') this.handleTransport(data); else if (this.phase.kind === 'protocol') this.handleProtocol(data); @@ -270,6 +277,12 @@ export class McpManagementOverlay implements Component { private handleListInput(data: string): void { const snapshot = this.input.surface?.snapshot(); const servers = snapshot?.servers ?? []; + if ( + (servers.length === 0 || snapshot?.initialization === 'error') && + this.handleTextScroll(data) + ) { + return; + } if (matchesKey(data, Key.up)) { this.selected = clamp(this.selected - 1, 0, servers.length - 1); } else if (matchesKey(data, Key.down)) { @@ -314,6 +327,19 @@ export class McpManagementOverlay implements Component { this.input.onChange(); } + private handleTextScroll(data: string): boolean { + if (matchesKey(data, Key.up)) this.top -= 1; + else if (matchesKey(data, Key.down)) this.top += 1; + else if (matchesKey(data, Key.pageUp)) this.top -= Math.max(1, this.bodyRows); + else if (matchesKey(data, Key.pageDown)) this.top += Math.max(1, this.bodyRows); + else if (matchesKey(data, Key.home)) this.top = 0; + else if (matchesKey(data, Key.end)) this.top = this.maxTop(); + else return false; + this.top = clamp(this.top, 0, this.maxTop()); + this.input.onChange(); + return true; + } + private handleAddChoice(data: string): void { if (matchesKey(data, 'g')) this.startInput('server_id', { serverId: '' }); else if (matchesKey(data, 'j')) this.startInput('import'); @@ -466,6 +492,13 @@ export class McpManagementOverlay implements Component { result = { status: 'failed', reason: 'manager-failed' }; } if (this.closed || attempt !== this.actionAttempt) return; + if (result.status === 'failed' && result.reason === 'invalid-config-file') { + this.phase = { kind: 'config_error', path: result.path }; + this.notice = undefined; + this.top = 0; + this.input.onChange(); + return; + } this.phase = { kind: 'list' }; this.notice = actionNotice(result, this.input.locale); this.input.onChange(); @@ -475,6 +508,9 @@ export class McpManagementOverlay implements Component { this.serverRows = []; const snapshot = this.input.surface?.snapshot(); if (!snapshot) return unavailableDocument(this.input.locale); + if (this.phase.kind === 'config_error') { + return wrapTextWithAnsi(invalidConfigFileCopy(this.input.locale, this.phase.path), width); + } if (this.phase.kind === 'input') return this.inputDocument(width); const editor = MCP_STATUS_COPY[this.input.locale].editor; if (this.phase.kind === 'add_choice') { @@ -528,7 +564,16 @@ export class McpManagementOverlay implements Component { } if (snapshot.initialization === 'loading') return [...lines, loadingCopy(this.input.locale)]; if (snapshot.initialization === 'error') { - return [...lines, ansi.red(loadErrorCopy(this.input.locale))]; + lines.push(ansi.red(loadErrorCopy(this.input.locale))); + if (snapshot.invalidConfigPath) { + lines.push( + ...wrapTextWithAnsi( + invalidConfigFileCopy(this.input.locale, snapshot.invalidConfigPath), + width, + ), + ); + } + return lines; } if (snapshot.servers.length === 0) return [...lines, '', emptyCopy(this.input.locale)]; lines.push(''); @@ -557,7 +602,9 @@ export class McpManagementOverlay implements Component { private footer(): string { const copy = MCP_STATUS_COPY[this.input.locale].footer; + if (this.phase.kind === 'config_error') return copy.diagnostic; if (this.phase.kind !== 'list') return copy.back; + if (this.input.surface?.snapshot().initialization === 'error') return copy.readOnly; if (!this.management()) return copy.readOnly; return this.input.surface?.snapshot().canManagePublicationCredential ? copy.managePublication @@ -749,7 +796,7 @@ function serverLines(server: TuiMcpServerSnapshot, locale: UiLocale, selected: b } function actionNotice( - result: TuiMcpActionResult, + result: TuiMcpNoticeResult, locale: UiLocale, ): { level: 'info' | 'error'; text: string } { if (result.status === 'conflict' || result.status === 'failed') { @@ -778,6 +825,14 @@ function resultCopy(locale: UiLocale, code: TuiMcpResultCode): string { return MCP_STATUS_COPY[locale].editor.results[code] ?? code; } +function invalidConfigFileCopy(locale: UiLocale, path: string): string { + return formatUiMessage( + MCP_STATUS_COPY[locale].invalidConfigFile, + { path: path.replace(/[\u0000-\u001f\u007f-\u009f]/gu, '') }, + locale, + ); +} + function confirmAddDocument(draft: GuidedDraft, locale: UiLocale): string[] { const editor = MCP_STATUS_COPY[locale].editor; return [ diff --git a/packages/cli/src/tui-copy-catalog.ts b/packages/cli/src/tui-copy-catalog.ts index 881dc7421b..3d5ab30736 100644 --- a/packages/cli/src/tui-copy-catalog.ts +++ b/packages/cli/src/tui-copy-catalog.ts @@ -267,6 +267,7 @@ export const TUI_COPY_RESOURCES = { title: 'MCP SERVERS', footer: { back: 'Esc back', + diagnostic: '↑/↓ scroll · Esc back', readOnly: '↑/↓ scroll · q/Esc close', manage: 'a Add · Enter Edit · Space Enable/disable · t Test · r Reconnect · d Remove · Esc Close', @@ -279,6 +280,8 @@ export const TUI_COPY_RESOURCES = { loading: 'Loading mcp.json and discovering tools…', loadError: 'MCP configuration could not be loaded; no tools were published to the Runtime Host.', + invalidConfigFile: + 'Invalid JSON in {path}. The file is unchanged. Close the app, back up and repair this file before retrying.', noServers: 'No MCP servers are configured. Press a to add one.', publication: { waiting: 'waiting to publish', @@ -375,6 +378,7 @@ export const TUI_COPY_RESOURCES = { title: 'MCP 服务器', footer: { back: 'Esc 返回', + diagnostic: '↑/↓ 滚动 · Esc 返回', readOnly: '↑/↓ 滚动 · q/Esc 关闭', manage: 'a 添加 · Enter 编辑 · Space 启用/停用 · t 测试 · r 重连 · d 删除 · Esc 关闭', managePublication: @@ -384,6 +388,8 @@ export const TUI_COPY_RESOURCES = { unavailableDetail: '远程 Runtime Host 的客户端 MCP 工具关联将在后续版本提供。', loading: '正在读取 mcp.json 并发现工具…', loadError: '无法读取或应用 MCP 配置；没有向 Runtime Host 发布工具。', + invalidConfigFile: + '{path} 中的 JSON 无效，文件未被修改。请关闭应用，备份并修复此文件后重试。', noServers: '尚未配置 MCP 服务器。按 a 添加。', publication: { waiting: '等待发布', @@ -475,6 +481,7 @@ export const TUI_COPY_RESOURCES = { title: 'MCP 伺服器', footer: { back: 'Esc 返回', + diagnostic: '↑/↓ 捲動 · Esc 返回', readOnly: '↑/↓ 捲動 · q/Esc 關閉', manage: 'a 新增 · Enter 編輯 · Space 啟用/停用 · t 測試 · r 重新連線 · d 刪除 · Esc 關閉', managePublication: @@ -484,6 +491,8 @@ export const TUI_COPY_RESOURCES = { unavailableDetail: '遠端 Runtime Host 的用戶端 MCP 工具關聯將於後續版本提供。', loading: '正在讀取 mcp.json 並探索工具…', loadError: '無法讀取或套用 MCP 設定；未向 Runtime Host 發佈任何工具。', + invalidConfigFile: + '{path} 中的 JSON 無效，檔案未被修改。請關閉應用程式，備份並修復此檔案後重試。', noServers: '尚未設定 MCP 伺服器。按 a 新增。', publication: { waiting: '等待發佈', diff --git a/packages/cli/src/tui-mcp-control.ts b/packages/cli/src/tui-mcp-control.ts index a8d0ba6c20..6ef7ab2e65 100644 --- a/packages/cli/src/tui-mcp-control.ts +++ b/packages/cli/src/tui-mcp-control.ts @@ -79,6 +79,8 @@ export interface TuiMcpServerSnapshot { export interface TuiMcpSnapshot { readonly initialization: 'loading' | 'ready' | 'error'; + /** Safe source location for invalid persisted JSON, never the parser message. */ + readonly invalidConfigPath?: string; readonly configuration: 'ready' | 'synchronizing' | 'out_of_sync'; readonly publication: TuiMcpPublicationState; readonly canManagePublicationCredential?: boolean; @@ -140,6 +142,11 @@ export type TuiMcpActionEffect = export type TuiMcpActionResult = | { readonly status: 'applied'; readonly effect: TuiMcpActionEffect } | { readonly status: 'tested'; readonly test: McpTestResult; readonly effect: TuiMcpActionEffect } + | { + readonly status: 'failed'; + readonly reason: 'invalid-config-file'; + readonly path: string; + } | { readonly status: 'failed'; readonly reason: 'commit-unknown'; @@ -423,9 +430,15 @@ class TuiMcpControllerImpl implements TuiMcpController { this.#config = cloneConfig(config); this.#refreshManagerSnapshot('ready', 'ready'); this.#requestPublication(); - } catch { + } catch (error) { if (this.#closed) return; - this.#updateSnapshot({ initialization: 'error', publication: 'not_published' }); + this.#updateSnapshot({ + initialization: 'error', + publication: 'not_published', + ...(error instanceof McpConfigSourceError && error.reason === 'invalid-json' && error.path + ? { invalidConfigPath: error.path } + : {}), + }); } } @@ -517,6 +530,10 @@ class TuiMcpControllerImpl implements TuiMcpController { if (error instanceof TuiMcpMutationError) return error.result; if (error instanceof McpConfigurationValidationError) return { status: 'failed', reason: 'invalid-config' }; + + if (error instanceof McpConfigSourceError && error.reason === 'invalid-json' && error.path) { + return { status: 'failed', reason: 'invalid-config-file', path: error.path }; + } if (error instanceof AtomicFileWriteCommitUnknownError) { // The transform has already published, including any credential // retirement. Reload its authority; never replay those effects. @@ -707,7 +724,9 @@ class TuiMcpControllerImpl implements TuiMcpController { } #updateSnapshot( - update: Partial>, + update: Partial< + Pick + >, ): void { this.#snapshot = freezeSnapshot({ ...this.#snapshot, ...update }); this.#notify(); diff --git a/packages/storage/src/__tests__/mcp-config-store.test.ts b/packages/storage/src/__tests__/mcp-config-store.test.ts index 2a116cd8c5..f4534f4e48 100644 --- a/packages/storage/src/__tests__/mcp-config-store.test.ts +++ b/packages/storage/src/__tests__/mcp-config-store.test.ts @@ -26,6 +26,7 @@ import { afterEach, test } from 'node:test'; import { MCP_CONFIG_VERSION, resolveMcpProtocolPreference } from '@maka/core/mcp'; import { createMcpConfigStore, + McpConfigSourceError, normalizeMcpConfig, normalizeMcpImport, } from '../mcp-config-store.js'; @@ -633,3 +634,44 @@ async function waitUntil(condition: () => boolean, timeoutMs = 3_000): Promise setTimeout(resolve, 20)); } } + +test('corrupt persisted MCP JSON has a safe actionable error and mutations cannot overwrite it', async () => { + const root = await tempRoot(); + const path = join(root, 'mcp.json'); + const bytes = Buffer.from('{"secret":"never-include-this"'); + await writeFile(path, bytes); + const store = createMcpConfigStore(root); + let transformed = false; + for (const operation of [ + () => store.get(), + () => + store.transform((config) => { + transformed = true; + return config; + }), + () => store.upsert('new', { command: 'unused' }), + () => store.remove('old'), + ]) { + await assert.rejects(operation(), (error) => { + assert.ok(error instanceof McpConfigSourceError); + assert.equal(error.reason, 'invalid-json'); + assert.equal(error.path, path); + assert.ok(error.message.includes(path)); + assert.match(error.message, /not modified.*back up and repair/u); + assert.equal(error.message.includes('never-include-this'), false); + assert.equal(error.cause, undefined); + return true; + }); + assert.deepEqual(await readFile(path), bytes); + } + assert.equal(transformed, false); + assert.throws( + () => normalizeMcpImport('{bad'), + (error) => { + assert.ok(error instanceof McpConfigSourceError); + assert.equal(error.reason, 'invalid-json'); + assert.equal(error.path, undefined); + return true; + }, + ); +}); diff --git a/packages/storage/src/mcp-config-store.ts b/packages/storage/src/mcp-config-store.ts index 0577a2f1a2..56cea28a0a 100644 --- a/packages/storage/src/mcp-config-store.ts +++ b/packages/storage/src/mcp-config-store.ts @@ -76,6 +76,7 @@ export class McpConfigSourceError extends Error { readonly reason: McpConfigSourceFailureReason, readonly version?: string, message: string = reason, + readonly path?: string, ) { super(message); this.name = 'McpConfigSourceError'; @@ -275,7 +276,22 @@ class FileMcpConfigStore implements McpConfigStore { if (Buffer.byteLength(text, 'utf8') > MAX_CONFIG_BYTES) { throw new Error('MCP config exceeds 1 MiB'); } - return normalizeMcpConfig(JSON.parse(text)); + let persisted: unknown; + try { + persisted = JSON.parse(text); + } catch (error) { + if (!(error instanceof SyntaxError)) throw error; + // JSON.parse can quote credentials in its message. Report the location + // and recovery action without retaining those source bytes in an error. + throw new McpConfigSourceError( + 'invalid-json', + undefined, + `MCP config at ${this.path} contains invalid JSON. The file was not modified. ` + + 'Close the app, back up and repair this file before retrying.', + this.path, + ); + } + return normalizeMcpConfig(persisted); } private async readOrCreate(): Promise { From 897186a7f6f16e29879b535116ad5cb434dcf17d Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Wed, 23 Sep 2026 14:39:15 +0800 Subject: [PATCH 3/8] fix(storage): harden corrupt settings recovery and reporting Recheck the source immediately before publishing defaults, preserve external repairs, and accept UTF-8 BOM files without resetting their settings. Reuse verified content-based backups across retries while preserving interrupted backup writes and moving to an available suffix. Keep recovery guidance pending until an application dialog is acknowledged, including startup failures and notification clicks with a closed window. Use the existing settings refresh paths and retain pending renderer changes. Cover interrupted processes, backup reuse failures, concurrent repairs, encoding and link boundaries, and startup notification delivery. Refs #4285 Generated-by: OpenAI Codex --- .../__tests__/client-settings-effects.test.ts | 53 +- .../settings-recovery-startup.test.ts | 315 +++++++++++ .../main/__tests__/settings-recovery.test.ts | 162 ++++-- .../src/main/client-settings-effects.ts | 23 +- apps/desktop/src/main/early-window.ts | 49 +- apps/desktop/src/main/main-window.ts | 12 +- apps/desktop/src/main/native-notification.ts | 32 ++ apps/desktop/src/main/notifications-main.ts | 7 +- apps/desktop/src/main/runtime-host-boot.ts | 6 +- apps/desktop/src/main/settings-recovery.ts | 116 ++-- docs/windows-test-inventory.md | 10 +- .../src/__tests__/atomic-file-write.test.ts | 43 ++ .../settings-store-onboarding.test.ts | 1 + .../__tests__/settings-store-recovery.test.ts | 506 +++++++++++++++++- packages/storage/src/atomic-file-write.ts | 5 + packages/storage/src/settings-store.ts | 192 ++++++- 16 files changed, 1338 insertions(+), 194 deletions(-) create mode 100644 apps/desktop/src/main/__tests__/settings-recovery-startup.test.ts create mode 100644 apps/desktop/src/main/native-notification.ts diff --git a/apps/desktop/src/main/__tests__/client-settings-effects.test.ts b/apps/desktop/src/main/__tests__/client-settings-effects.test.ts index 5c89e31c44..d1177da7c9 100644 --- a/apps/desktop/src/main/__tests__/client-settings-effects.test.ts +++ b/apps/desktop/src/main/__tests__/client-settings-effects.test.ts @@ -44,11 +44,12 @@ test('applies each client settings snapshot once across local writes and file wa observeLocale: () => undefined, emitExternalChanged: () => { rendererEvents += 1; + return true; }, }); assert.equal(await effects.refresh(false), true); - assert.equal(await effects.refresh(true), true); // First renderer delivery. + assert.equal(await effects.refresh(true), false); // Startup establishes the renderer baseline. assert.equal(await effects.refresh(true), false); current = { @@ -60,16 +61,16 @@ test('applies each client settings snapshot once across local writes and file wa assert.deepEqual(keepAwake, [false, true]); assert.equal(botApplications, 1); - assert.equal(rendererEvents, 2); + assert.equal(rendererEvents, 1); // The shipped default is already on screen before the first snapshot is // read, so a run that never leaves it must not touch the OS icon at all. assert.deepEqual(appIcons, []); }); -test('silent refreshes retain an undelivered renderer change without repeating effects', async () => { +test('silent refreshes retain an un-emitted renderer change without repeating effects', async () => { let current = createDefaultSettings(); const keepAwake: boolean[] = []; - const deliveredLocales: string[] = []; + const emittedLocales: string[] = []; const effects = createClientSettingsEffects({ settingsStore: { get: async () => current }, applyWorkHub: async () => undefined, @@ -78,7 +79,7 @@ test('silent refreshes retain an undelivered renderer change without repeating e applyAppIcon: async () => undefined, systemPrefersDark: () => false, observeLocale: () => undefined, - emitExternalChanged: () => { deliveredLocales.push(current.personalization.uiLocale); }, + emitExternalChanged: () => { emittedLocales.push(current.personalization.uiLocale); return true; }, }); await effects.refresh(false); current = { @@ -88,12 +89,12 @@ test('silent refreshes retain an undelivered renderer change without repeating e }; assert.equal(await effects.refresh(false), true); assert.equal(await effects.refresh(false), false); - assert.deepEqual(deliveredLocales, []); - // A later write supersedes the silently applied snapshot before delivery. + assert.deepEqual(emittedLocales, []); + // A later write supersedes the silently applied snapshot before emission. current = { ...current, personalization: { ...current.personalization, uiLocale: 'zh-TW' } }; assert.equal(await effects.refresh(true), true); assert.equal(await effects.refresh(true), false); - assert.deepEqual(deliveredLocales, ['zh-TW']); + assert.deepEqual(emittedLocales, ['zh-TW']); assert.deepEqual(keepAwake, [false, true]); }); @@ -110,7 +111,7 @@ test('applies a chosen app icon once, and again only when the choice changes', a }, systemPrefersDark: () => false, observeLocale: () => undefined, - emitExternalChanged: () => undefined, + emitExternalChanged: () => true, }); await effects.refresh(false); @@ -147,7 +148,7 @@ test('an OS appearance flip re-applies the icon without any setting changing', a }, systemPrefersDark: () => systemDark, observeLocale: () => undefined, - emitExternalChanged: () => undefined, + emitExternalChanged: () => true, }); await effects.refresh(false); @@ -184,7 +185,7 @@ test('with one icon for both appearances a theme flip changes nothing', async () }, systemPrefersDark: () => systemDark, observeLocale: () => undefined, - emitExternalChanged: () => undefined, + emitExternalChanged: () => true, }); await effects.refresh(false); @@ -210,7 +211,7 @@ test('an explicit dark preference ignores what the OS reports', async () => { }, systemPrefersDark: () => false, observeLocale: () => undefined, - emitExternalChanged: () => undefined, + emitExternalChanged: () => true, }); await effects.refresh(false); assert.deepEqual(applied, ['ink']); @@ -227,10 +228,36 @@ test('applies WorkHub enable state from the supplied snapshot without another st applyAppIcon: async () => undefined, systemPrefersDark: () => false, observeLocale: () => undefined, - emitExternalChanged: () => undefined, + emitExternalChanged: () => true, }); const settings = createDefaultSettings(); await effects.apply({ ...settings, workHub: { enabled: true } }, true); await effects.apply({ ...settings, workHub: { enabled: false } }, true); assert.deepEqual(applied, [true, false]); }); + + +test('an unavailable renderer does not consume a changed settings fingerprint', async () => { + let current = createDefaultSettings(); + let available = false; + let emitted = 0; + const effects = createClientSettingsEffects({ + settingsStore: { get: async () => current }, + applyWorkHub: async () => undefined, + applyKeepSystemAwake: async () => undefined, + applyBotSettings: async () => undefined, + applyAppIcon: async () => undefined, + systemPrefersDark: () => false, + observeLocale: () => undefined, + emitExternalChanged: () => { if (!available) return false; emitted += 1; return true; }, + }); + await effects.refresh(false); + current = { ...current, system: { keepSystemAwake: true } }; + assert.equal(await effects.refresh(true), true); // Effects changed without an event. + assert.equal(await effects.refresh(true), false); + assert.equal(emitted, 0); + available = true; + assert.equal(await effects.refresh(true), true); // Only the pending event changes. + assert.equal(await effects.refresh(true), false); + assert.equal(emitted, 1); +}); diff --git a/apps/desktop/src/main/__tests__/settings-recovery-startup.test.ts b/apps/desktop/src/main/__tests__/settings-recovery-startup.test.ts new file mode 100644 index 0000000000..2a5554574d --- /dev/null +++ b/apps/desktop/src/main/__tests__/settings-recovery-startup.test.ts @@ -0,0 +1,315 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import assert from 'node:assert/strict'; +import { EventEmitter } from 'node:events'; +import { readFileSync } from 'node:fs'; +import fs, { mkdir, mkdtemp, readFile, readdir, rm, writeFile } from 'node:fs/promises'; +import { createRequire, syncBuiltinESMExports } from 'node:module'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { test } from 'node:test'; +import { runInNewContext } from 'node:vm'; +import { parse } from '@babel/parser'; +import { transform } from 'esbuild'; +import type { MessageBoxOptions } from 'electron'; +import type { AppSettings } from '@maka/core/settings'; +import { resolveSystemUiLocale } from '@maka/core/ui-locale'; +import { createSettingsStore, SettingsRecoveryCommitUnknownError } from '@maka/storage/settings-store'; +import { createAppQuitCoordinator, type AppQuitCoordinatorDeps } from '../app-quit-coordinator.js'; +import { createSettingsRecoveryReporter } from '../settings-recovery.js'; +import { createWindowRevealGate } from '../window-reveal.js'; + +const require = createRequire(import.meta.url); +const turn = () => new Promise((resolve) => setImmediate(resolve)); +const source = readFileSync(new URL('../../../src/main/early-window.ts', import.meta.url), 'utf8'); +const imports = parse(source, { sourceType: 'module', plugins: ['typescript'] }).program.body + .filter((node) => node.type === 'ImportDeclaration'); +const importEnd = imports.at(-1)?.end ?? 0; +const body = source.slice(importEnd).replace(/\bexport\s+(?=(?:async\s+)?(?:function|const|let|var|class)\b)/gu, ''); +const boot = (await transform(`${source.slice(0, importEnd)}\nexport default async function() {\n${body}\n}`, { + loader: 'ts', format: 'cjs', target: 'esnext', +})).code; + +for (const nativeFailure of ['unsupported', 'failed', 'commit-unknown'] as const) { + test(`early-window presents recovery guidance despite ${nativeFailure}`, { + timeout: 5_000, skip: nativeFailure === 'commit-unknown' && process.platform === 'win32', + }, async (t) => { + const userData = await mkdtemp(join(tmpdir(), 'maka-recovery-window-')); + t.after(async () => { + t.mock.restoreAll(); + syncBuiltinESMExports(); + await rm(userData, { recursive: true, force: true }); + }); + const root = join(userData, 'workspaces', 'default'); + await mkdir(root, { recursive: true }); + const settingsPath = join(root, 'settings.json'); + await writeFile(settingsPath, 'sk-live-SECRET'); + if (nativeFailure === 'commit-unknown') { + const originalOpen = fs.open; + let directorySyncs = 0; + t.mock.method(fs, 'open', async (...args: Parameters) => { + const handle = await originalOpen(...args); + if (args[0] === root && ++directorySyncs === 2) { + t.mock.method(handle, 'sync', async () => { throw new Error('injected publication fence failure'); }); + } + return handle; + }); + syncBuiltinESMExports(); + } + const window = Object.assign(new EventEmitter(), { + isVisible: () => visible, + isMinimized: () => false, + isDestroyed: () => false, + }); + let visible = false; + let constructed = false; + let windowLaunch: Promise | undefined; + let windowError: unknown; + const dialogs: MessageBoxOptions[] = []; + let resolveShown!: () => void; + const shown = new Promise((resolve) => { resolveShown = resolve; }); + const logs: string[] = []; + let banners = 0; + const deps = { + app: { + isPackaged: true, + getAppPath: () => '/test/Maka.app', + getPath: () => userData, + getPreferredSystemLanguages: () => ['en-US'], + on: () => undefined, + }, + ipcMain: { handle: () => undefined }, + nativeTheme: { shouldUseDarkColors: false }, + Notification: { isSupported: () => nativeFailure === 'failed' }, + showNativeNotification: (_copy: unknown, _focus: unknown, failed: () => void) => { banners += 1; failed(); }, + isIsolatedE2e: false, + revealMode: 'active', + resolveSystemUiLocale, + resolveShellEnv: async () => undefined, + resolveBuildInfo: () => ({ mode: 'packaged' }), + resolveE2eFixture: () => undefined, + resolveDesktopStorageRoot: async () => ({ canonicalPath: root }), + startupStep: (_name: string, work: Promise) => work, + createSettingsRecoveryReporter, + createSettingsStore, + createDesktopLocaleAuthority: () => ({ current: () => 'en', observe: () => 'en' }), + isDarkAppearance: () => false, + bootContext: {}, + createMainWindowController: (options: { + settingsStore: { get(): Promise }; + onWindowConstructed(): void; + }) => ({ + browserWindow: () => constructed ? window : undefined, + hasOpenWindows: () => constructed, + createWindow: async () => { + // The real controller reads settings before creating BrowserWindow. + await options.settingsStore.get(); + constructed = true; + options.onWindowConstructed(); + }, + }), + createAppQuitCoordinator: (options: { + focusOrCreateWindow(signal: AbortSignal): Promise; + onWindowCreationError(error: unknown): void; + }) => ({ + focusOrCreateWindow: () => { + windowLaunch = options.focusOrCreateWindow(new AbortController().signal).catch((error) => { + windowError = error; + options.onWindowCreationError(error); + }); + return windowLaunch; + }, + handleBeforeQuit: () => undefined, + }), + showBrowserMessageBox: async (options: MessageBoxOptions, parent: unknown) => { + assert.equal(parent, nativeFailure === 'commit-unknown' ? undefined : window); + assert.equal(visible, nativeFailure !== 'commit-unknown'); + dialogs.push(options); + resolveShown(); + return { response: 0, checkboxChecked: false }; + }, + }; + await runInNewContext(`${boot}\nmodule.exports.default()`, { + module: { exports: {} }, process: { env: {}, argv: [] }, + console: { warn: (message: string) => logs.push(message), error: (message: string) => logs.push(message) }, + require: (name: string) => name.startsWith('node:') ? require(name) : deps, + }); + await windowLaunch; + if (nativeFailure === 'commit-unknown') { + assert.ok(windowError instanceof SettingsRecoveryCommitUnknownError); + assert.equal(constructed, false, 'the failing storage read still aborts window creation'); + } else { + await turn(); + assert.equal(dialogs.length, 0); + window.emit('ready-to-show'); + await turn(); + assert.equal(dialogs.length, 0, 'a hidden window must not consume the guidance'); + visible = true; + window.emit('show'); + } + await shown; + assert.equal(dialogs.length, 1); + assert.notEqual(dialogs[0].message, dialogs[0].title); + assert.match(dialogs[0].message, nativeFailure === 'commit-unknown' ? /unconfirmed/u : /reset to defaults/u); + if (nativeFailure === 'commit-unknown') assert.match(dialogs[0].message + (dialogs[0].detail ?? ''), /unconfirmed/u); + assert.ok(dialogs[0].detail?.includes(settingsPath)); + const backup = (await readdir(root)).find((file) => file.includes('.corrupt-')); + assert.ok(backup); + assert.ok(dialogs[0].detail?.includes(join(root, backup))); + assert.equal(await readFile(join(root, backup), 'utf8'), 'sk-live-SECRET'); + assert.equal(JSON.stringify(dialogs).includes('sk-live-SECRET'), false); + assert.equal(logs.join('').includes('sk-live-SECRET'), false); + assert.equal(banners, nativeFailure === 'failed' ? 1 : 0); + window.emit('show'); + window.emit('restore'); + await turn(); + assert.equal(dialogs.length, 1); + }); +} + +test('clicking a recovery notification reopens a closed main window and presents its pending notice', { + timeout: 5_000, +}, async (t) => { + const userData = await mkdtemp(join(tmpdir(), 'maka-recovery-notification-')); + t.after(() => rm(userData, { recursive: true, force: true })); + const root = join(userData, 'workspaces', 'default'); + await mkdir(root, { recursive: true }); + const settingsPath = join(root, 'settings.json'); + await writeFile(settingsPath, '{}'); + + function createWindow() { + let visible = false; + const window = Object.assign(new EventEmitter(), { + isVisible: () => visible, + isMinimized: () => false, + isDestroyed: () => false, + focus: () => undefined, + restore: () => undefined, + maximize: () => undefined, + showInactive: () => { visible = true; window.emit('show'); }, + show: () => { visible = true; window.emit('show'); }, + }); + return window; + } + let window: ReturnType | undefined; + let creations = 0; + let store: ReturnType | undefined; + let windowLaunch: Promise | undefined; + const gate = createWindowRevealGate('active'); + const clicks: (() => void)[] = []; + const dialogs: MessageBoxOptions[] = []; + let resolveShown!: () => void; + const shown = new Promise((resolve) => { resolveShown = resolve; }); + const deps = { + app: { + isPackaged: true, + getAppPath: () => '/test/Maka.app', + getPath: () => userData, + getPreferredSystemLanguages: () => ['en-US'], + on: () => undefined, + }, + ipcMain: { handle: () => undefined }, + nativeTheme: { shouldUseDarkColors: false }, + Notification: { isSupported: () => true }, + showNativeNotification: (_copy: unknown, click: () => void) => { clicks.push(click); }, + isIsolatedE2e: false, + revealMode: 'active', + resolveSystemUiLocale, + resolveShellEnv: async () => undefined, + resolveBuildInfo: () => ({ mode: 'packaged' }), + resolveE2eFixture: () => undefined, + resolveDesktopStorageRoot: async () => ({ canonicalPath: root }), + startupStep: (_name: string, work: Promise) => work, + createSettingsRecoveryReporter, + createSettingsStore: (...args: Parameters) => { + store = createSettingsStore(...args); + return store; + }, + createDesktopLocaleAuthority: () => ({ current: () => 'en', observe: () => 'en' }), + isDarkAppearance: () => false, + bootContext: {}, + createMainWindowController: (options: { + settingsStore: { get(): Promise }; + onWindowConstructed(): void; + }) => ({ + browserWindow: () => window, + hasOpenWindows: () => window !== undefined, + focus: () => gate.requestFocus(window ?? null), + createWindow: async () => { + await options.settingsStore.get(); + window = createWindow(); + creations += 1; + gate.reset(); + options.onWindowConstructed(); + window.emit('ready-to-show'); + gate.markReady(window); + }, + }), + createAppQuitCoordinator: (options: AppQuitCoordinatorDeps) => { + const coordinator = createAppQuitCoordinator(options); + return { + ...coordinator, + focusOrCreateWindow: () => { + windowLaunch = coordinator.focusOrCreateWindow(); + return windowLaunch; + }, + }; + }, + showBrowserMessageBox: async (options: MessageBoxOptions, parent: unknown) => { + assert.ok(window?.isVisible()); + assert.equal(parent, window); + dialogs.push(options); + resolveShown(); + return { response: 0, checkboxChecked: false }; + }, + }; + await runInNewContext(`${boot}\nmodule.exports.default()`, { + module: { exports: {} }, process: { env: {}, argv: [] }, + console: { warn: () => undefined, error: () => undefined }, + require: (name: string) => name.startsWith('node:') ? require(name) : deps, + }); + await windowLaunch; + assert.equal(creations, 1); + assert.ok(store); + // The app remains in the Windows tray after the main window is closed. + // No macOS activate event is emitted to reopen it on a notification click. + window = undefined; + await writeFile(settingsPath, 'sk-live-SECRET'); + await store.get(); + await turn(); + assert.equal(clicks.length, 1); + assert.equal(dialogs.length, 0); + + clicks[0](); + await windowLaunch; + assert.equal(creations, 2, 'clicking the banner recreates the main window'); + await shown; + assert.equal(dialogs.length, 1); + assert.ok(dialogs[0].detail?.includes(settingsPath)); + const backup = (await readdir(root)).find((name) => name.includes('.corrupt-')); + assert.ok(backup); + assert.ok(dialogs[0].detail?.includes(join(root, backup))); + + clicks[0](); + await windowLaunch; + await turn(); + assert.equal(creations, 2, 'a later click focuses the existing window'); + assert.equal(dialogs.length, 1, 'the acknowledged notice is not repeated'); +}); diff --git a/apps/desktop/src/main/__tests__/settings-recovery.test.ts b/apps/desktop/src/main/__tests__/settings-recovery.test.ts index 645812cd55..d5dc2b47e1 100644 --- a/apps/desktop/src/main/__tests__/settings-recovery.test.ts +++ b/apps/desktop/src/main/__tests__/settings-recovery.test.ts @@ -40,106 +40,153 @@ const event: CorruptSettingsRecovery = { }; const turn = () => new Promise((resolve) => setImmediate(resolve)); -function harness(options: { e2e?: boolean; supported?: boolean; throws?: 'support' | 'create' | 'show' } = {}) { +function harness(options: { + e2e?: boolean; + supported?: boolean; + throws?: 'support' | 'show'; + holdNotice?: boolean; +} = {}) { const notices: { title: string; body: string }[] = []; + const appNotices: { title: string; message: string; detail: string }[] = []; const logs: string[] = []; const failures: (() => void)[] = []; + const dismissals: (() => void)[] = []; let supports = 0; + let available = false; + let failNotice = false; const reporter = createSettingsRecoveryReporter({ e2e: options.e2e ?? false, locale: () => 'en', log: (message) => { logs.push(message); }, + showNotice: async (copy) => { + if (!available) return false; + if (failNotice) throw new Error('secret dialog detail'); + appNotices.push(copy); + if (options.holdNotice) await new Promise((resolve) => { dismissals.push(resolve); }); + return true; + }, notifications: { isSupported() { supports += 1; if (options.throws === 'support') throw new Error('unsupported'); return options.supported ?? true; }, - create(copy, failed) { - if (options.throws === 'create') throw new Error('create failed'); + show(copy, failed) { failures.push(failed); - return { show() { - if (options.throws === 'show') throw new Error('show failed'); - notices.push(copy); - } }; + if (options.throws === 'show') throw new Error('show failed'); + notices.push(copy); }, }, }); - return { reporter, notices, logs, failures, supports: () => supports }; + return { + reporter, notices, appNotices, logs, failures, dismissals, + supports: () => supports, + setAvailable: () => { available = true; reporter.onWindowReady(); }, + failNotice: (value: boolean) => { failNotice = value; }, + }; } -test('localized recovery copy names the backup, privacy review and uncertain outcome', () => { +test('localized recovery guidance names full paths, reset scope, privacy review and uncertain outcome', () => { for (const locale of UI_LOCALES) { const recovered = settingsRecoveryCopy(event, locale); const unknown = settingsRecoveryCopy({ ...event, outcome: 'commit-unknown' }, locale); - assert.ok(recovered.body.includes(event.backupPath)); - assert.ok(unknown.body.includes(event.backupPath)); + for (const copy of [recovered, unknown]) { + assert.ok(copy.detail.includes(event.settingsPath)); + assert.ok(copy.detail.includes(event.backupPath)); + assert.ok(copy.body.indexOf(event.backupPath) < copy.body.indexOf('\n')); + assert.match(copy.detail, /Incognito|隐身|無痕/u); + assert.match(copy.detail, /bot configuration|机器人配置|機器人設定/u); + assert.match(copy.detail, /onboarding|首次使用/u); + assert.doesNotMatch(copy.detail, /now off|已关闭|已關閉/u); + } assert.notEqual(recovered.title, unknown.title); - assert.match(recovered.body, /Incognito|隐身|無痕/u); - assert.doesNotMatch(recovered.body + unknown.body, /now off|已关闭|已關閉/u); - assert.match(unknown.body, /unconfirmed|未确认|未確認/u); - const failed = settingsRecoveryCopy({ ...event, outcome: 'commit-unknown' }, locale, true); - assert.match(failed.body, /unconfirmed|未确认|未確認/u); - assert.match(failed.body, /Restart|重启|重新啟動/u); + assert.match(unknown.message + unknown.detail, /unconfirmed|未确认|未確認/u); + assert.match(unknown.message + unknown.detail, /Restart|重启|重新啟動/u); } }); -test('early recovery is logged and reported immediately, then reread when effects become ready', async () => { - const h = harness(); - let refreshes = 0; - h.reporter.onRecovery(event); - assert.equal(h.notices.length, 1); - assert.ok(h.logs[0].includes(event.backupPath)); - assert.equal(refreshes, 0); - const effects = { refresh: async (notify: boolean) => { assert.equal(notify, true); refreshes += 1; return true; } }; - h.reporter.setEffects(effects); - await turn(); - assert.equal(refreshes, 1); - h.reporter.setEffects(effects); - await turn(); - assert.equal(refreshes, 1); -}); - -for (const options of [{ e2e: true }, { supported: false }]) { - test('notification suppression does not suppress recovery diagnostics or effects', async () => { +for (const options of [{ supported: false }, { e2e: true }, { throws: 'show' as const }]) { + test(`startup recovery survives unavailable native notifications (${JSON.stringify(options)})`, async () => { const h = harness(options); - let refreshed = false; - h.reporter.setEffects({ refresh: async () => { refreshed = true; return false; } }); h.reporter.onRecovery(event); await turn(); + assert.equal(h.appNotices.length, 0); + assert.ok(h.logs[0].includes(event.backupPath)); + h.setAvailable(); + await turn(); + assert.equal(h.appNotices.length, 1); + assert.ok(h.appNotices[0].detail.includes(event.backupPath)); + h.reporter.onWindowReady(); + await turn(); + assert.equal(h.appNotices.length, 1); assert.equal(h.notices.length, 0); - assert.equal(refreshed, true); - assert.ok(h.logs.some((line) => line.includes(event.backupPath))); - if (options.e2e) assert.equal(h.supports(), 0); + if ('e2e' in options) assert.equal(h.supports(), 0); }); } -for (const phase of ['support', 'create', 'show'] as const) { - test(`notification ${phase} failure is isolated`, async () => { +for (const phase of ['support', 'show'] as const) { + test(`notification ${phase} failure cannot suppress the app notice`, async () => { const h = harness({ throws: phase }); + h.setAvailable(); h.reporter.onRecovery(event); await turn(); + assert.equal(h.appNotices.length, 1); assert.ok(h.logs.some((line) => line.includes('notification failed'))); }); } -test('asynchronous native notification failure is logged without throwing', () => { +test('asynchronous native notification failure retains one app notice and never creates a second banner', async () => { const h = harness(); + h.setAvailable(); h.reporter.onRecovery(event); assert.doesNotThrow(() => h.failures[0]()); + await turn(); + assert.equal(h.notices.length, 1); + assert.equal(h.appNotices.length, 1); assert.ok(h.logs.some((line) => line.includes('notification failed'))); }); -test('refresh failure keeps the publication warning and does not leak the failing effect error', async () => { +test('a failed app dialog remains pending for the next usable window without leaking the error', async () => { const h = harness(); - h.reporter.setEffects({ refresh: async () => { throw new Error('secret effect detail'); } }); + h.failNotice(true); + h.setAvailable(); h.reporter.onRecovery({ ...event, outcome: 'commit-unknown' }); await turn(); - assert.equal(h.notices.length, 2); - assert.match(h.notices[1].body, /unconfirmed/u); - assert.match(h.notices[1].body, /Restart/u); - assert.equal(JSON.stringify(h).includes('secret effect detail'), false); - assert.ok(h.logs.some((line) => line.includes('refresh failed'))); + assert.equal(h.appNotices.length, 0); + assert.ok(h.logs.some((line) => line.includes('app notice failed'))); + assert.equal(h.logs.join('').includes('secret dialog detail'), false); + h.failNotice(false); + h.reporter.onWindowReady(); + await turn(); + assert.equal(h.appNotices.length, 1); + assert.match(h.appNotices[0].message + h.appNotices[0].detail, /unconfirmed/u); + assert.equal(h.notices.length, 1); +}); + +test('multiple recoveries wait for dismissal and repeated window events do not repeat notices', async () => { + const h = harness({ holdNotice: true }); + h.setAvailable(); + h.reporter.onRecovery(event); + h.reporter.onRecovery({ ...event, backupPath: `${event.backupPath}-second`, outcome: 'commit-unknown' }); + await turn(); + assert.equal(h.appNotices.length, 1); + h.reporter.onWindowReady(); + await turn(); + assert.equal(h.appNotices.length, 1); + h.dismissals[0](); + await turn(); + assert.equal(h.appNotices.length, 2); + assert.match(h.appNotices[1].message + h.appNotices[1].detail, /unconfirmed/u); + h.dismissals[1](); + await turn(); + h.reporter.onWindowReady(); + await turn(); + assert.equal(h.appNotices.length, 2); + // Reusing a complete backup on a later corruption is still a new reset. + h.reporter.onRecovery(event); + await turn(); + assert.equal(h.appNotices.length, 3); + h.dismissals[2](); }); async function realStore(t: TestContext) { @@ -163,7 +210,7 @@ async function realStore(t: TestContext) { applyBotSettings: async (value) => { bots.push(value); }, applyAppIcon: async () => {}, observeLocale: (settings) => { observed.push(settings.personalization.uiLocale); }, - emitExternalChanged: () => { changes += 1; }, + emitExternalChanged: () => { changes += 1; return true; }, }); return { root, path: join(root, 'settings.json'), h, store, effects, observed, bots, keepAwake, changes: () => changes }; } @@ -173,25 +220,23 @@ for (const notifyRenderer of [false, true]) { const { path, h, effects, observed, bots, keepAwake, changes, store } = await realStore(t); await store.update({ personalization: { uiLocale: 'zh-CN' }, system: { keepSystemAwake: true } }); await effects.refresh(false); - h.reporter.setEffects(effects); - await writeFile(path, '{"secret":"never print this"'); + await writeFile(path, 'sk-live-SECRET'); await effects.refresh(notifyRenderer); await turn(); - await effects.refresh(true); // Barrier behind the callback's queued refresh. - assert.deepEqual(observed, ['zh-CN', 'auto', 'auto', 'auto']); + await effects.refresh(true); // The existing file watcher rereads after a silent theme refresh. + assert.deepEqual(observed, ['zh-CN', 'auto', 'auto']); assert.deepEqual(keepAwake, [true, false]); assert.equal(bots.length, 1); // Bots did not change, so effects deduplicate them. assert.equal(changes(), 1); assert.equal(h.notices.length, 1); - assert.equal(h.logs.join('').includes('never print this'), false); + assert.equal(h.logs.join('').includes('sk-live-SECRET'), false); }); } -test('recovery before effects initialization refreshes the latest file including a subsequent mutation', async (t) => { +test('startup effects read the latest file including a mutation after recovery', async (t) => { const { path, h, effects, store, observed } = await realStore(t); await writeFile(path, ''); await store.update({ personalization: { uiLocale: 'zh-TW' } }); - h.reporter.setEffects(effects); await turn(); await effects.refresh(true); assert.ok(observed.every((locale) => locale === 'zh-TW')); @@ -204,7 +249,6 @@ test('published reset failure is independently reported and consumers reread wit const { root, path, h, effects, store, observed } = await realStore(t); await store.update({ personalization: { uiLocale: 'zh-CN' } }); await effects.refresh(false); - h.reporter.setEffects(effects); await writeFile(path, '{bad'); const originalOpen = fs.open; let directoryCount = 0; diff --git a/apps/desktop/src/main/client-settings-effects.ts b/apps/desktop/src/main/client-settings-effects.ts index 75e744616e..33d37cf737 100644 --- a/apps/desktop/src/main/client-settings-effects.ts +++ b/apps/desktop/src/main/client-settings-effects.ts @@ -25,6 +25,8 @@ import { import { isDarkAppearance } from './theme-source.js'; import type { SettingsStore } from '@maka/storage/settings-store'; +/** Returns true when a settings snapshot/effect changes or an event is emitted. + * A silent refresh can return true; this is not a renderer acknowledgment. */ export interface ClientSettingsEffects { apply(settings: AppSettings, notifyRenderer: boolean): Promise; refresh(notifyRenderer: boolean): Promise; @@ -43,7 +45,8 @@ interface ClientSettingsEffectDependencies { */ readonly systemPrefersDark: () => boolean; readonly observeLocale: (settings: AppSettings) => void; - readonly emitExternalChanged: () => void; + /** False means no live renderer accepted an IPC send (not a delivery ack). */ + readonly emitExternalChanged: () => boolean; } export function createClientSettingsEffects( @@ -69,6 +72,7 @@ export function createClientSettingsEffects( const nextRendererFingerprint = JSON.stringify(settings); const nextBotFingerprint = JSON.stringify(settings.botChat); const settingsChanged = nextRendererFingerprint !== appliedSettingsFingerprint; + const firstSnapshot = appliedSettingsFingerprint === undefined; const rendererChanged = nextRendererFingerprint !== rendererFingerprint; const keepAwakeChanged = settings.system.keepSystemAwake !== keepSystemAwake; const botChanged = nextBotFingerprint !== botFingerprint; @@ -102,14 +106,15 @@ export function createClientSettingsEffects( appIcon = nextAppIcon; } appliedSettingsFingerprint = nextRendererFingerprint; - const rendererNotified = notifyRenderer && rendererChanged; - // A silent refresh may apply recovered settings before the recovery - // callback runs. Only an actual delivery consumes the renderer change. - if (rendererNotified) { - dependencies.emitExternalChanged(); - rendererFingerprint = nextRendererFingerprint; - } - return settingsChanged || rendererNotified || keepAwakeChanged || botChanged || appIconChanged; + // The renderer loads initial settings itself. Establish that baseline + // without making an unchanged first watcher pass emit an extra event. + if (firstSnapshot && !notifyRenderer) rendererFingerprint = nextRendererFingerprint; + // A theme-triggered silent refresh can observe a later file change before + // the watcher. Preserve that change until a live renderer accepts a send; + // successful IPC emission does not acknowledge renderer handling. + const rendererEmitted = notifyRenderer && rendererChanged && dependencies.emitExternalChanged(); + if (rendererEmitted) rendererFingerprint = nextRendererFingerprint; + return settingsChanged || rendererEmitted || keepAwakeChanged || botChanged || appIconChanged; }); tail = run.then( () => undefined, diff --git a/apps/desktop/src/main/early-window.ts b/apps/desktop/src/main/early-window.ts index 858d515c7a..4684d41a17 100644 --- a/apps/desktop/src/main/early-window.ts +++ b/apps/desktop/src/main/early-window.ts @@ -58,6 +58,7 @@ import { showMessageBoxWithDiagnostics, } from "./native-diagnostic-dialog.js"; import { resolveShellEnv } from "./shell-env.js"; +import { showNativeNotification } from "./native-notification.js"; import { createSettingsRecoveryReporter } from "./settings-recovery.js"; import { isIsolatedE2e, revealMode } from "./startup-context.js"; import { resolveDesktopStorageRoot } from "./storage-root-startup.js"; @@ -176,16 +177,31 @@ if (!resolvedLocalStorageRoot) { throw new Error("Desktop storage root resolution did not complete"); } export const startupLocalStorageRoot = resolvedLocalStorageRoot; +let allowStandaloneRecoveryNotice = false; export const settingsRecovery = createSettingsRecoveryReporter({ e2e: isIsolatedE2e, locale: () => resolveSystemUiLocale(app.getPreferredSystemLanguages()), notifications: { isSupported: () => Notification.isSupported(), - create: (copy, failed) => { - const notification = new Notification(copy); - notification.on('failed', failed); - return notification; - }, + show: (copy, failed) => showNativeNotification(copy, () => { + void quitCoordinator.focusOrCreateWindow(); + }, failed), + }, + showNotice: async (copy) => { + const window = mainWindowController.browserWindow(); + const usableWindow = window && (isIsolatedE2e || (window.isVisible() && !window.isMinimized())); + if (!usableWindow && !allowStandaloneRecoveryNotice) return false; + await showDesktopMessageBox({ + type: "warning", + title: copy.title, + message: copy.message, + detail: copy.detail, + buttons: [copy.acknowledge], + defaultId: 0, + cancelId: 0, + noLink: true, + }); + return true; }, log: (message) => console.warn(message), }); @@ -229,9 +245,15 @@ export const mainWindowController = createMainWindowController({ onWindowConstructed: () => { // `ready-to-show` is the first painted frame — the point after which the // Runtime Host module graph may evaluate without starving the paint. - mainWindowController - .browserWindow() - ?.once('ready-to-show', resolveFirstWindowConstructed); + allowStandaloneRecoveryNotice = false; + const window = mainWindowController.browserWindow(); + window?.once('ready-to-show', () => { + resolveFirstWindowConstructed(); + settingsRecovery.onWindowReady(); + }); + // A recovery can happen while the app window is closed or minimized. + window?.on('show', settingsRecovery.onWindowReady); + window?.on('restore', settingsRecovery.onWindowReady); }, onClose: () => mainWindowDelegates.onMainWindowClose(), onClosed: () => mainWindowDelegates.onMainWindowClosed(), @@ -277,8 +299,14 @@ export const quitCoordinator = createAppQuitCoordinator({ }, onCleanupError: (error) => console.error("[runtime-host] shutdown failed:", error), - onWindowCreationError: (error) => - console.error("[window] creation failed:", error), + onWindowCreationError: (error) => { + console.error("[window] creation failed:", error); + // A published reset can throw commit-unknown during the window's first + // settings read. Preserve that failure and present its guidance even when + // there is no main window to parent the dialog; never replay the operation. + allowStandaloneRecoveryNotice = true; + settingsRecovery.onWindowReady(); + }, resumeQuit: () => app.quit(), }); app.on("before-quit", quitCoordinator.handleBeforeQuit); @@ -287,6 +315,7 @@ app.on("before-quit", quitCoordinator.handleBeforeQuit); // commit could race ahead of target wiring and leave the window hidden. ipcMain.handle("window:notifyRendererReady", (event): void => { mainWindowController.notifyRendererReady(event.sender, event.senderFrame); + if (mainWindowController.isMainRenderer(event.sender)) settingsRecovery.onWindowReady(); }); // The renderer's invoke gate: it resolves when the Runtime Host boot module's // registration pass has run, so a renderer call that lands while the heavy diff --git a/apps/desktop/src/main/main-window.ts b/apps/desktop/src/main/main-window.ts index 37f7f94a11..8277d6cfd8 100644 --- a/apps/desktop/src/main/main-window.ts +++ b/apps/desktop/src/main/main-window.ts @@ -144,13 +144,19 @@ function ownsRenderer(contents: Electron.WebContents): boolean { } let browserViews: BrowserViewManager | undefined; -/** Broadcast existing app events once to each live owned renderer, even if the main window is closed. */ -export function safeSendToRenderer(channel: string, ...args: unknown[]): void { +/** Broadcast once to each live owned renderer, even if the main window is closed. + * Returns whether at least one IPC send was issued, not a renderer acknowledgment. */ +export function safeSendToRenderer(channel: string, ...args: unknown[]): boolean { const recipients = new Set(auxiliaryRenderers.keys()); if (mainWindow && !mainWindow.isDestroyed()) recipients.add(mainWindow.webContents); + let emitted = false; for (const contents of recipients) { - if (!contents.isDestroyed()) contents.send(channel, ...args); + if (!contents.isDestroyed()) { + contents.send(channel, ...args); + emitted = true; + } } + return emitted; } // The close button's centre sits on the same vertical line as the sidebar's diff --git a/apps/desktop/src/main/native-notification.ts b/apps/desktop/src/main/native-notification.ts new file mode 100644 index 0000000000..8c8a0c76db --- /dev/null +++ b/apps/desktop/src/main/native-notification.ts @@ -0,0 +1,32 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Notification } from 'electron'; + +/** Keep app notifications consistent when users click an OS banner. */ +export function showNativeNotification( + copy: { title: string; body: string }, + focusWindow: () => void, + failed?: () => void, +): void { + const notification = new Notification({ title: copy.title, body: copy.body }); + notification.on('click', focusWindow); + if (failed) notification.on('failed', failed); + notification.show(); +} diff --git a/apps/desktop/src/main/notifications-main.ts b/apps/desktop/src/main/notifications-main.ts index 7ec306a520..f0a4f622ac 100644 --- a/apps/desktop/src/main/notifications-main.ts +++ b/apps/desktop/src/main/notifications-main.ts @@ -20,6 +20,7 @@ import { app, Notification } from 'electron'; import type { AppSettings } from '@maka/core/settings'; import type { createMainWindowController } from './main-window.js'; +import { showNativeNotification } from './native-notification.js'; import type { DesktopLocaleAuthority } from './desktop-locale-authority.js'; import { type RunNotificationEvent, @@ -55,11 +56,7 @@ export function createRunNotifier(deps: NotificationsDeps): (input: RunNotificat input, deps.locale.observe(settings), ); - const notification = new Notification({ title: copy.title, body: copy.body }); - // Clicking the banner should pull the (unfocused/minimized) window - // back to the foreground — `focus()` already restores + shows. - notification.on('click', () => deps.mainWindowController.focus()); - notification.show(); + showNativeNotification(copy, () => deps.mainWindowController.focus()); app.dock?.bounce('informational'); }); } diff --git a/apps/desktop/src/main/runtime-host-boot.ts b/apps/desktop/src/main/runtime-host-boot.ts index 2b987831aa..f5dd3b5428 100644 --- a/apps/desktop/src/main/runtime-host-boot.ts +++ b/apps/desktop/src/main/runtime-host-boot.ts @@ -92,7 +92,6 @@ import { mainWindowController, mainWindowDelegates, quitCoordinator, - settingsRecovery, settingsStore, shellEnvReady, showDesktopMessageBox, @@ -180,6 +179,7 @@ import { import { registerRuntimeHostConfigIpc } from "./runtime-host-config-ipc-main.js"; import { createCapabilityRevisionPublisher } from "./runtime-host-capability-revision-publisher.js"; import { buildClientSettingsTools } from "./client-settings-tools.js"; +import { safeSendToRenderer } from "./main-window.js"; import { createClientSettingsEffects } from "./client-settings-effects.js"; import { registerClientSettingsIpc } from "./client-settings-ipc-main.js"; import { startClientSettingsWatcher } from "./client-settings-watcher.js"; @@ -849,11 +849,11 @@ const clientSettingsEffects = createClientSettingsEffects({ systemPrefersDark: () => nativeTheme.shouldUseDarkColors, observeLocale: (settings) => desktopLocale.observe(settings), emitExternalChanged: () => { - mainWindowController.send("settings:clientChanged"); + const emitted = safeSendToRenderer("settings:clientChanged"); sendActiveRuntimeHostEvent("settings:externalChanged", { ts: Date.now() }); + return emitted; }, }); -settingsRecovery.setEffects(clientSettingsEffects); // An OS appearance flip changes no setting, so nothing else would notice it. // Only the icon depends on the answer, and `refresh` re-resolves it and // no-ops when the resolved tile is the one already applied — which is the diff --git a/apps/desktop/src/main/settings-recovery.ts b/apps/desktop/src/main/settings-recovery.ts index 65e61239af..8f2a151a77 100644 --- a/apps/desktop/src/main/settings-recovery.ts +++ b/apps/desktop/src/main/settings-recovery.ts @@ -19,77 +19,75 @@ import type { UiCatalog, UiLocale } from '@maka/core/ui-locale'; import type { CorruptSettingsRecovery } from '@maka/storage/settings-store'; -import type { ClientSettingsEffects } from './client-settings-effects.js'; interface RecoveryCopy { title: string; + message: string; body: string; + detail: string; + acknowledge: string; } -type RecoveryStatus = CorruptSettingsRecovery['outcome'] | 'refresh-failed'; - const COPY = { 'zh-CN': { recovered: { title: '设置已恢复为默认值', - body: '设置文件损坏，已恢复默认设置。请检查隐身模式、隐私和其他偏好。', + body: '设置文件不是有效的 JSON，已恢复默认设置。', }, 'commit-unknown': { title: '设置已重置，保存状态待确认', - body: '默认设置已写入，但磁盘同步失败，持久保存状态尚未确认。请检查隐身模式和隐私设置。', - }, - 'refresh-failed': { - title: '运行中的设置刷新失败', - body: '设置文件已重置，但运行中的设置未能全部刷新。请重启应用并检查隐私设置。', + body: '默认设置已写入，但磁盘同步失败，持久保存状态尚未确认。请重启应用并检查设置。', }, + review: '偏好、机器人配置和首次使用设置均已重置。请检查隐身模式、隐私和其他偏好；如需找回原配置，请先退出 Maka，再检查下面的原始文件备份。', + settings: '设置文件：', backup: '原始文件备份：', + acknowledge: '知道了', }, 'zh-TW': { recovered: { title: '設定已還原為預設值', - body: '設定檔案損毀，已還原預設設定。請檢查無痕模式、隱私與其他偏好。', + body: '設定檔案不是有效的 JSON，已還原預設設定。', }, 'commit-unknown': { title: '設定已重設，儲存狀態待確認', - body: '預設設定已寫入，但磁碟同步失敗，尚未確認是否持久儲存。請檢查無痕模式與隱私設定。', - }, - 'refresh-failed': { - title: '執行中的設定更新失敗', - body: '設定檔案已重設，但執行中的設定未能全部更新。請重新啟動應用程式並檢查隱私設定。', + body: '預設設定已寫入，但磁碟同步失敗，尚未確認是否持久儲存。請重新啟動應用程式並檢查設定。', }, + review: '偏好、機器人設定與首次使用設定均已重設。請檢查無痕模式、隱私與其他偏好；如需找回原設定，請先結束 Maka，再檢查下方的原始檔案備份。', + settings: '設定檔案：', backup: '原始檔案備份：', + acknowledge: '知道了', }, en: { recovered: { title: 'Settings restored to defaults', - body: 'The settings file was invalid and has been reset. Please review Incognito, privacy and other preferences.', + body: 'The settings file contained invalid JSON and has been reset to defaults.', }, 'commit-unknown': { title: 'Settings reset; save status uncertain', - body: 'Default settings were written, but disk synchronization failed and durability is unconfirmed. Please review Incognito and privacy settings.', - }, - 'refresh-failed': { - title: 'Running settings could not be refreshed', - body: 'The settings file was reset, but some running settings could not be refreshed. Restart the app and review your privacy settings.', + body: 'Default settings were written, but disk synchronization failed and durability is unconfirmed. Restart Maka and check your settings.', }, + review: 'Preferences, bot configuration and onboarding settings have been reset. Review Incognito, privacy and other preferences. To recover your previous configuration, quit Maka first, then inspect the original file backup below.', + settings: 'Settings file: ', backup: 'Original file backup: ', + acknowledge: 'OK', }, -} satisfies UiCatalog & { backup: string }>; +} satisfies UiCatalog & { + review: string; settings: string; backup: string; acknowledge: string; +}>; export function settingsRecoveryCopy( recovery: CorruptSettingsRecovery, locale: UiLocale, - refreshFailed = false, ): RecoveryCopy { const copy = COPY[locale]; - const message = copy[refreshFailed ? 'refresh-failed' : recovery.outcome]; - // Preserve the durability warning when reporting a separate refresh failure. - const warning = refreshFailed && recovery.outcome === 'commit-unknown' - ? ` ${copy['commit-unknown'].body}` - : ''; + const message = copy[recovery.outcome]; return { title: message.title, - body: `${message.body}${warning} ${copy.backup}${recovery.backupPath}`, + message: message.body, + // Native banners can truncate their body; put the actionable path first. + body: `${copy.backup}${recovery.backupPath}\n${message.body}`, + detail: `${copy.review}\n\n${copy.settings}${recovery.settingsPath}\n${copy.backup}${recovery.backupPath}`, + acknowledge: copy.acknowledge, }; } @@ -98,23 +96,26 @@ interface SettingsRecoveryReporterDependencies { readonly locale: () => UiLocale; readonly notifications: { isSupported(): boolean; - create(copy: RecoveryCopy, failed: () => void): { show(): void }; + show(copy: Pick, failed: () => void): void; }; + /** False leaves the notice pending until a usable window becomes available. + * Resolves true only after the app's persistent dialog has been dismissed. */ + readonly showNotice: (copy: RecoveryCopy) => Promise; /** Receives only the result and paths, never JSON contents or parser errors. */ readonly log: (message: string) => void; } -/** Observes recovery without becoming a second settings authority. Notification - * delivery is best-effort; refresh reuses the existing effects queue and rereads - * the file after the storage operation releases its own queue. */ +/** Keeps recovery guidance until the app can display it. The existing startup + * and file-watcher paths remain the only settings-effect refresh authorities. */ export function createSettingsRecoveryReporter(deps: SettingsRecoveryReporterDependencies) { - let effects: Pick | undefined; - let pending: CorruptSettingsRecovery | undefined; + const pending: CorruptSettingsRecovery[] = []; + let presenting = false; + let presentationRequested = false; const log = (message: string): void => { try { deps.log(message); } catch { /* Diagnostics must not block recovery. */ } }; - const notify = (recovery: CorruptSettingsRecovery, refreshFailed = false): void => { + const notify = (recovery: CorruptSettingsRecovery): void => { if (deps.e2e) return; const failed = () => log(`[settings-recovery] notification failed; backup=${recovery.backupPath}`); try { @@ -122,34 +123,43 @@ export function createSettingsRecoveryReporter(deps: SettingsRecoveryReporterDep log(`[settings-recovery] notifications unavailable; backup=${recovery.backupPath}`); return; } - deps.notifications.create(settingsRecoveryCopy(recovery, deps.locale(), refreshFailed), failed).show(); + deps.notifications.show(settingsRecoveryCopy(recovery, deps.locale()), failed); } catch { failed(); } }; - const refresh = (): void => { - const target = effects; - const recovery = pending; - if (!target || !recovery) return; - pending = undefined; - // Never await this from onRecovery: refresh can itself be the read that - // discovers corruption, and both storage and effects serialize operations. - void Promise.resolve().then(() => target.refresh(true)).catch(() => { - log(`[settings-recovery] refresh failed; outcome=${recovery.outcome}; backup=${recovery.backupPath}; restart the app`); - notify(recovery, true); + const present = (): void => { + if (pending.length === 0) return; + presentationRequested = true; + if (presenting) return; + presenting = true; + // The dialog's theme lookup can read settings. Start after the recovery + // observer returns, without waiting inside the serialized storage read. + void Promise.resolve().then(async () => { + while (pending.length > 0) { + presentationRequested = false; + const recovery = pending[0]; + try { + if (!await deps.showNotice(settingsRecoveryCopy(recovery, deps.locale()))) return; + } catch { + log(`[settings-recovery] app notice failed; backup=${recovery.backupPath}`); + return; + } + pending.shift(); + } + }).finally(() => { + presenting = false; + if (presentationRequested) present(); }); }; return { onRecovery(recovery: CorruptSettingsRecovery): void { log(`[settings-recovery] outcome=${recovery.outcome}; settings=${recovery.settingsPath}; backup=${recovery.backupPath}`); + pending.push(recovery); notify(recovery); - pending = recovery; - refresh(); - }, - setEffects(value: Pick): void { - effects = value; - refresh(); + present(); }, + onWindowReady: present, }; } diff --git a/docs/windows-test-inventory.md b/docs/windows-test-inventory.md index a0007444e1..cce5f657f6 100644 --- a/docs/windows-test-inventory.md +++ b/docs/windows-test-inventory.md @@ -16,10 +16,10 @@ Locations intentionally omit line numbers so unrelated edits do not invalidate t | Classification | Count | |---|---:| | windows-backend-gap | 27 | -| portable-candidate | 45 | +| portable-candidate | 51 | | platform-contract | 38 | -Total Windows-excluded declarations: **110** +Total Windows-excluded declarations: **116** ## Inventory @@ -34,6 +34,7 @@ Total Windows-excluded declarations: **110** | portable-candidate | `apps/desktop/src/main/__tests__/opencli-chrome.test.ts` Windows opens the store page in installed Chrome, not the default browser | `process.platform === 'win32'` | | platform-contract | `apps/desktop/src/main/__tests__/project-context-root.test.ts` rejects a session cwd without read and traversal access | `process.platform === 'win32' ? 'POSIX permissions are required to make the session cwd inaccessible' : process.getuid?.() === 0` | | platform-contract | `apps/desktop/src/main/__tests__/runtime-host-skills-ipc-main.test.ts` reports create_failed without opening when a Skill directory parent is not writable | `process.platform === 'win32' ? 'POSIX permissions are required to make the Skill directory parent read-only' : process.getuid?.() === 0` | +| portable-candidate | `apps/desktop/src/main/__tests__/settings-recovery-startup.test.ts` early-window presents recovery guidance despite ${nativeFailure} | `nativeFailure === 'commit-unknown' && process.platform === 'win32'` | | portable-candidate | `apps/desktop/src/main/__tests__/settings-recovery.test.ts` published reset failure is independently reported and consumers reread without replaying a mutation | `process.platform === 'win32'` | | platform-contract | `apps/desktop/src/main/__tests__/shell-env.test.ts` imports the login PATH without importing application control variables | `process.platform === 'win32'` | | platform-contract | `apps/desktop/src/main/__tests__/shell-env.test.ts` keeps the inherited PATH and does not log shell stderr when capture fails | `process.platform === 'win32'` | @@ -122,6 +123,11 @@ Total Windows-excluded declarations: **110** | platform-contract | `packages/storage/src/__tests__/runtime-policy-stores.test.ts` successor recovery removes credentials orphaned by an interrupted connection removal | `process.platform === 'win32' ? 'POSIX permissions are required to inject a persistence failure' : false` | | platform-contract | `packages/storage/src/__tests__/runtime-policy-stores.test.ts` fails closed on final symlinks, FIFOs, and oversized documents without changing bytes | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/settings-store-onboarding.test.ts` preserves a restrictive umask-derived settings.json mode and leaves no temp file behind | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/settings-store-recovery.test.ts` corrupt settings behind a symlink are not reset and the link survives | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/settings-store-recovery.test.ts` a valid settings symlink keeps its existing read behavior | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/settings-store-recovery.test.ts` a symlink installed during temp preparation is not replaced or followed for recovery | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/settings-store-recovery.test.ts` a completed backup survives directory sync failure and is fenced again before reuse | `process.platform === 'win32'` | +| portable-candidate | `packages/storage/src/__tests__/settings-store-recovery.test.ts` a ${unsafe} backup candidate is never reused or replaced | `process.platform === 'win32' && (unsafe === 'public mode' \|\| unsafe === 'hard link')` | | portable-candidate | `packages/storage/src/__tests__/settings-store-recovery.test.ts` private backups do not change the normal settings umask policy | `process.platform === 'win32'` | | portable-candidate | `packages/storage/src/__tests__/settings-store-recovery.test.ts` backup ${phase} failure preserves source and reports the original cause | `process.platform === 'win32' && (phase === 'chmod' \|\| phase === 'directory')` | | portable-candidate | `packages/storage/src/__tests__/settings-store-recovery.test.ts` refuses a preexisting backup ${planted} without deleting it | `process.platform === 'win32' && planted === 'symlink'` | diff --git a/packages/storage/src/__tests__/atomic-file-write.test.ts b/packages/storage/src/__tests__/atomic-file-write.test.ts index 9427351753..55dabd20e4 100644 --- a/packages/storage/src/__tests__/atomic-file-write.test.ts +++ b/packages/storage/src/__tests__/atomic-file-write.test.ts @@ -52,6 +52,49 @@ async function withTempDir(fn: (dir: string) => Promise): Promise { } describe('writeAtomicFile', () => { + test('checks publication preconditions after closing the prepared temp and cleans up on rejection', async () => { + await withTempDir(async (dir) => { + const path = join(dir, 'settings.json'); + const temporaryPath = join(dir, '.settings.json.guarded.tmp'); + await writeFile(path, 'old'); + let closed = false; + const fault = new Error('source changed'); + await assert.rejects( + writeAtomicFile( + path, + 'new', + { + fileMode: 0o600, + beforePublish: async () => { + assert.equal(closed, true); + assert.equal(await readFile(temporaryPath, 'utf8'), 'new'); + assert.equal(await readFile(path, 'utf8'), 'old'); + throw fault; + }, + }, + { + randomUUID: () => 'guarded', + open: async (...args) => { + const handle = await open(...args); + return { + writeFile: handle.writeFile.bind(handle), + chmod: handle.chmod.bind(handle), + sync: handle.sync.bind(handle), + close: async () => { + await handle.close(); + closed = true; + }, + }; + }, + }, + ), + fault, + ); + assert.equal(await readFile(path, 'utf8'), 'old'); + assert.deepEqual(await readdir(dir), ['settings.json']); + }); + }); + test('writes the exact bytes and leaves no temp file behind', async () => { await withTempDir(async (dir) => { const path = join(dir, 'settings.json'); diff --git a/packages/storage/src/__tests__/settings-store-onboarding.test.ts b/packages/storage/src/__tests__/settings-store-onboarding.test.ts index d15fbd00be..620505f416 100644 --- a/packages/storage/src/__tests__/settings-store-onboarding.test.ts +++ b/packages/storage/src/__tests__/settings-store-onboarding.test.ts @@ -340,6 +340,7 @@ describe('SettingsStore.get file recovery', () => { assert.equal(settings.schemaVersion, 1); assert.match(raw, /"schemaVersion": 1/); + assert.deepEqual(await readdir(workspaceRoot), ['settings.json']); } finally { await rm(workspaceRoot, { recursive: true, force: true }); } diff --git a/packages/storage/src/__tests__/settings-store-recovery.test.ts b/packages/storage/src/__tests__/settings-store-recovery.test.ts index e6ed242a4b..d33811bc14 100644 --- a/packages/storage/src/__tests__/settings-store-recovery.test.ts +++ b/packages/storage/src/__tests__/settings-store-recovery.test.ts @@ -18,8 +18,11 @@ */ import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import { createHash } from 'node:crypto'; import fs, { chmod, + lstat, mkdtemp, readFile, readdir, @@ -42,7 +45,8 @@ import { } from '../settings-store.js'; import { AtomicFileWriteCommitUnknownError } from '../atomic-file-write.js'; -const corrupt = Buffer.from('{"appearance":{"theme":"dark"},"secret":"do-not-log"'); +const secret = 'sk-live-SECRET'; +const corrupt = Buffer.from(`{"a":${secret}}`); const turn = () => new Promise((resolve) => setImmediate(resolve)); async function fixture(t: TestContext, options?: SettingsStoreOptions) { @@ -66,10 +70,449 @@ async function fixture(t: TestContext, options?: SettingsStoreOptions) { return { root, path, events, store }; } +async function changeDuringTempWrite(t: TestContext, root: string, change: () => Promise) { + // The shared writer captures its default open dependency at module load. + // Intercept the handle method so the edit occurs inside the real temp write. + const probePath = join(root, 'handle-probe'); + const probe = await fs.open(probePath, 'wx'); + const prototype = Object.getPrototypeOf(probe); + const originalWrite = probe.writeFile; + await probe.close(); + await rm(probePath); + t.mock.method( + prototype, + 'writeFile', + async function (this: typeof probe, ...args: Parameters) { + await originalWrite.apply(this, args); + if (typeof args[0] === 'string' && args[1] === 'utf8') await change(); + }, + ); +} + +test('the invalid-token fixture would expose its secret if a parser error escaped', () => { + assert.throws( + () => JSON.parse(corrupt.toString('utf8')), + (error) => { + assert.ok(error instanceof SyntaxError); + assert.ok(error.message.includes(secret)); + return true; + }, + ); +}); + +test('UTF-8 BOM settings are read without resetting or rewriting the file', async (t) => { + const { path, root, store, events } = await fixture(t); + const bytes = Buffer.from('\uFEFF{"appearance":{"theme":"dark"}}'); + await writeFile(path, bytes); + assert.equal((await store.get()).appearance.theme, 'dark'); + assert.deepEqual(await readFile(path), bytes); + assert.deepEqual(await readdir(root), ['settings.json']); + assert.deepEqual(events, []); +}); + +for (const bigEndian of [false, true]) { + test(`UTF-16 ${bigEndian ? 'BE' : 'LE'} settings require UTF-8 conversion without resetting`, async (t) => { + const { path, root, store, events } = await fixture(t); + const bytes = Buffer.from('\uFEFF{"appearance":{"theme":"dark"}}', 'utf16le'); + if (bigEndian) bytes.swap16(); + await writeFile(path, bytes); + await assert.rejects(store.get(), /Save the file as UTF-8/); + assert.deepEqual(await readFile(path), bytes); + assert.deepEqual(await readdir(root), ['settings.json']); + assert.deepEqual(events, []); + }); +} + +test('corrupt settings behind a symlink are not reset and the link survives', { + skip: process.platform === 'win32', +}, async (t) => { + const { path, root, store, events } = await fixture(t); + const target = join(root, 'linked-settings.json'); + await fs.rename(path, target); + await symlink(target, path); + await assert.rejects(store.get(), /symbolic-link target manually/); + assert.equal((await lstat(path)).isSymbolicLink(), true); + assert.deepEqual(await readFile(target), corrupt); + assert.equal((await readdir(root)).length, 2); + assert.deepEqual(events, []); +}); + +test('a valid settings symlink keeps its existing read behavior', { + skip: process.platform === 'win32', +}, async (t) => { + const { path, root, store, events } = await fixture(t); + const target = join(root, 'linked-settings.json'); + const text = '{"appearance":{"theme":"dark"}}'; + await writeFile(target, text); + await rm(path); + await symlink(target, path); + assert.equal((await store.get()).appearance.theme, 'dark'); + assert.equal((await lstat(path)).isSymbolicLink(), true); + assert.equal(await readFile(target, 'utf8'), text); + assert.deepEqual(events, []); +}); + +test('a symlink installed during temp preparation is not replaced or followed for recovery', { + skip: process.platform === 'win32', +}, async (t) => { + const { path, root, store, events } = await fixture(t); + const target = join(root, 'linked-settings.json'); + const text = '{"appearance":{"theme":"dark"}}'; + await writeFile(target, text); + await changeDuringTempWrite(t, root, async () => { + await rm(path); + await symlink(target, path); + }); + syncBuiltinESMExports(); + await assert.rejects( + store.get(), + (error) => + error instanceof SettingsRecoveryError && + /symbolic-link target manually/.test(String(error.cause)), + ); + assert.equal((await lstat(path)).isSymbolicLink(), true); + assert.equal(await readFile(target, 'utf8'), text); + assert.equal( + (await readdir(root)).some((name) => name.endsWith('.tmp')), + false, + ); + assert.deepEqual(events, []); +}); + +for (const stage of ['initial read', 'backup write', 'temp write'] as const) { + test(`a repair of the same byte length during ${stage} is used without publishing defaults`, async (t) => { + const { path, root, store, events } = await fixture(t); + const repaired = '{"appearance":{"theme":"dark"}}'; + await writeFile(path, corrupt.toString('utf8').padEnd(Buffer.byteLength(repaired), ' ')); + if (stage === 'initial read') { + const originalRead = fs.readFile; + t.mock.method( + fs, + 'readFile', + async (...args: Parameters) => { + const result = await originalRead(...args); + if (args[0] === path) await writeFile(path, repaired); + return result; + }, + { times: 1 }, + ); + } else if (stage === 'temp write') { + await changeDuringTempWrite(t, root, () => writeFile(path, repaired)); + } else { + const originalOpen = fs.open; + t.mock.method(fs, 'open', async (...args: Parameters) => { + const handle = await originalOpen(...args); + if ( + stage === 'backup write' && + String(args[0]).startsWith(`${path}.corrupt-`) && + args[1] === 'wx' + ) { + const originalWrite = handle.writeFile.bind(handle); + t.mock.method( + handle, + 'writeFile', + async (...writeArgs: Parameters) => { + await originalWrite(...writeArgs); + await writeFile(path, repaired); + }, + ); + } + return handle; + }); + } + syncBuiltinESMExports(); + assert.equal((await store.get()).appearance.theme, 'dark'); + assert.equal(await readFile(path, 'utf8'), repaired); + assert.deepEqual(events, []); + const files = await readdir(root); + assert.equal( + files.some((name) => name.endsWith('.tmp')), + false, + ); + assert.equal(files.length, stage === 'initial read' ? 1 : 2); + }); +} + +for (const change of ['remove', 'another corrupt value', 'same bytes in a new inode'] as const) { + test(`a concurrent ${change} aborts the current recovery without retrying`, async (t) => { + const { path, root, store, events } = await fixture(t); + await changeDuringTempWrite(t, root, async () => { + await rm(path); + if (change !== 'remove') + await writeFile(path, change === 'another corrupt value' ? '{' : corrupt); + }); + syncBuiltinESMExports(); + await assert.rejects( + store.get(), + change === 'remove' ? /reset failed/ : /changed during recovery/, + ); + if (change === 'remove') await assert.rejects(readFile(path), { code: 'ENOENT' }); + else + assert.deepEqual( + await readFile(path), + change === 'another corrupt value' ? Buffer.from('{') : corrupt, + ); + const files = await readdir(root); + assert.equal(files.filter((name) => name.startsWith('settings.json.corrupt-')).length, 1); + assert.equal( + files.some((name) => name.endsWith('.tmp')), + false, + ); + assert.deepEqual(events, []); + }); +} + +test('recovery survives successive process exits during backup writes', async (t) => { + const { path, root, store, events } = await fixture(t); + const interruptedBackups: string[] = []; + for (const length of [0, 3]) { + // Exit without unwinding the store's cleanup, as a terminated process would. + // Exercise both an empty exclusive creation and a partially written backup. + const child = spawnSync( + process.execPath, + [ + '--input-type=module', + '-e', + ` + import fs from 'node:fs/promises'; + import { syncBuiltinESMExports } from 'node:module'; + import { createSettingsStore } from ${JSON.stringify(new URL('../settings-store.js', import.meta.url).href)}; + const open = fs.open; + fs.open = async (...args) => { + const handle = await open(...args); + if (String(args[0]).startsWith(${JSON.stringify(`${path}.corrupt-`)}) && args[1] === 'wx') { + handle.writeFile = async (bytes) => { + await handle.write(bytes.subarray(0, ${length})); + await handle.sync(); + process.exit(73); + }; + } + return handle; + }; + syncBuiltinESMExports(); + await createSettingsStore(${JSON.stringify(root)}).get(); + `, + ], + { encoding: 'utf8', timeout: 5_000 }, + ); + assert.equal(child.error, undefined); + assert.equal(child.status, 73, child.stderr); + assert.deepEqual(await readFile(path), corrupt); + const backup = (await readdir(root)).find( + (name) => name.startsWith('settings.json.corrupt-') && !interruptedBackups.includes(name), + ); + assert.ok(backup); + interruptedBackups.push(backup); + assert.deepEqual(await readFile(join(root, backup)), corrupt.subarray(0, length)); + } + + assert.deepEqual(await store.get(), createDefaultSettings()); + assert.equal(events.length, 1); + assert.deepEqual(await readFile(events[0].backupPath), corrupt); + assert.deepEqual(JSON.parse(await readFile(path, 'utf8')), createDefaultSettings()); + assert.deepEqual(await readFile(join(root, interruptedBackups[0])), Buffer.alloc(0)); + assert.deepEqual(await readFile(join(root, interruptedBackups[1])), corrupt.subarray(0, 3)); + assert.equal((await readdir(root)).length, 4); +}); + +test('failed resets reuse one complete backup across calls and recreated store instances', async (t) => { + const { path, root, store, events } = await fixture(t); + const failure = Object.assign(new Error('rename failed'), { code: 'EPERM' }); + const originalRename = fs.rename; + t.mock.method(fs, 'rename', async (...args: Parameters) => { + if (args[1] === path) throw failure; + return originalRename(...args); + }); + syncBuiltinESMExports(); + const backups = new Set(); + for (const current of [store, store, createSettingsStore(root), createSettingsStore(root)]) { + await assert.rejects(current.get(), (error) => { + assert.ok(error instanceof SettingsRecoveryError); + assert.equal(error.phase, 'reset'); + assert.equal(error.cause, failure); + backups.add(error.backupPath!); + return true; + }); + } + assert.equal(backups.size, 1); + assert.equal((await readdir(root)).length, 2); + assert.deepEqual(await readFile([...backups][0]), corrupt); + assert.deepEqual(events, []); +}); + +test('failed resets reuse an alternative backup without replacing an interrupted write', async (t) => { + const { path, root, store, events } = await fixture(t); + const incomplete = `${path}.corrupt-${createHash('sha256').update(corrupt).digest('hex')}`; + await writeFile(incomplete, corrupt.subarray(0, 3), { mode: 0o600 }); + const failure = Object.assign(new Error('rename failed'), { code: 'EPERM' }); + const originalRename = fs.rename; + t.mock.method(fs, 'rename', async (...args: Parameters) => { + if (args[1] === path) throw failure; + return originalRename(...args); + }); + syncBuiltinESMExports(); + const backups = new Set(); + for (const current of [store, store, createSettingsStore(root), createSettingsStore(root)]) { + await assert.rejects(current.get(), (error) => { + assert.ok(error instanceof SettingsRecoveryError); + assert.equal(error.phase, 'reset'); + assert.equal(error.cause, failure); + assert.ok(error.backupPath); + assert.notEqual(error.backupPath, incomplete); + backups.add(error.backupPath); + return true; + }); + } + assert.equal(backups.size, 1); + assert.deepEqual(await readFile([...backups][0]), corrupt); + assert.deepEqual(await readFile(incomplete), corrupt.subarray(0, 3)); + assert.deepEqual(await readFile(path), corrupt); + assert.equal((await readdir(root)).length, 3); + assert.deepEqual(events, []); +}); + +for (const phase of ['open', 'readFile', 'sync'] as const) { + test(`an I/O failure during backup reuse (${phase}) does not create an alternative`, async (t) => { + const { path, root, store, events } = await fixture(t); + const backup = `${path}.corrupt-${createHash('sha256').update(corrupt).digest('hex')}`; + await writeFile(backup, corrupt, { mode: 0o600 }); + const failure = Object.assign(new Error(`backup reuse ${phase} failed`), { code: 'EIO' }); + const originalOpen = fs.open; + t.mock.method(fs, 'open', async (...args: Parameters) => { + const reuse = args[0] === backup && args[1] !== 'wx'; + if (reuse && phase === 'open') throw failure; + const handle = await originalOpen(...args); + if (reuse && phase !== 'open') { + t.mock.method(handle, phase, async () => { + throw failure; + }); + } + return handle; + }); + syncBuiltinESMExports(); + await assert.rejects(store.get(), (error) => { + assert.ok(error instanceof SettingsRecoveryError); + assert.equal(error.phase, 'backup'); + assert.equal(error.cause, failure); + return true; + }); + assert.deepEqual(await readFile(path), corrupt); + assert.deepEqual(await readFile(backup), corrupt); + assert.equal((await readdir(root)).length, 2); + assert.deepEqual(events, []); + }); +} + +test('a different corruption after a failed reset gets its own backup and preserves both versions', async (t) => { + const { path, root, store, events } = await fixture(t); + const failure = Object.assign(new Error('rename failed'), { code: 'EPERM' }); + const originalRename = fs.rename; + t.mock.method(fs, 'rename', async (...args: Parameters) => { + if (args[1] === path) throw failure; + return originalRename(...args); + }); + syncBuiltinESMExports(); + const first = corrupt; + const second = Buffer.from('{"appearance":'); + const backups: string[] = []; + for (const [current, bytes] of [ + [store, first], + [store, second], + [createSettingsStore(root), second], + ] as const) { + await writeFile(path, bytes); + await assert.rejects(current.get(), (error) => { + assert.ok(error instanceof SettingsRecoveryError); + assert.equal(error.phase, 'reset'); + assert.equal(error.cause, failure); + assert.ok(error.backupPath); + backups.push(error.backupPath); + return true; + }); + assert.deepEqual(await readFile(path), bytes); + } + assert.notEqual(backups[0], backups[1], 'new source bytes need a separate backup'); + assert.equal(backups[1], backups[2], 'a new store reuses the second complete backup'); + assert.deepEqual(await readFile(backups[0]), first); + assert.deepEqual(await readFile(backups[1]), second); + assert.equal((await readdir(root)).length, 3, 'only the source and its two byte versions remain'); + assert.deepEqual(events, []); +}); + +test('a completed backup survives directory sync failure and is fenced again before reuse', { + skip: process.platform === 'win32', +}, async (t) => { + const { path, root, store } = await fixture(t); + const originalOpen = fs.open; + let directorySyncs = 0; + let backupCreates = 0; + t.mock.method(fs, 'open', async (...args: Parameters) => { + const handle = await originalOpen(...args); + if (String(args[0]).startsWith(`${path}.corrupt-`) && args[1] === 'wx') backupCreates += 1; + if (args[0] === root) { + const originalSync = handle.sync.bind(handle); + t.mock.method(handle, 'sync', async () => { + if (++directorySyncs === 1) throw new Error('directory fence failed'); + await originalSync(); + }); + } + return handle; + }); + syncBuiltinESMExports(); + let backup = ''; + await assert.rejects(store.get(), (error) => { + assert.ok(error instanceof SettingsRecoveryError); + assert.ok(error.unsyncedBackupPath); + backup = error.unsyncedBackupPath; + assert.equal(error.backupPath, undefined); + return true; + }); + assert.deepEqual(await readFile(backup), corrupt); + assert.deepEqual(await createSettingsStore(root).get(), createDefaultSettings()); + assert.equal(backupCreates, 1); + assert.equal(directorySyncs, 3); + assert.equal((await readdir(root)).length, 2); +}); + +for (const unsafe of ['tampered', 'partial', 'public mode', 'hard link'] as const) { + test(`a ${unsafe} backup candidate is never reused or replaced`, { + skip: process.platform === 'win32' && (unsafe === 'public mode' || unsafe === 'hard link'), + }, async (t) => { + const { path, root, store } = await fixture(t); + const backup = `${path}.corrupt-${createHash('sha256').update(corrupt).digest('hex')}`; + const bytes = + unsafe === 'partial' + ? corrupt.subarray(0, 3) + : unsafe === 'tampered' + ? Buffer.alloc(corrupt.length, 0x78) + : corrupt; + await writeFile(backup, bytes, { mode: 0o600 }); + if (unsafe === 'public mode') await chmod(backup, 0o644); + if (unsafe === 'hard link') await fs.link(backup, join(root, 'another-link')); + for (const current of [store, createSettingsStore(root)]) { + await writeFile(path, corrupt); + assert.deepEqual(await current.get(), createDefaultSettings()); + } + assert.deepEqual(await readFile(backup), bytes); + const backups = (await readdir(root)).filter((name) => + name.startsWith('settings.json.corrupt-'), + ); + assert.equal(backups.length, 2); + const alternative = backups.find((name) => join(root, name) !== backup); + assert.ok(alternative); + assert.deepEqual(await readFile(join(root, alternative)), corrupt); + assert.deepEqual(JSON.parse(await readFile(path, 'utf8')), createDefaultSettings()); + assert.equal((await stat(join(root, alternative))).nlink, 1); + if (process.platform !== 'win32') + assert.equal((await stat(join(root, alternative))).mode & 0o777, 0o600); + }); +} + for (const [name, bytes] of [ ['empty', Buffer.alloc(0)], - ['truncated', corrupt], - ['invalid UTF-8 and truncated', Buffer.concat([corrupt, Buffer.from([0xff, 0xc3])])], + ['truncated', Buffer.from('{"appearance":')], + ['invalid token', corrupt], + ['invalid UTF-8', Buffer.concat([corrupt, Buffer.from([0xff, 0xc3])])], ] as const) { test(`recovers ${name} settings with an exact byte backup before reporting`, async (t) => { const { root, path, store, events } = await fixture(t); @@ -78,7 +521,7 @@ for (const [name, bytes] of [ assert.equal(events.length, 1); assert.equal(events[0].settingsPath, path); assert.equal(events[0].outcome, 'recovered'); - assert.match(events[0].backupPath, /settings\.json\.corrupt-\d+-[a-f0-9-]+$/u); + assert.match(events[0].backupPath, /settings\.json\.corrupt-[a-f0-9]{64}$/u); assert.deepEqual(await readFile(events[0].backupPath), bytes); assert.deepEqual(JSON.parse(await readFile(path, 'utf8')), createDefaultSettings()); await store.get(); @@ -170,7 +613,7 @@ test('mutations use the recovered defaults and execute their own work once', asy await writeFile(path, corrupt); assert.deepEqual(await store.clearOnboardingMilestone('first_chat_sent'), []); assert.equal(events.length, 4); - assert.equal(new Set(events.map((event) => event.backupPath)).size, 4); + assert.equal(new Set(events.map((event) => event.backupPath)).size, 1); }); test('private backups do not change the normal settings umask policy', { @@ -241,11 +684,20 @@ for (const phase of ['open', 'writeFile', 'chmod', 'sync', 'close', 'directory'] assert.equal(error.settingsPath, path); assert.equal(error.backupPath, undefined); assert.equal(error.incompleteBackupPath, undefined); - assert.equal(error.message.includes('do-not-log'), false); + assert.equal(error.message.includes(secret), false); + assert.equal(error.unsyncedBackupPath !== undefined, phase === 'directory'); return true; }); assert.deepEqual(await readFile(path), corrupt); - assert.deepEqual(await readdir(root), ['settings.json']); + if (phase === 'directory') { + const backups = (await readdir(root)).filter((name) => + name.startsWith('settings.json.corrupt-'), + ); + assert.equal(backups.length, 1); + assert.deepEqual(await readFile(join(root, backups[0])), corrupt); + } else { + assert.deepEqual(await readdir(root), ['settings.json']); + } assert.deepEqual(events, []); }); } @@ -277,17 +729,39 @@ test('backup cleanup failure retains the original cause and identifies an incomp assert.deepEqual(await readFile(path), corrupt); }); +test('backup failure cleanup does not remove an externally replaced backup entry', async (t) => { + const { path, root, store } = await fixture(t); + const originalOpen = fs.open; + let backup = ''; + t.mock.method(fs, 'open', async (...args: Parameters) => { + const handle = await originalOpen(...args); + if (String(args[0]).startsWith(`${path}.corrupt-`) && args[1] === 'wx') { + backup = String(args[0]); + t.mock.method(handle, 'writeFile', async () => { + await fs.rename(backup, join(root, 'moved-backup')); + await writeFile(backup, 'externally replaced'); + throw new Error('backup write failed'); + }); + } + return handle; + }); + syncBuiltinESMExports(); + await assert.rejects(store.get(), SettingsRecoveryError); + assert.equal(await readFile(backup, 'utf8'), 'externally replaced'); + assert.deepEqual(await readFile(path), corrupt); +}); + for (const planted of ['file', 'symlink'] as const) { test(`refuses a preexisting backup ${planted} without deleting it`, { skip: process.platform === 'win32' && planted === 'symlink', }, async (t) => { - const { path, root, store } = await fixture(t); + const { path, root, store, events } = await fixture(t); const target = join(root, 'unrelated'); await writeFile(target, 'keep me'); const originalOpen = fs.open; let collision = ''; t.mock.method(fs, 'open', async (...args: Parameters) => { - if (String(args[0]).startsWith(`${path}.corrupt-`)) { + if (!collision && String(args[0]).startsWith(`${path}.corrupt-`) && args[1] === 'wx') { collision = String(args[0]); if (planted === 'file') await writeFile(collision, 'keep me'); else await symlink(target, collision); @@ -295,15 +769,13 @@ for (const planted of ['file', 'symlink'] as const) { return originalOpen(...args); }); syncBuiltinESMExports(); - await assert.rejects( - store.get(), - (error) => - error instanceof SettingsRecoveryError && - (error.cause as NodeJS.ErrnoException).code === 'EEXIST', - ); + assert.deepEqual(await store.get(), createDefaultSettings()); assert.equal(await readFile(collision, 'utf8'), 'keep me'); assert.equal(await readFile(target, 'utf8'), 'keep me'); - assert.deepEqual(await readFile(path), corrupt); + assert.equal(events.length, 1); + assert.notEqual(events[0].backupPath, collision); + assert.deepEqual(await readFile(events[0].backupPath), corrupt); + if (planted === 'symlink') assert.equal((await lstat(collision)).isSymbolicLink(), true); }); } @@ -410,7 +882,7 @@ for (const mutation of ['get', 'update', 'updateIf', 'milestone'] as const) { assert.equal(error.cause.cause, failure); assert.equal(error.settingsPath, path); assert.equal(error.backupPath, events[0]?.backupPath); - assert.equal(error.message.includes('do-not-log'), false); + assert.equal(error.message.includes(secret), false); return true; }); assert.equal(syncs, 1); diff --git a/packages/storage/src/atomic-file-write.ts b/packages/storage/src/atomic-file-write.ts index f6a6320fb8..b59ea0f4f3 100644 --- a/packages/storage/src/atomic-file-write.ts +++ b/packages/storage/src/atomic-file-write.ts @@ -47,6 +47,10 @@ export interface AtomicFileWriteOptions { /** Effective mode to apply to the temporary file before it is synchronized * and published. */ fileMode: number; + /** Validate a caller-owned precondition after preparing the temp file, just + * before publication. This is not an atomic compare-and-swap against other + * processes; a rejection leaves the target untouched and removes the temp. */ + beforePublish?: () => Promise; } /** The fs surface `writeAtomicFile` needs; injectable for fault-injection @@ -107,6 +111,7 @@ export async function writeAtomicFile( await handle.close().catch(() => {}); throw error; } + await options.beforePublish?.(); await rename(tempPath, path); published = true; tempCreated = false; diff --git a/packages/storage/src/settings-store.ts b/packages/storage/src/settings-store.ts index 90f0e4aa29..958f7deb27 100644 --- a/packages/storage/src/settings-store.ts +++ b/packages/storage/src/settings-store.ts @@ -17,8 +17,9 @@ * under the License. */ -import { randomUUID } from 'node:crypto'; -import { mkdir, open, readFile, rm } from 'node:fs/promises'; +import { createHash } from 'node:crypto'; +import { constants, type BigIntStats } from 'node:fs'; +import { lstat, mkdir, open, readFile, rm } from 'node:fs/promises'; import { dirname, join } from 'node:path'; import type { AppSettings, UpdateAppSettingsInput } from '@maka/core/settings'; import type { OnboardingMilestone, OnboardingMilestoneId } from '@maka/core/onboarding'; @@ -46,12 +47,15 @@ export class SettingsRecoveryError extends Error { readonly backupPath?: string; /** A file this attempt created but could not finish or remove. */ readonly incompleteBackupPath?: string; + /** Complete bytes retained after a failed directory durability fence. */ + readonly unsyncedBackupPath?: string; constructor(options: { settingsPath: string; phase: 'backup' | 'reset'; backupPath?: string; incompleteBackupPath?: string; + unsyncedBackupPath?: string; cause: unknown; }) { const detail = options.backupPath @@ -63,6 +67,9 @@ export class SettingsRecoveryError extends Error { (options.incompleteBackupPath ? ` An incomplete backup may remain at ${options.incompleteBackupPath}.` : '') + + (options.unsyncedBackupPath + ? ` Complete backup bytes remain at ${options.unsyncedBackupPath}, but directory synchronization is unconfirmed.` + : '') + ' Close the app and check file access and disk space before retrying.', { cause: options.cause }, ); @@ -71,6 +78,7 @@ export class SettingsRecoveryError extends Error { this.phase = options.phase; this.backupPath = options.backupPath; this.incompleteBackupPath = options.incompleteBackupPath; + this.unsyncedBackupPath = options.unsyncedBackupPath; } } @@ -171,7 +179,7 @@ class FileSettingsStore implements SettingsStore { let persisted: unknown; try { - persisted = JSON.parse(bytes.toString('utf8')); + persisted = parseSettings(bytes, this.settingsPath); } catch (error) { if (!(error instanceof SyntaxError)) throw error; // Never attach the parse error: its message can quote stored secrets. @@ -185,11 +193,19 @@ class FileSettingsStore implements SettingsStore { } private async recoverCorruptSettings(bytes: Buffer): Promise { + const snapshot = await readSettingsSnapshot(this.settingsPath); + if (!snapshot.bytes.equals(bytes)) return this.readRepairedSettings(); const backupPath = await this.backupCorruptSettings(bytes); const settings = createDefaultSettings(); try { - await this.write(settings); + await this.write(settings, async () => { + const current = await readSettingsSnapshot(this.settingsPath); + if (!sameSnapshot(snapshot.stat, current.stat) || !current.bytes.equals(bytes)) { + throw new SettingsChangedError(this.settingsPath); + } + }); } catch (error) { + if (error instanceof SettingsChangedError) return this.readRepairedSettings(); if (error instanceof AtomicFileWriteCommitUnknownError) { this.reportRecovery(backupPath, 'commit-unknown'); throw new SettingsRecoveryCommitUnknownError(this.settingsPath, backupPath, error); @@ -205,37 +221,82 @@ class FileSettingsStore implements SettingsStore { return settings; } + private async readRepairedSettings(): Promise { + // One reread, never a recursive recovery: a removed or still-corrupt file + // must not enter the first-run ENOENT path or trigger repeated backups. + const { bytes } = await readSettingsSnapshot(this.settingsPath); + let persisted: unknown; + try { + persisted = parseSettings(bytes, this.settingsPath); + } catch (error) { + if (!(error instanceof SyntaxError)) throw error; + throw new SettingsChangedError(this.settingsPath); + } + return normalizeSettings(persisted); + } + /** An exclusive byte-for-byte backup, completed before replacing settings. * Keeping the source in place avoids turning a failed recovery into ENOENT. + * Preserve unusable candidates (including interrupted writes) and try the + * next suffix. Repeated resets reuse the first complete, private backup. * This has the same platform fsync limits as the shared atomic writer. */ private async backupCorruptSettings(bytes: Buffer): Promise { - const backupPath = `${this.settingsPath}.corrupt-${Date.now()}-${randomUUID()}`; + const digest = createHash('sha256').update(bytes).digest('hex'); + const backupBase = `${this.settingsPath}.corrupt-${digest}`; + let backupPath = backupBase; let created = false; + let createdStat: BigIntStats | undefined; + let complete = false; try { - const handle = await open(backupPath, 'wx', 0o600); - created = true; - try { - await handle.writeFile(bytes); - if (process.platform !== 'win32') await handle.chmod(0o600); - await handle.sync(); - await handle.close(); - } catch (error) { - await handle.close().catch(() => {}); - throw error; + let handle; + for (let attempt = 0; ; attempt += 1) { + backupPath = attempt === 0 ? backupBase : `${backupBase}-${attempt}`; + try { + handle = await open(backupPath, 'wx', 0o600); + created = true; + break; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; + if (await verifyAndSyncBackup(backupPath, bytes)) break; + } } + if (handle) { + try { + createdStat = await handle.stat({ bigint: true }); + await handle.writeFile(bytes); + if (process.platform !== 'win32') await handle.chmod(0o600); + await handle.sync(); + await handle.close(); + } catch (error) { + await handle.close().catch(() => {}); + throw error; + } + } + complete = true; await syncDirectory(dirname(this.settingsPath)); return backupPath; } catch (error) { let incompleteBackupPath: string | undefined; - if (created) { - await rm(backupPath, { force: true }).catch(() => { - incompleteBackupPath = backupPath; - }); + if (created && !complete) { + try { + const current = await lstat(backupPath, { bigint: true }); + // The content-derived name is predictable. Never remove a different + // entry another process installed after our exclusive creation. + if (createdStat && current.dev === createdStat.dev && current.ino === createdStat.ino) { + await rm(backupPath, { force: true }); + } else { + incompleteBackupPath = backupPath; + } + } catch (cleanupError) { + if ((cleanupError as NodeJS.ErrnoException).code !== 'ENOENT') + incompleteBackupPath = backupPath; + } } throw new SettingsRecoveryError({ settingsPath: this.settingsPath, phase: 'backup', incompleteBackupPath, + unsyncedBackupPath: complete ? backupPath : undefined, cause: error, }); } @@ -338,13 +399,14 @@ class FileSettingsStore implements SettingsStore { return result; } - private async write(settings: AppSettings): Promise { + private async write(settings: AppSettings, beforePublish?: () => Promise): Promise { // SettingsStore does not own the workspace directory's permission policy: // sibling stores such as MCP config may independently harden the same root. // Keep both directory creation and the historical umask-derived file mode. await mkdir(dirname(this.settingsPath), { recursive: true }); await writeAtomicFile(this.settingsPath, JSON.stringify(settings, null, 2) + '\n', { fileMode: 0o666 & ~process.umask(), + beforePublish, }); } @@ -355,6 +417,96 @@ class FileSettingsStore implements SettingsStore { } } +class SettingsChangedError extends Error { + constructor(path: string) { + super( + `Settings at ${path} changed during recovery. The current file was not replaced. Finish editing the file and retry.`, + ); + this.name = 'SettingsChangedError'; + } +} + +function parseSettings(bytes: Buffer, path: string): unknown { + if ((bytes[0] === 0xff && bytes[1] === 0xfe) || (bytes[0] === 0xfe && bytes[1] === 0xff)) { + throw new Error( + `Settings at ${path} use UTF-16 encoding. Save the file as UTF-8 and retry. The file was not replaced.`, + ); + } + return JSON.parse(bytes.toString('utf8').replace(/^\uFEFF/u, '')); +} + +function sameSnapshot(left: BigIntStats, right: BigIntStats): boolean { + return ( + left.isFile() && + right.isFile() && + left.dev === right.dev && + left.ino === right.ino && + left.size === right.size && + left.mtimeNs === right.mtimeNs && + left.ctimeNs === right.ctimeNs + ); +} + +function readFlags(readWrite = false): number { + const access = readWrite ? constants.O_RDWR : constants.O_RDONLY; + return process.platform === 'win32' + ? access + : access | constants.O_NOFOLLOW | constants.O_NONBLOCK; +} + +async function readSettingsSnapshot(path: string): Promise<{ bytes: Buffer; stat: BigIntStats }> { + const initial = await lstat(path, { bigint: true }); + if (!initial.isFile()) { + throw new Error( + `Cannot automatically recover settings at ${path}: the path is not a regular file. Repair the file or its symbolic-link target manually.`, + ); + } + const handle = await open(path, readFlags()); + try { + const opened = await handle.stat({ bigint: true }); + if (!sameSnapshot(initial, opened)) throw new SettingsChangedError(path); + const bytes = await handle.readFile(); + const final = await handle.stat({ bigint: true }); + const current = await lstat(path, { bigint: true }); + if ( + !sameSnapshot(opened, final) || + !sameSnapshot(final, current) || + BigInt(bytes.length) !== final.size + ) { + throw new SettingsChangedError(path); + } + return { bytes, stat: final }; + } finally { + await handle.close(); + } +} + +/** An unusable candidate is left intact. Actual I/O failures still abort recovery. */ +async function verifyAndSyncBackup(path: string, bytes: Buffer): Promise { + const initial = await lstat(path, { bigint: true }); + if ( + !initial.isFile() || + initial.size !== BigInt(bytes.length) || + initial.nlink !== 1n || + (process.platform !== 'win32' && + ((initial.mode & 0o777n) !== 0o600n || initial.uid !== BigInt(process.geteuid!()))) + ) { + return false; + } + const handle = await open(path, readFlags(true)); + try { + const opened = await handle.stat({ bigint: true }); + if (!sameSnapshot(initial, opened) || !(await handle.readFile()).equals(bytes)) return false; + const final = await handle.stat({ bigint: true }); + const current = await lstat(path, { bigint: true }); + if (!sameSnapshot(opened, final) || !sameSnapshot(final, current)) return false; + await handle.sync(); + return true; + } finally { + await handle.close(); + } +} + function hasLegacyProxyCredentialFields(value: unknown): boolean { if (!isRecord(value) || !isRecord(value.network) || !isRecord(value.network.proxy)) { return false; From 34f0cb31a80b93fa6a9715f536bb51e48bc8c75d Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Wed, 23 Sep 2026 14:40:04 +0800 Subject: [PATCH 4/8] fix(mcp): preserve repair errors across IPC and TUI actions Carry safe typed MCP file errors through a shared main/preload/renderer result envelope instead of matching Electron-wrapped English messages. Keep the original error cause available to native consumers. Block management actions after TUI initialization fails while keeping repair details scrollable, including the empty server list. Clarify localized repair guidance and cover all MCP IPC operations and parser-secret redaction. Refs #4285 Generated-by: OpenAI Codex --- .../__tests__/mcp-ipc-commit-unknown.test.ts | 53 +++++++++----- .../src/main/__tests__/mcp-page-model.test.ts | 25 +++++++ .../main/__tests__/mcp-preload-scope.test.ts | 66 +++++++++++++++++ apps/desktop/src/main/mcp-ipc-main.ts | 37 +++++++--- apps/desktop/src/preload/bridge-contract.d.ts | 23 +++--- apps/desktop/src/preload/preload.ts | 23 +++--- .../controller/use-mcp-controller.ts | 14 ++-- .../module-hub/model/mcp-page-model.ts | 28 ++++++-- .../src/renderer/features/module-hub/ports.ts | 24 ++++--- .../renderer/features/module-hub/testing.ts | 1 + apps/desktop/src/shared/mcp-ipc.ts | 35 ++++++++++ .../src/__tests__/pi-tui-mcp-status.test.ts | 70 +++++++++++++++++++ .../cli/src/__tests__/tui-mcp-control.test.ts | 22 ++++-- packages/cli/src/pi-tui-mcp-status.ts | 9 +-- packages/cli/src/tui-copy-catalog.ts | 6 +- .../src/__tests__/mcp-config-store.test.ts | 12 +++- 16 files changed, 362 insertions(+), 86 deletions(-) create mode 100644 apps/desktop/src/shared/mcp-ipc.ts diff --git a/apps/desktop/src/main/__tests__/mcp-ipc-commit-unknown.test.ts b/apps/desktop/src/main/__tests__/mcp-ipc-commit-unknown.test.ts index 102171a1cd..587e23164e 100644 --- a/apps/desktop/src/main/__tests__/mcp-ipc-commit-unknown.test.ts +++ b/apps/desktop/src/main/__tests__/mcp-ipc-commit-unknown.test.ts @@ -29,12 +29,11 @@ import { McpClientManager } from '@maka/mcp'; import { AtomicFileWriteCommitUnknownError, createMcpConfigStore, - McpConfigSourceError, type McpConfigStore, } from '@maka/storage/mcp-config-store'; import { registerMcpIpcMain, type McpIpcMainDeps } from '../mcp-ipc-main.js'; import { getMcpCopy } from '../../renderer/locales/mcp-copy.js'; -import { mcpConfigFailureMessage } from '../../renderer/features/module-hub/testing.js'; +import { mcpConfigFailureMessage, unwrapMcpIpcResult } from '../../renderer/features/module-hub/testing.js'; test('MCP remove reconciles a live manager after the real store publishes then fails directory sync', { skip: process.platform === 'win32', @@ -245,27 +244,43 @@ function mutationHarness(t: TestContext, store: McpConfigStore, overrides: Parti }; } -test('MCP IPC preserves repair guidance for a corrupt persisted file and does not import over it', async (t) => { +test('MCP IPC transports corrupt-file details for reads and every mutation without exposing parser secrets', async (t) => { const { root, store } = await fixtureStore(t); const path = join(root, 'mcp.json'); - const source = '{"secret":"must-not-appear"'; + const source = 'sk-live-SECRET'; + assert.throws(() => JSON.parse(source), (error) => { + assert.ok(error instanceof SyntaxError && error.message.includes(source)); + return true; + }); await writeFile(path, source); const ipc = mutationHarness(store); - for (const call of [() => ipc.invoke('mcp:getConfig'), () => ipc.invoke('mcp:importConfig', '{"new":{"command":"unused"}}')]) { - await assert.rejects(call(), (error) => { - assert.ok(error instanceof McpConfigSourceError); - assert.equal(error.path, path); - assert.ok(error.message.includes(path)); - assert.match(error.message, /back up and repair/u); - for (const locale of ['en', 'zh-CN', 'zh-TW'] as const) { - const copy = getMcpCopy(locale); - const ipcError: Error = new Error(`Error invoking remote method 'mcp:getConfig': ${error.name}: ${error.message}`); - assert.equal(mcpConfigFailureMessage(ipcError, copy), copy.errors.invalidConfigFile(path)); - } - assert.equal(error.message.includes('must-not-appear'), false); - return true; - }); + const calls: [string, ...unknown[]][] = [ + ['mcp:getConfig'], + ['mcp:importConfig', '{"new":{"command":"unused"}}'], + ['mcp:add', 'new', { command: 'unused' }], + ['mcp:update', 'old', { command: 'unused' }, { command: 'old' }], + ['mcp:setEnabled', 'old', true], + ['mcp:remove', 'old'], + ]; + for (const [channel, ...args] of calls) { + // Electron serializes the fulfilled value, not custom Error fields. + const result: unknown = structuredClone(await ipc.invoke(channel, ...args)); + assert.deepEqual(result, { kind: 'invalid-mcp-config-file', path }); + assert.equal(JSON.stringify(result).includes(source), false); + for (const locale of ['en', 'zh-CN', 'zh-TW'] as const) { + const copy = getMcpCopy(locale); + assert.throws(() => unwrapMcpIpcResult(result), (error) => { + const wrapped = new Error('Runtime Host action failed', { cause: error }); + assert.equal(mcpConfigFailureMessage(wrapped, copy), copy.errors.invalidConfigFile(path)); + return true; + }); + } + assert.equal(await readFile(path, 'utf8'), source); } + assert.deepEqual(await ipc.invoke('mcp:importConfig', source), { + status: 'invalid', reason: 'invalid-json', + }, 'pasted invalid JSON remains an import validation result'); assert.deepEqual(ipc.synced, []); - assert.equal(await readFile(path, 'utf8'), source); + assert.deepEqual(ipc.retired, []); + assert.deepEqual(ipc.emitted, []); }); diff --git a/apps/desktop/src/main/__tests__/mcp-page-model.test.ts b/apps/desktop/src/main/__tests__/mcp-page-model.test.ts index c225165df1..f85957743e 100644 --- a/apps/desktop/src/main/__tests__/mcp-page-model.test.ts +++ b/apps/desktop/src/main/__tests__/mcp-page-model.test.ts @@ -28,6 +28,7 @@ import { mcpDraftProtocolPreference, mcpDraftFromConfig, mcpConfigFailureMessage, + unwrapMcpIpcResult, } from '../../renderer/features/module-hub/testing.js'; const copy = getMcpCopy('en'); @@ -212,3 +213,27 @@ test('an untouched environment reads back unchanged, whatever its values hold', assert.ok(isMcpStdioConfig(saved)); assert.deepEqual(saved.env, env); }); + + +test('corrupt MCP localization uses typed data, not English error text or custom IPC Error fields', () => { + const path = '/profile/mcp.json'; + const result = structuredClone({ kind: 'invalid-mcp-config-file', path: '/profile/\u0001mcp.json' }); + for (const locale of ['en', 'zh-CN', 'zh-TW'] as const) { + const localized = getMcpCopy(locale); + assert.throws(() => unwrapMcpIpcResult(result), (error) => { + assert.equal(mcpConfigFailureMessage(error, localized), localized.errors.invalidConfigFile(path)); + assert.equal( + mcpConfigFailureMessage(new Error('unrelated wrapper text', { cause: error }), localized), + localized.errors.invalidConfigFile(path), + ); + return true; + }); + const oldMessage = `MCP config at ${path} contains invalid JSON. The file was not modified. Close the app, back up and repair this file before retrying.`; + assert.equal(mcpConfigFailureMessage(new Error(oldMessage), localized), undefined); + } + const invalidImport = { status: 'invalid', reason: 'invalid-json' } as const; + assert.equal(unwrapMcpIpcResult(invalidImport), invalidImport); + const cyclic = new Error('cyclic cause'); + cyclic.cause = cyclic; + assert.equal(mcpConfigFailureMessage(cyclic, copy), undefined); +}); diff --git a/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts b/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts index 3488236ab1..45d28d1a68 100644 --- a/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts +++ b/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts @@ -19,6 +19,13 @@ import assert from 'node:assert/strict'; import { readFileSync } from 'node:fs'; +import { EventEmitter } from 'node:events'; +import { createRequire } from 'node:module'; +import { runInNewContext } from 'node:vm'; +import { build } from 'esbuild'; +import type { MakaBridge } from '../../preload/bridge-contract.js'; +import { unwrapMcpIpcResult, mcpConfigFailureMessage } from '../../renderer/features/module-hub/testing.js'; +import { getMcpCopy } from '../../renderer/locales/mcp-copy.js'; import { fileURLToPath } from 'node:url'; import { test } from 'node:test'; @@ -50,3 +57,62 @@ test('every MCP bridge method rides the scoped Runtime Host seam', () => { assert.match(preloadSource, new RegExp(`invokeSelectedRuntimeHost\\(host, '${channel}'`, 'u')); } }); + + +test('every MCP bridge method carries typed config failures intact through the bundled preload', async () => { + const failure = { kind: 'invalid-mcp-config-file', path: '/profile/mcp.json' } as const; + const events = new EventEmitter(); + const channels: string[] = []; + const owner = { hostId: 'owner', targetEpoch: 'epoch', profileId: 'local', + profileName: 'Local', profileKind: 'local', profileAccess: 'owner', readiness: 'ready' }; + const ipcRenderer = { + on: events.on.bind(events), off: events.off.bind(events), send() {}, + async invoke(channel: string, ...args: unknown[]) { + if (channel === 'app:bootstrapReady') return undefined; + if (channel === 'runtime-host:identities') return structuredClone([owner]); + assert.ok(channel.startsWith('mcp:'), channel); + assert.deepEqual(JSON.parse(JSON.stringify(args[0])), owner); + channels.push(channel); + return structuredClone(failure); + }, + }; + let bridge: MakaBridge | undefined; + const bundle = await build({ + entryPoints: [fileURLToPath(new URL('../../../src/preload/preload.ts', import.meta.url))], + bundle: true, write: false, platform: 'node', format: 'cjs', external: ['electron'], + }); + const require = createRequire(import.meta.url); + runInNewContext(bundle.outputFiles[0]!.text, { + require: (id: string) => id === 'electron' ? { + ipcRenderer, + contextBridge: { exposeInMainWorld(name: string, value: MakaBridge) { + if (name === 'maka') bridge = value; + } }, + } : require(id), + process: { env: {} }, Buffer, console, setTimeout, clearTimeout, TextEncoder, TextDecoder, + crypto: globalThis.crypto, + }); + assert.ok(bridge); + const mcp = bridge.mcp; + const host = { hostId: 'owner', profileId: 'local' }; + const server = { command: 'unused' }; + const calls = [ + () => mcp.getConfig(host), () => mcp.listStatuses(host), + () => mcp.importConfig('{}', host), () => mcp.add('id', server, host), + () => mcp.update('id', server, server, host), () => mcp.setEnabled('id', true, host), + () => mcp.remove('id', host), + () => mcp.test('id', host), () => mcp.login('id', host), + () => mcp.cancelLogin('id', host), () => mcp.logout('id', host), + ]; + for (const call of calls) { + // Plain fulfilled data survives the context bridge; rebuilding an Error + // in preload would introduce a second lossy error-serialization boundary. + const result: unknown = structuredClone(await call()); + assert.deepEqual(result, failure); + assert.throws(() => unwrapMcpIpcResult(result), (error) => { + assert.equal(mcpConfigFailureMessage(error, getMcpCopy('en')), getMcpCopy('en').errors.invalidConfigFile(failure.path)); + return true; + }); + } + assert.equal(new Set(channels).size, calls.length); +}); diff --git a/apps/desktop/src/main/mcp-ipc-main.ts b/apps/desktop/src/main/mcp-ipc-main.ts index 65a32b9963..9a1518ac76 100644 --- a/apps/desktop/src/main/mcp-ipc-main.ts +++ b/apps/desktop/src/main/mcp-ipc-main.ts @@ -36,6 +36,7 @@ import { normalizeMcpImport, type McpConfigStore, } from '@maka/storage/mcp-config-store'; +import type { McpConfigFileFailure } from '../shared/mcp-ipc.js'; import type { McpOAuthController } from './mcp-oauth-controller.js'; import { redactMcpConfigSecrets, @@ -80,6 +81,22 @@ export function createMcpExclusiveLane(): McpExclusiveLane { } export function registerMcpIpcMain(deps: McpIpcMainDeps): () => void { + const handle = (channel: string, listener: Parameters[1]): void => { + deps.ipcMain.handle(channel, async (event, ...args) => { + try { + return await listener(event, ...args); + } catch (error) { + if (error instanceof McpConfigSourceError && error.reason === 'invalid-json' && error.path !== undefined) { + const failure: McpConfigFileFailure = { + kind: 'invalid-mcp-config-file', + path: error.path.replace(/[\u0000-\u001f\u007f-\u009f]/gu, ''), + }; + return failure; + } + throw error; + } + }); + }; // Main is the authority on operation exclusivity, not the renderer's // advisory locks: while a login round owns a server, a config mutation // would race the browser callback against a changed or absent server. @@ -146,18 +163,18 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): () => void { // toward it leaves with clientSecret replaced by the sentinel, and every // config it sends back has sentinels restored from disk before the store // and the manager (which needs the real secret) see it. - deps.ipcMain.handle('mcp:getConfig', async () => { + handle('mcp:getConfig', async () => { await deps.ensureReady(); return redactMcpConfigSecrets(await deps.store.get()); }); - deps.ipcMain.handle('mcp:listStatuses', async () => { + handle('mcp:listStatuses', async () => { await deps.ensureReady(); return deps.manager.statuses().map((status) => ({ ...status, ...(deps.oauth.isActive(status.serverId) ? { authorizationPending: true } : {}), })); }); - deps.ipcMain.handle( + handle( 'mcp:importConfig', async (_event, source: string): Promise => { let imported: McpConfigFile; @@ -190,7 +207,7 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): () => void { }); }, ); - deps.ipcMain.handle( + handle( 'mcp:add', async (_event, serverId: string, config: McpServerConfig): Promise => { assertNoActiveLogin(serverId); @@ -262,7 +279,7 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): () => void { ); // Flips the switch on what is on disk, so a toggle never writes back the // rest of an older copy. - deps.ipcMain.handle('mcp:setEnabled', (_event, serverId: string, enabled: boolean) => + handle('mcp:setEnabled', (_event, serverId: string, enabled: boolean) => updateServer(serverId, (_current, previous) => ({ ...previous, enabled })), ); const removeServer = async (serverId: string): Promise => @@ -272,7 +289,7 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): () => void { return { ...current, mcpServers }; }), ); - deps.ipcMain.handle('mcp:remove', async (_event, serverId: string) => { + handle('mcp:remove', async (_event, serverId: string) => { assertNoActiveLogin(serverId); const next = await removeServer(serverId); await deps.manager.sync(next); @@ -281,13 +298,13 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): () => void { // servers' secrets must leave as sentinels here too. return redactMcpConfigSecrets(next); }); - deps.ipcMain.handle('mcp:test', async (_event, serverId: string) => { + handle('mcp:test', async (_event, serverId: string) => { await deps.ensureReady(); const result = await deps.manager.test(serverId); deps.emitChanged(deps.manager.statuses()); return result; }); - deps.ipcMain.handle('mcp:login', async (_event, serverId: string) => { + handle('mcp:login', async (_event, serverId: string) => { // No preflight here: readiness and the callback-port lookup run INSIDE // the controller under its round deadline, so a stalled store cannot // park this promise (and the renderer's login lock) forever. @@ -300,14 +317,14 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): () => void { changed(deps); } }); - deps.ipcMain.handle('mcp:cancelLogin', async (_event, serverId: string) => { + handle('mcp:cancelLogin', async (_event, serverId: string) => { const cancelled = deps.oauth.cancelLogin(serverId); // The round's own rejection path abandons the persisted pending state; // the renderer just needs the resulting statuses. if (cancelled) changed(deps); return cancelled; }); - deps.ipcMain.handle('mcp:logout', async (_event, serverId: string) => { + handle('mcp:logout', async (_event, serverId: string) => { // Like mcp:login, no preflight here: readiness runs INSIDE the // controller under its round deadline, so a stalled store cannot park // the renderer's logout lock forever. diff --git a/apps/desktop/src/preload/bridge-contract.d.ts b/apps/desktop/src/preload/bridge-contract.d.ts index 5aed37c36a..22a69427f8 100644 --- a/apps/desktop/src/preload/bridge-contract.d.ts +++ b/apps/desktop/src/preload/bridge-contract.d.ts @@ -17,6 +17,7 @@ * under the License. */ +import type { McpIpcResult } from '../shared/mcp-ipc.js'; import type { WorkHubAnswerInput, WorkHubAnswerResult, @@ -1586,22 +1587,22 @@ export interface MakaBridge { subscribeEvents(handler: (event: ConnectionEvent) => void, host?: DesktopRuntimeHostRef): () => void; }; mcp: { - getConfig(host?: DesktopRuntimeHostRef): Promise; - listStatuses(host?: DesktopRuntimeHostRef): Promise; - importConfig(source: string, host?: DesktopRuntimeHostRef): Promise; + getConfig(host?: DesktopRuntimeHostRef): Promise>; + listStatuses(host?: DesktopRuntimeHostRef): Promise>; + importConfig(source: string, host?: DesktopRuntimeHostRef): Promise>; /** Adds a new server; a taken id comes back as `{ status: 'exists' }` * instead of an error, so the dialog can put it on the id field. */ - add(serverId: string, config: McpServerConfig, host?: DesktopRuntimeHostRef): Promise; + add(serverId: string, config: McpServerConfig, host?: DesktopRuntimeHostRef): Promise>; /** Saves an edit made against `basis`, the server as last shown; one * changed or removed elsewhere since comes back `stale`. */ - update(serverId: string, config: McpServerConfig, basis: McpServerConfig, host?: DesktopRuntimeHostRef): Promise; - setEnabled(serverId: string, enabled: boolean, host?: DesktopRuntimeHostRef): Promise; - remove(serverId: string, host?: DesktopRuntimeHostRef): Promise; - test(serverId: string, host?: DesktopRuntimeHostRef): Promise; - login(serverId: string, host?: DesktopRuntimeHostRef): Promise; + update(serverId: string, config: McpServerConfig, basis: McpServerConfig, host?: DesktopRuntimeHostRef): Promise>; + setEnabled(serverId: string, enabled: boolean, host?: DesktopRuntimeHostRef): Promise>; + remove(serverId: string, host?: DesktopRuntimeHostRef): Promise>; + test(serverId: string, host?: DesktopRuntimeHostRef): Promise>; + login(serverId: string, host?: DesktopRuntimeHostRef): Promise>; /** Ends an in-flight login round; resolves false when none is active. */ - cancelLogin(serverId: string, host?: DesktopRuntimeHostRef): Promise; - logout(serverId: string, host?: DesktopRuntimeHostRef): Promise; + cancelLogin(serverId: string, host?: DesktopRuntimeHostRef): Promise>; + logout(serverId: string, host?: DesktopRuntimeHostRef): Promise>; chromeStatus(host?: DesktopRuntimeHostRef): Promise; connectChrome(host?: DesktopRuntimeHostRef): Promise; subscribeChanges(handler: (statuses: McpServerStatus[]) => void): () => void; diff --git a/apps/desktop/src/preload/preload.ts b/apps/desktop/src/preload/preload.ts index 6476d066c4..4da390cc57 100644 --- a/apps/desktop/src/preload/preload.ts +++ b/apps/desktop/src/preload/preload.ts @@ -17,6 +17,7 @@ * under the License. */ +import type { McpIpcResult } from '../shared/mcp-ipc.js'; import { invokeWhenReady, sendWhenReady } from './bootstrap-invoke.js'; import { createClientPluginRouting } from './client-plugin-routing.js'; import type { @@ -3184,41 +3185,41 @@ const makaBridge = { }, }, mcp: { - getConfig(host?: DesktopRuntimeHostRef): Promise { + getConfig(host?: DesktopRuntimeHostRef): Promise> { return invokeSelectedRuntimeHost(host, 'mcp:getConfig'); }, - listStatuses(host?: DesktopRuntimeHostRef): Promise { + listStatuses(host?: DesktopRuntimeHostRef): Promise> { return invokeSelectedRuntimeHost(host, 'mcp:listStatuses'); }, - importConfig(source: string, host?: DesktopRuntimeHostRef): Promise { + importConfig(source: string, host?: DesktopRuntimeHostRef): Promise> { return invokeSelectedRuntimeHost(host, 'mcp:importConfig', source); }, - add(serverId: string, config: McpServerConfig, host?: DesktopRuntimeHostRef): Promise { + add(serverId: string, config: McpServerConfig, host?: DesktopRuntimeHostRef): Promise> { return invokeSelectedRuntimeHost(host, 'mcp:add', serverId, config); }, - update(serverId: string, config: McpServerConfig, basis: McpServerConfig, host?: DesktopRuntimeHostRef): Promise { + update(serverId: string, config: McpServerConfig, basis: McpServerConfig, host?: DesktopRuntimeHostRef): Promise> { return invokeSelectedRuntimeHost(host, 'mcp:update', serverId, config, basis); }, - setEnabled(serverId: string, enabled: boolean, host?: DesktopRuntimeHostRef): Promise { + setEnabled(serverId: string, enabled: boolean, host?: DesktopRuntimeHostRef): Promise> { return invokeSelectedRuntimeHost(host, 'mcp:setEnabled', serverId, enabled); }, - remove(serverId: string, host?: DesktopRuntimeHostRef): Promise { + remove(serverId: string, host?: DesktopRuntimeHostRef): Promise> { return invokeSelectedRuntimeHost(host, 'mcp:remove', serverId); }, - test(serverId: string, host?: DesktopRuntimeHostRef): Promise { + test(serverId: string, host?: DesktopRuntimeHostRef): Promise> { return invokeSelectedRuntimeHost(host, 'mcp:test', serverId); }, // Same scoped seam as every other MCP method: the handlers live on the // Runtime Host's ScopedIpcMain, whose first argument is the host ref — // a raw invoke would put serverId in that slot and fail the scope check // before the handler ever ran. - login(serverId: string, host?: DesktopRuntimeHostRef): Promise { + login(serverId: string, host?: DesktopRuntimeHostRef): Promise> { return invokeSelectedRuntimeHost(host, 'mcp:login', serverId); }, - cancelLogin(serverId: string, host?: DesktopRuntimeHostRef): Promise { + cancelLogin(serverId: string, host?: DesktopRuntimeHostRef): Promise> { return invokeSelectedRuntimeHost(host, 'mcp:cancelLogin', serverId); }, - logout(serverId: string, host?: DesktopRuntimeHostRef): Promise { + logout(serverId: string, host?: DesktopRuntimeHostRef): Promise> { return invokeSelectedRuntimeHost(host, 'mcp:logout', serverId); }, chromeStatus(host?: DesktopRuntimeHostRef): Promise { diff --git a/apps/desktop/src/renderer/features/module-hub/controller/use-mcp-controller.ts b/apps/desktop/src/renderer/features/module-hub/controller/use-mcp-controller.ts index 89fe80d5a3..676ba6896e 100644 --- a/apps/desktop/src/renderer/features/module-hub/controller/use-mcp-controller.ts +++ b/apps/desktop/src/renderer/features/module-hub/controller/use-mcp-controller.ts @@ -28,6 +28,8 @@ import { type OpencliChromeStatus, } from '@maka/core/mcp'; import { useMountedRef } from '@maka/ui'; +import type { McpIpcResult } from '../../../../shared/mcp-ipc.js'; +import { unwrapMcpIpcResult } from '../model/mcp-page-model.js'; import { useModuleHubServices } from '../services-context.js'; import type { ModuleHubRuntimeHostRef } from '../ports.js'; import { isDefaultRuntimeHostCurrent, runOnDefaultRuntimeHost } from './default-runtime-host.js'; @@ -59,8 +61,10 @@ export function useMcpController() { !await isDefaultRuntimeHostCurrent(runtimeHosts, result.host) || !mounted.current || request !== revision.current ) return; - setConfig(result.value[0]); - setStatuses(result.value[1]); + const nextConfig = unwrapMcpIpcResult(result.value[0]); + const nextStatuses = unwrapMcpIpcResult(result.value[1]); + setConfig(nextConfig); + setStatuses(nextStatuses); setChrome(result.value[2]); } catch (failure) { if (mounted.current && request === revision.current) setError(failure); @@ -99,7 +103,7 @@ export function useMcpController() { return () => clearInterval(timer); }, [awaitingChrome, mcp, runtimeHosts, mounted]); - async function run(key: string, action: (host: ModuleHubRuntimeHostRef) => Promise): Promise { + async function run(key: string, action: (host: ModuleHubRuntimeHostRef) => Promise>): Promise { if (operation.current) return undefined; const current: { key: string; host?: ModuleHubRuntimeHostRef; cancelled?: boolean } = { key }; operation.current = current; @@ -110,7 +114,7 @@ export function useMcpController() { current.host = host; return action(host); }); - if (mounted.current && await isDefaultRuntimeHostCurrent(runtimeHosts, result.host)) return result.value; + if (mounted.current && await isDefaultRuntimeHostCurrent(runtimeHosts, result.host)) return unwrapMcpIpcResult(result.value); } catch (failure) { if (mounted.current && !current.cancelled) setError(failure); } finally { @@ -150,7 +154,7 @@ export function useMcpController() { if (current.key !== `login:${id}` || !current.host) return; current.cancelled = true; try { - await mcp.cancelLogin(id, current.host); + unwrapMcpIpcResult(await mcp.cancelLogin(id, current.host)); } catch (failure) { current.cancelled = false; if (mounted.current) setError(failure); diff --git a/apps/desktop/src/renderer/features/module-hub/model/mcp-page-model.ts b/apps/desktop/src/renderer/features/module-hub/model/mcp-page-model.ts index faffec5b5f..26395ff393 100644 --- a/apps/desktop/src/renderer/features/module-hub/model/mcp-page-model.ts +++ b/apps/desktop/src/renderer/features/module-hub/model/mcp-page-model.ts @@ -24,16 +24,32 @@ import type { } from '@maka/core/mcp'; import { isMcpStdioConfig, resolveMcpProtocolPreference } from '@maka/core/mcp'; import type { McpCopy } from '../../../locales/mcp-copy.js'; +import { isMcpConfigFileFailure, type McpIpcResult } from '../../../../shared/mcp-ipc.js'; import { formatCommandLine, parseCommandLine } from './mcp-command-line.js'; -/** Electron preserves error messages, but not custom error fields. Map only - * the fixed config-error messages to safe, localized presentation. */ +class McpConfigFileError extends Error { + constructor(readonly path: string) { + super('Invalid persisted MCP configuration'); + } +} + +/** Unwrap in the renderer, after both IPC and contextBridge serialization. */ +export function unwrapMcpIpcResult(result: McpIpcResult): T { + if (isMcpConfigFileFailure(result)) { + throw new McpConfigFileError(result.path.replace(/[\u0000-\u001f\u007f-\u009f]/gu, '')); + } + return result; +} + export function mcpConfigFailureMessage(error: unknown, copy: McpCopy): string | undefined { - const message = error instanceof Error ? error.message : typeof error === 'string' ? error : ''; - const invalidFile = /MCP config at ([^\r\n]+) contains invalid JSON\. The file was not modified\. Close the app, back up and repair this file before retrying\.$/u.exec(message); - if (invalidFile) { - return copy.errors.invalidConfigFile(invalidFile[1].replace(/[\u0000-\u001f\u007f-\u009f]/gu, '')); + // Runtime Host actions add a diagnostic-target wrapper inside the renderer. + // Follow its local cause without interpreting display text as an IPC code. + const seen = new Set(); + for (let cause = error; cause instanceof Error && !seen.has(cause); cause = cause.cause) { + if (cause instanceof McpConfigFileError) return copy.errors.invalidConfigFile(cause.path); + seen.add(cause); } + const message = error instanceof Error ? error.message : typeof error === 'string' ? error : ''; if (message.includes('MCP write durability is uncertain and runtime state is out of sync')) { return copy.errors.writeOutOfSync; } diff --git a/apps/desktop/src/renderer/features/module-hub/ports.ts b/apps/desktop/src/renderer/features/module-hub/ports.ts index e92c886dbc..e5c65e70a9 100644 --- a/apps/desktop/src/renderer/features/module-hub/ports.ts +++ b/apps/desktop/src/renderer/features/module-hub/ports.ts @@ -43,6 +43,8 @@ import type { SkillLocationsSnapshot, } from '../../../shared/skill-locations.js'; +import type { McpIpcResult } from '../../../shared/mcp-ipc.js'; + export type ModuleHubUnsubscribe = () => void; export interface ModuleHubRuntimeHostRef { @@ -250,17 +252,17 @@ export interface ModuleHubClipboardService { /** Environment capabilities owned by the Module Hub feature slice. */ export interface ModuleHubMcpService { - getConfig(host: ModuleHubRuntimeHostRef): Promise; - listStatuses(host: ModuleHubRuntimeHostRef): Promise; - add(id: string, config: McpServerConfig, host: ModuleHubRuntimeHostRef): Promise; - update(id: string, config: McpServerConfig, basis: McpServerConfig, host: ModuleHubRuntimeHostRef): Promise; - setEnabled(id: string, enabled: boolean, host: ModuleHubRuntimeHostRef): Promise; - importConfig(source: string, host: ModuleHubRuntimeHostRef): Promise; - remove(id: string, host: ModuleHubRuntimeHostRef): Promise; - test(id: string, host: ModuleHubRuntimeHostRef): Promise; - login(id: string, host: ModuleHubRuntimeHostRef): Promise; - cancelLogin(id: string, host: ModuleHubRuntimeHostRef): Promise; - logout(id: string, host: ModuleHubRuntimeHostRef): Promise; + getConfig(host: ModuleHubRuntimeHostRef): Promise>; + listStatuses(host: ModuleHubRuntimeHostRef): Promise>; + add(id: string, config: McpServerConfig, host: ModuleHubRuntimeHostRef): Promise>; + update(id: string, config: McpServerConfig, basis: McpServerConfig, host: ModuleHubRuntimeHostRef): Promise>; + setEnabled(id: string, enabled: boolean, host: ModuleHubRuntimeHostRef): Promise>; + importConfig(source: string, host: ModuleHubRuntimeHostRef): Promise>; + remove(id: string, host: ModuleHubRuntimeHostRef): Promise>; + test(id: string, host: ModuleHubRuntimeHostRef): Promise>; + login(id: string, host: ModuleHubRuntimeHostRef): Promise>; + cancelLogin(id: string, host: ModuleHubRuntimeHostRef): Promise>; + logout(id: string, host: ModuleHubRuntimeHostRef): Promise>; chromeStatus(host: ModuleHubRuntimeHostRef): Promise; connectChrome(host: ModuleHubRuntimeHostRef): Promise; subscribeChanges(handler: () => void): ModuleHubUnsubscribe; diff --git a/apps/desktop/src/renderer/features/module-hub/testing.ts b/apps/desktop/src/renderer/features/module-hub/testing.ts index 9ebd412e15..6f416e96e4 100644 --- a/apps/desktop/src/renderer/features/module-hub/testing.ts +++ b/apps/desktop/src/renderer/features/module-hub/testing.ts @@ -40,6 +40,7 @@ export { mcpDraftProtocolPreference, mcpDraftFromConfig, mcpConfigFailureMessage, + unwrapMcpIpcResult, } from "./model/mcp-page-model.js"; export { useModuleHubController } from "./controller/use-module-hub-controller.js"; export { diff --git a/apps/desktop/src/shared/mcp-ipc.ts b/apps/desktop/src/shared/mcp-ipc.ts new file mode 100644 index 0000000000..2c32938461 --- /dev/null +++ b/apps/desktop/src/shared/mcp-ipc.ts @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** MCP config failures travel as data across both Electron IPC and the + * context bridge, which do not preserve custom Error properties. Successful + * values keep their existing shape; pasted JSON validation is a separate + * McpConfigImportResult, not a persisted-file failure. */ +export interface McpConfigFileFailure { + readonly kind: 'invalid-mcp-config-file'; + readonly path: string; +} + +export type McpIpcResult = T | McpConfigFileFailure; + +export function isMcpConfigFileFailure(value: unknown): value is McpConfigFileFailure { + return typeof value === 'object' && value !== null && + 'kind' in value && value.kind === 'invalid-mcp-config-file' && + 'path' in value && typeof value.path === 'string'; +} diff --git a/packages/cli/src/__tests__/pi-tui-mcp-status.test.ts b/packages/cli/src/__tests__/pi-tui-mcp-status.test.ts index 54a78d70b0..8c2c5e77b7 100644 --- a/packages/cli/src/__tests__/pi-tui-mcp-status.test.ts +++ b/packages/cli/src/__tests__/pi-tui-mcp-status.test.ts @@ -573,6 +573,7 @@ test('invalid persisted MCP JSON renders its location and repair guidance in eve const text = overlay.render(160).map(stripAnsi).join('\n'); assert.match(text, /\/profile\/mcp\.json/u); assert.match(text, /back up and repair|备份并修复|備份並修復/u); + assert.match(text, /Quit maka|退出 maka/u); assert.match(text, /unchanged|未被修改/u); } }); @@ -603,6 +604,7 @@ for (const locale of ['en', 'zh-CN', 'zh-TW'] as const) { const text = render(); assert.ok(text.includes('/profile/mcp.json')); assert.match(text, /back up and repair|备份并修复|備份並修復/u); + assert.match(text, /Quit maka|退出 maka/u); assert.ok(text.includes(TUI_COPY_RESOURCES['mcp-status'][locale].footer.diagnostic)); assert.doesNotMatch(text, /\u0000/u); assert.equal(mcp.snapshot().initialization, 'ready'); @@ -673,3 +675,71 @@ for (const phase of ['initialization', 'mutation'] as const) { assert.ok(expanded.includes('retrying.')); }); } + +test('an initialization error permits scrolling and closing but rejects all management keys', async () => { + for (const exitKey of ['q', '\u001b']) { + const snapshot = { + ...listSnapshot(), + initialization: 'error' as const, + invalidConfigPath: '/profile/mcp.json', + canManagePublicationCredential: true, + }; + const mcp = surface(snapshot); + const actions: TuiMcpAction[] = []; + let edits = 0; + mcp.execute = async (action) => { + actions.push(action); + return { status: 'applied', effect: 'published' }; + }; + mcp.configForEdit = () => { + edits += 1; + return undefined; + }; + let closed = 0; + const overlay = new McpManagementOverlay({ + locale: 'en', + tui: fakeTui(), + surface: mcp, + viewportRows: () => 8, + onClose: () => { + closed += 1; + }, + onChange: () => {}, + }); + const render = () => overlay.render(100).map(stripAnsi).join('\n'); + const before = render(); + for (const key of ['a', 'p', 'x', '\r', ' ', 't', 'r', 'd']) { + overlay.handleInput(key); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal( + render(), + before, + `management key ${JSON.stringify(key)} must leave the error view intact`, + ); + } + assert.deepEqual(actions, []); + assert.equal(edits, 0); + assert.equal(closed, 0); + overlay.handleInput(exitKey); + assert.equal(closed, 1); + } +}); + +test('a ready empty list scrolls to its add guidance in a short terminal and remains manageable', () => { + const mcp = surface({ ...listSnapshot(), publication: 'not_published', servers: [] }); + const overlay = new McpManagementOverlay({ + locale: 'en', + surface: mcp, + viewportRows: () => 3, + onClose: () => {}, + onChange: () => {}, + }); + const render = () => overlay.render(120).map(stripAnsi).join('\n'); + assert.equal(render().includes('No MCP servers are configured.'), false); + overlay.handleInput('\u001b[F'); + assert.match(render(), /No MCP servers are configured\. Press a to add one\./u); + overlay.handleInput('\u001b[H'); + assert.match(render(), /not published/u); + overlay.handleInput('a'); + assert.doesNotMatch(render(), /not published/u); +}); diff --git a/packages/cli/src/__tests__/tui-mcp-control.test.ts b/packages/cli/src/__tests__/tui-mcp-control.test.ts index d7c1e6fcf4..84222a64ab 100644 --- a/packages/cli/src/__tests__/tui-mcp-control.test.ts +++ b/packages/cli/src/__tests__/tui-mcp-control.test.ts @@ -1421,7 +1421,14 @@ test('TUI MCP retains only the invalid persisted config path for repair guidance const manager = managerHarness(0, []); const connection = connectionHarness(); const path = join(root, 'mcp.json'); - const bytes = '{"secret":"do-not-display-this"'; + const bytes = 'sk-live-SECRET'; + assert.throws( + () => JSON.parse(bytes), + (error) => { + assert.ok(error instanceof SyntaxError && error.message.includes(bytes)); + return true; + }, + ); await writeFile(path, bytes); const controller = createTuiMcpController( { workspaceRoot: root, connection: connection.connection }, @@ -1437,7 +1444,7 @@ test('TUI MCP retains only the invalid persisted config path for repair guidance 'invalid MCP file to fail initialization', ); assert.equal(controller.snapshot().invalidConfigPath, path); - assert.equal(JSON.stringify(controller.snapshot()).includes('do-not-display-this'), false); + assert.equal(JSON.stringify(controller.snapshot()).includes(bytes), false); assert.equal(connection.replacements.length, 0); assert.equal(await readFile(path, 'utf8'), bytes); } finally { @@ -1494,7 +1501,14 @@ for (const kind of ['add', 'edit', 'set_enabled', 'remove', 'commit_import'] as }; const before = controller.snapshot(); order.length = 0; - const bytes = '{"secret":"never-display-this"'; + const bytes = 'sk-live-SECRET'; + assert.throws( + () => JSON.parse(bytes), + (error) => { + assert.ok(error instanceof SyntaxError && error.message.includes(bytes)); + return true; + }, + ); await writeFile(path, bytes); const result = await controller.execute(actions[kind]); assert.deepEqual(result, { status: 'failed', reason: 'invalid-config-file', path }); @@ -1503,7 +1517,7 @@ for (const kind of ['add', 'edit', 'set_enabled', 'remove', 'commit_import'] as assert.deepEqual(order, []); assert.equal(connection.unregisters, 0); assert.equal(await readFile(path, 'utf8'), bytes); - assert.equal(JSON.stringify(result).includes('never-display-this'), false); + assert.equal(JSON.stringify(result).includes(bytes), false); // An external repair makes the next explicit operation usable without a // controller restart or a stale initialization-error flag. diff --git a/packages/cli/src/pi-tui-mcp-status.ts b/packages/cli/src/pi-tui-mcp-status.ts index 4101343e95..e144f2aac1 100644 --- a/packages/cli/src/pi-tui-mcp-status.ts +++ b/packages/cli/src/pi-tui-mcp-status.ts @@ -277,12 +277,13 @@ export class McpManagementOverlay implements Component { private handleListInput(data: string): void { const snapshot = this.input.surface?.snapshot(); const servers = snapshot?.servers ?? []; - if ( - (servers.length === 0 || snapshot?.initialization === 'error') && - this.handleTextScroll(data) - ) { + if (snapshot?.initialization === 'error') { + this.handleTextScroll(data); return; } + // A ready empty list still has explanatory text below the publication + // status, which must remain reachable in a short terminal. + if (servers.length === 0 && this.handleTextScroll(data)) return; if (matchesKey(data, Key.up)) { this.selected = clamp(this.selected - 1, 0, servers.length - 1); } else if (matchesKey(data, Key.down)) { diff --git a/packages/cli/src/tui-copy-catalog.ts b/packages/cli/src/tui-copy-catalog.ts index 3d5ab30736..49ba7084f9 100644 --- a/packages/cli/src/tui-copy-catalog.ts +++ b/packages/cli/src/tui-copy-catalog.ts @@ -281,7 +281,7 @@ export const TUI_COPY_RESOURCES = { loadError: 'MCP configuration could not be loaded; no tools were published to the Runtime Host.', invalidConfigFile: - 'Invalid JSON in {path}. The file is unchanged. Close the app, back up and repair this file before retrying.', + 'Invalid JSON in {path}. The file is unchanged. Quit maka, back up and repair this file before retrying.', noServers: 'No MCP servers are configured. Press a to add one.', publication: { waiting: 'waiting to publish', @@ -389,7 +389,7 @@ export const TUI_COPY_RESOURCES = { loading: '正在读取 mcp.json 并发现工具…', loadError: '无法读取或应用 MCP 配置；没有向 Runtime Host 发布工具。', invalidConfigFile: - '{path} 中的 JSON 无效，文件未被修改。请关闭应用，备份并修复此文件后重试。', + '{path} 中的 JSON 无效，文件未被修改。请退出 maka，备份并修复此文件后重试。', noServers: '尚未配置 MCP 服务器。按 a 添加。', publication: { waiting: '等待发布', @@ -492,7 +492,7 @@ export const TUI_COPY_RESOURCES = { loading: '正在讀取 mcp.json 並探索工具…', loadError: '無法讀取或套用 MCP 設定；未向 Runtime Host 發佈任何工具。', invalidConfigFile: - '{path} 中的 JSON 無效，檔案未被修改。請關閉應用程式，備份並修復此檔案後重試。', + '{path} 中的 JSON 無效，檔案未被修改。請退出 maka，備份並修復此檔案後重試。', noServers: '尚未設定 MCP 伺服器。按 a 新增。', publication: { waiting: '等待發佈', diff --git a/packages/storage/src/__tests__/mcp-config-store.test.ts b/packages/storage/src/__tests__/mcp-config-store.test.ts index f4534f4e48..be1d0c40d4 100644 --- a/packages/storage/src/__tests__/mcp-config-store.test.ts +++ b/packages/storage/src/__tests__/mcp-config-store.test.ts @@ -638,7 +638,15 @@ async function waitUntil(condition: () => boolean, timeoutMs = 3_000): Promise { const root = await tempRoot(); const path = join(root, 'mcp.json'); - const bytes = Buffer.from('{"secret":"never-include-this"'); + const secret = 'sk-live-SECRET'; + const bytes = Buffer.from(secret); + assert.throws( + () => JSON.parse(bytes.toString('utf8')), + (error) => { + assert.ok(error instanceof SyntaxError && error.message.includes(secret)); + return true; + }, + ); await writeFile(path, bytes); const store = createMcpConfigStore(root); let transformed = false; @@ -658,7 +666,7 @@ test('corrupt persisted MCP JSON has a safe actionable error and mutations canno assert.equal(error.path, path); assert.ok(error.message.includes(path)); assert.match(error.message, /not modified.*back up and repair/u); - assert.equal(error.message.includes('never-include-this'), false); + assert.equal(error.message.includes(secret), false); assert.equal(error.cause, undefined); return true; }); From 100b7f0e319a0af556e4dc69657255bb77401ec2 Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Wed, 23 Sep 2026 15:11:03 +0800 Subject: [PATCH 5/8] fix(desktop): keep MCP IPC contract out of renderer runtime dependencies Keep the MCP failure guard beside its only renderer consumer and declare shared IPC result types in a .d.ts file. This preserves typed repair errors without adding a source module to the legacy renderer dependency closure. Leave the architecture rules and debt ledger unchanged. Verified the strict architecture check against the CI merge base, all 112 checker tests, 24 MCP regression tests, Desktop build, typecheck, lint, formatting and Knip. Refs #4285 Generated-by: OpenAI Codex --- apps/desktop/src/main/mcp-ipc-main.ts | 2 +- .../features/module-hub/model/mcp-page-model.ts | 8 +++++++- apps/desktop/src/shared/{mcp-ipc.ts => mcp-ipc.d.ts} | 10 ++-------- 3 files changed, 10 insertions(+), 10 deletions(-) rename apps/desktop/src/shared/{mcp-ipc.ts => mcp-ipc.d.ts} (72%) diff --git a/apps/desktop/src/main/mcp-ipc-main.ts b/apps/desktop/src/main/mcp-ipc-main.ts index 9a1518ac76..fb5685c979 100644 --- a/apps/desktop/src/main/mcp-ipc-main.ts +++ b/apps/desktop/src/main/mcp-ipc-main.ts @@ -265,7 +265,7 @@ export function registerMcpIpcMain(deps: McpIpcMainDeps): () => void { // `basis` is the server as the renderer last showed it, secrets redacted. A // server that no longer matches it was changed elsewhere (the TUI edits the // same file), and saving over it would silently drop that change. - deps.ipcMain.handle( + handle( 'mcp:update', (_event, serverId: string, config: McpServerConfig, basis: McpServerConfig) => updateServer(serverId, (current, previous) => { diff --git a/apps/desktop/src/renderer/features/module-hub/model/mcp-page-model.ts b/apps/desktop/src/renderer/features/module-hub/model/mcp-page-model.ts index 26395ff393..5657e1d2bb 100644 --- a/apps/desktop/src/renderer/features/module-hub/model/mcp-page-model.ts +++ b/apps/desktop/src/renderer/features/module-hub/model/mcp-page-model.ts @@ -24,9 +24,15 @@ import type { } from '@maka/core/mcp'; import { isMcpStdioConfig, resolveMcpProtocolPreference } from '@maka/core/mcp'; import type { McpCopy } from '../../../locales/mcp-copy.js'; -import { isMcpConfigFileFailure, type McpIpcResult } from '../../../../shared/mcp-ipc.js'; +import type { McpConfigFileFailure, McpIpcResult } from '../../../../shared/mcp-ipc.js'; import { formatCommandLine, parseCommandLine } from './mcp-command-line.js'; +function isMcpConfigFileFailure(value: unknown): value is McpConfigFileFailure { + return typeof value === 'object' && value !== null && + 'kind' in value && value.kind === 'invalid-mcp-config-file' && + 'path' in value && typeof value.path === 'string'; +} + class McpConfigFileError extends Error { constructor(readonly path: string) { super('Invalid persisted MCP configuration'); diff --git a/apps/desktop/src/shared/mcp-ipc.ts b/apps/desktop/src/shared/mcp-ipc.d.ts similarity index 72% rename from apps/desktop/src/shared/mcp-ipc.ts rename to apps/desktop/src/shared/mcp-ipc.d.ts index 2c32938461..70be5e2ba4 100644 --- a/apps/desktop/src/shared/mcp-ipc.ts +++ b/apps/desktop/src/shared/mcp-ipc.d.ts @@ -17,8 +17,8 @@ * under the License. */ -/** MCP config failures travel as data across both Electron IPC and the - * context bridge, which do not preserve custom Error properties. Successful +/** Type-only wire contract. MCP config failures travel as data across Electron + * IPC and the context bridge, which do not preserve custom Error properties. Successful * values keep their existing shape; pasted JSON validation is a separate * McpConfigImportResult, not a persisted-file failure. */ export interface McpConfigFileFailure { @@ -27,9 +27,3 @@ export interface McpConfigFileFailure { } export type McpIpcResult = T | McpConfigFileFailure; - -export function isMcpConfigFileFailure(value: unknown): value is McpConfigFileFailure { - return typeof value === 'object' && value !== null && - 'kind' in value && value.kind === 'invalid-mcp-config-file' && - 'path' in value && typeof value.path === 'string'; -} From fa3f3f8201dbaccdd586c141d5e165ab35cd679d Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Wed, 23 Sep 2026 17:05:49 +0800 Subject: [PATCH 6/8] test(desktop): mock Runtime Host readiness in MCP preload test Handle the upstream runtime-host:awaitReady handshake and verify its target scope before exercising typed MCP config failures. Generated-by: OpenAI Codex --- apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts b/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts index 45d28d1a68..fe10f51974 100644 --- a/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts +++ b/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts @@ -70,6 +70,10 @@ test('every MCP bridge method carries typed config failures intact through the b async invoke(channel: string, ...args: unknown[]) { if (channel === 'app:bootstrapReady') return undefined; if (channel === 'runtime-host:identities') return structuredClone([owner]); + if (channel === 'runtime-host:awaitReady') { + assert.deepEqual(JSON.parse(JSON.stringify(args[0])), owner); + return { ready: true }; + } assert.ok(channel.startsWith('mcp:'), channel); assert.deepEqual(JSON.parse(JSON.stringify(args[0])), owner); channels.push(channel); From d4bf0ad734bb32b7f8e2a4ed14d71b736d321b38 Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Thu, 24 Sep 2026 04:49:12 +0800 Subject: [PATCH 7/8] fix(mcp): accept UTF-8 BOM and retain late repair diagnostics Accept a leading UTF-8 BOM on MCP reads and imports, and retain late file diagnostics after dismissing the TUI busy view without interrupting drafts or newer operations. Document the UTF-8 configuration contract and verify typed repair errors through the rebased Module Hub controller. Generated-by: OpenAI Codex --- README.md | 1 + README.zh-CN.md | 1 + .../__tests__/mcp-ipc-commit-unknown.test.ts | 2 +- .../module-hub-mcp-controller.test.ts | 49 ++++++- .../src/__tests__/pi-tui-mcp-status.test.ts | 127 ++++++++++++++++++ packages/cli/src/pi-tui-mcp-status.ts | 32 ++++- .../src/__tests__/mcp-config-store.test.ts | 50 +++++++ packages/storage/src/mcp-config-store.ts | 10 +- 8 files changed, 263 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 82fcc0c17c..345eb1b2a4 100644 --- a/README.md +++ b/README.md @@ -194,6 +194,7 @@ Workspace data lives under Electron `userData` by default: artifacts/ ``` +- When editing `settings.json` or `mcp.json` by hand, save the file as UTF-8 (preferably without a BOM). A UTF-8 BOM is accepted. Maka does not guess other encodings for files without a BOM or automatically convert UTF-16; explicitly convert those files to UTF-8 in your editor before using them. - API keys and similar secrets are a local plaintext file (`credential-vault.json`), readable only by your OS account. The renderer never sees them. - Tools that write files or run a shell must pass the sandbox boundary first. - `runtime.sqlite` is the live record. Older JSONL transcripts and Electron `safeStorage` credential files are not imported; an upgraded workspace can show empty threads, and those credentials must be entered again. diff --git a/README.zh-CN.md b/README.zh-CN.md index fc8a4bfdd9..1f459dc476 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -193,6 +193,7 @@ Workspace 数据默认放在 Electron `userData` 下： artifacts/ ``` +- 手动编辑 `settings.json` 或 `mcp.json` 时，请保存为 UTF-8（建议不带 BOM；也支持 UTF-8 BOM）。Maka 不会猜测无 BOM 文件的其他编码，也不会自动转换 UTF-16；请先在编辑器中显式转换为 UTF-8，再使用这些文件。 - API key 一类的机密存在本地明文文件（`credential-vault.json`），只有你的系统账号能读。界面进程拿不到明文。 - 写文件、跑 Shell 的工具必须先过沙箱边界。 - `runtime.sqlite` 是当前生效的那份记录。更早的 JSONL transcript 和 Electron `safeStorage` 凭据不会导入；升级后会话可能是空的，那些凭据需要重新填写。 diff --git a/apps/desktop/src/main/__tests__/mcp-ipc-commit-unknown.test.ts b/apps/desktop/src/main/__tests__/mcp-ipc-commit-unknown.test.ts index 587e23164e..e9e6328a55 100644 --- a/apps/desktop/src/main/__tests__/mcp-ipc-commit-unknown.test.ts +++ b/apps/desktop/src/main/__tests__/mcp-ipc-commit-unknown.test.ts @@ -253,7 +253,7 @@ test('MCP IPC transports corrupt-file details for reads and every mutation witho return true; }); await writeFile(path, source); - const ipc = mutationHarness(store); + const ipc = mutationHarness(t, store); const calls: [string, ...unknown[]][] = [ ['mcp:getConfig'], ['mcp:importConfig', '{"new":{"command":"unused"}}'], diff --git a/apps/desktop/src/main/__tests__/module-hub-mcp-controller.test.ts b/apps/desktop/src/main/__tests__/module-hub-mcp-controller.test.ts index 06fad01c1f..d1a0d21cbc 100644 --- a/apps/desktop/src/main/__tests__/module-hub-mcp-controller.test.ts +++ b/apps/desktop/src/main/__tests__/module-hub-mcp-controller.test.ts @@ -22,7 +22,8 @@ import { afterEach, test } from 'node:test'; import { act, createElement } from 'react'; import { deferred } from '@maka/core/test-only/async-primitives'; import { createDefaultMcpConfig, type McpConfigFile, type McpServerStatus } from '@maka/core/mcp'; -import { createFakeModuleHubServices, ModuleHubServicesProvider, useMcpController } from '../../renderer/features/module-hub/testing.js'; +import { mcpConfigFailureMessage, createFakeModuleHubServices, ModuleHubServicesProvider, useMcpController } from '../../renderer/features/module-hub/testing.js'; +import { getMcpCopy } from '../../renderer/locales/mcp-copy.js'; import { cleanupFakeDom, installReactRenderer } from './fake-dom.js'; afterEach(cleanupFakeDom); @@ -178,3 +179,49 @@ test('MCP follows a configured Chrome server until its extension connects', asyn await act(async () => { t.mock.timers.tick(10_000); }); assert.equal(reads, settled); }); + +test('MCP controller decodes corrupt-file IPC data on load and every action', async () => { + const { root } = installReactRenderer(); + const defaults = createFakeModuleHubServices(); + const failure = { kind: 'invalid-mcp-config-file', path: '/profile/mcp.json' } as const; + let badRead = true; + const services = createFakeModuleHubServices({ mcp: { + ...defaults.mcp, + getConfig: async () => badRead ? structuredClone(failure) : createDefaultMcpConfig(), + add: async () => structuredClone(failure), + update: async () => structuredClone(failure), + setEnabled: async () => structuredClone(failure), + importConfig: async () => structuredClone(failure), + remove: async () => structuredClone(failure), + test: async () => structuredClone(failure), + login: async () => structuredClone(failure), + cancelLogin: async () => structuredClone(failure), + logout: async () => structuredClone(failure), + } }); + let controller!: ReturnType; + function Probe() { controller = useMcpController(); return null; } + await act(async () => root.render(createElement(ModuleHubServicesProvider, { services }, createElement(Probe)))); + const assertDiagnostic = () => { + assert.ok(controller.error instanceof Error); + assert.equal(mcpConfigFailureMessage(controller.error, getMcpCopy('en')), getMcpCopy('en').errors.invalidConfigFile(failure.path)); + assert.deepEqual(controller.config, createDefaultMcpConfig()); + assert.equal(controller.busy, null); + }; + assertDiagnostic(); + badRead = false; + const server = { command: 'node' }; + for (const action of [ + () => controller.add('id', server), + () => controller.update('id', server, server), + () => controller.setEnabled('id', true), + () => controller.importConfig('{}'), + () => controller.remove('id'), + () => controller.test('id'), + () => controller.login('id'), + () => controller.cancelLogin('id'), + () => controller.logout('id'), + ]) { + await act(async () => { assert.equal(await action(), undefined); }); + assertDiagnostic(); + } +}); diff --git a/packages/cli/src/__tests__/pi-tui-mcp-status.test.ts b/packages/cli/src/__tests__/pi-tui-mcp-status.test.ts index 8c2c5e77b7..0542ad804d 100644 --- a/packages/cli/src/__tests__/pi-tui-mcp-status.test.ts +++ b/packages/cli/src/__tests__/pi-tui-mcp-status.test.ts @@ -743,3 +743,130 @@ test('a ready empty list scrolls to its add guidance in a short terminal and rem overlay.handleInput('a'); assert.doesNotMatch(render(), /not published/u); }); + +for (const destination of ['list', 'input', 'closed', 'newer-success', 'newer-error'] as const) { + test(`late MCP file diagnostics after Esc respect ${destination}`, async () => { + let resolve!: (value: TuiMcpActionResult) => void; + const pending = new Promise((done) => { + resolve = done; + }); + const mcp = surface(listSnapshot()); + mcp.execute = () => pending; + let changes = 0; + let closed = false; + const overlay = new McpManagementOverlay({ + locale: 'en', + tui: fakeTui(), + surface: mcp, + viewportRows: () => 20, + onClose: () => { + closed = true; + }, + onChange: () => { + changes++; + }, + }); + const render = () => overlay.render(160).map(stripAnsi).join('\n'); + render(); + overlay.handleInput(' '); + overlay.handleInput('\u001b'); + assert.match(render(), /filesystem/u); + if (destination === 'input') { + overlay.handleInput('a'); + overlay.handleInput('j'); + overlay.handleInput('{"draft":'); + } else if (destination === 'closed') { + overlay.handleInput('q'); + } else if (destination.startsWith('newer-')) { + mcp.execute = async () => + destination === 'newer-success' + ? { status: 'applied', effect: 'published' } + : { status: 'failed', reason: 'invalid-config-file', path: '/newer/mcp.json' }; + overlay.handleInput(' '); + await new Promise((done) => setImmediate(done)); + } + const before = render(); + const priorChanges = changes; + resolve({ status: 'failed', reason: 'invalid-config-file', path: '/late/mcp.json' }); + await new Promise((done) => setImmediate(done)); + if (destination === 'closed' || destination.startsWith('newer-')) { + assert.equal(changes, priorChanges); + assert.equal(render(), before); + assert.equal(closed, destination === 'closed'); + assert.doesNotMatch(render(), /\/late\/mcp.json/u); + } else { + if (destination === 'input') { + assert.equal(render(), before, 'the draft must remain intact'); + overlay.handleInput('\u001b'); + } + assert.match(render(), /\/late\/mcp.json/u); + assert.match(render(), /back up and repair/u); + overlay.handleInput('\u001b'); + assert.equal(closed, false); + assert.match(render(), /filesystem/u); + assert.doesNotMatch(render(), /\/late\/mcp.json/u); + } + }); +} + +test('Esc during busy still discards an ordinary late result without interrupting a draft', async () => { + let resolve!: (value: TuiMcpActionResult) => void; + const mcp = surface(listSnapshot()); + mcp.execute = () => + new Promise((done) => { + resolve = done; + }); + const overlay = new McpManagementOverlay({ + locale: 'en', + tui: fakeTui(), + surface: mcp, + viewportRows: () => 20, + onClose: () => {}, + onChange: () => {}, + }); + const render = () => overlay.render(160).map(stripAnsi).join('\n'); + render(); + overlay.handleInput(' '); + overlay.handleInput('\u001b'); + overlay.handleInput('a'); + overlay.handleInput('j'); + overlay.handleInput('unfinished draft'); + const before = render(); + resolve({ status: 'applied', effect: 'published' }); + await new Promise((done) => setImmediate(done)); + assert.equal(render(), before); +}); + +for (const succeeds of [true, false]) { + test(`a deferred diagnostic is ${succeeds ? 'cleared by a successful write' : 'retained after another failure'}`, async () => { + let resolve!: (value: TuiMcpActionResult) => void; + const mcp = surface(listSnapshot()); + mcp.execute = () => + new Promise((done) => { + resolve = done; + }); + const overlay = new McpManagementOverlay({ + locale: 'en', + surface: mcp, + viewportRows: () => 20, + onClose: () => {}, + onChange: () => {}, + }); + const render = () => overlay.render(160).map(stripAnsi).join('\n'); + render(); + overlay.handleInput(' '); + overlay.handleInput('\u001b'); + overlay.handleInput('d'); + const confirmation = render(); + resolve({ status: 'failed', reason: 'invalid-config-file', path: '/late/mcp.json' }); + await new Promise((done) => setImmediate(done)); + assert.equal(render(), confirmation); + mcp.execute = async () => + succeeds + ? { status: 'applied', effect: 'published' } + : { status: 'failed', reason: 'manager-failed' }; + overlay.handleInput('y'); + await new Promise((done) => setImmediate(done)); + assert.equal(render().includes('/late/mcp.json'), !succeeds); + }); +} diff --git a/packages/cli/src/pi-tui-mcp-status.ts b/packages/cli/src/pi-tui-mcp-status.ts index e144f2aac1..0e9a976969 100644 --- a/packages/cli/src/pi-tui-mcp-status.ts +++ b/packages/cli/src/pi-tui-mcp-status.ts @@ -173,6 +173,7 @@ export class McpManagementOverlay implements Component { private editor: OverlayTextInput | undefined; private closed = false; private actionAttempt = 0; + private pendingConfigErrorPath: string | undefined; constructor( private readonly input: { @@ -203,10 +204,8 @@ export class McpManagementOverlay implements Component { } if (this.phase.kind === 'busy') { if (matchesKey(data, Key.escape)) { - this.actionAttempt += 1; this.backToList(); } else if (matchesKey(data, 'q')) { - this.actionAttempt += 1; this.close(); } return; @@ -494,14 +493,24 @@ export class McpManagementOverlay implements Component { } if (this.closed || attempt !== this.actionAttempt) return; if (result.status === 'failed' && result.reason === 'invalid-config-file') { - this.phase = { kind: 'config_error', path: result.path }; - this.notice = undefined; - this.top = 0; + // Esc dismisses the busy view, not the operation or its file diagnostic. + // Defer presentation while the user is editing another form. + this.pendingConfigErrorPath = result.path; + this.showPendingConfigError(); this.input.onChange(); return; } + if ( + result.status === 'applied' && + ['add', 'edit', 'commit_import', 'set_enabled', 'remove'].includes(action.kind) + ) { + this.pendingConfigErrorPath = undefined; + } + // Do not restore a dismissed busy view for ordinary completion results. + if (this.phase.kind !== 'busy') return; this.phase = { kind: 'list' }; this.notice = actionNotice(result, this.input.locale); + this.showPendingConfigError(); this.input.onChange(); } @@ -619,9 +628,22 @@ export class McpManagementOverlay implements Component { this.clearEditor(); this.phase = { kind: 'list' }; if (clearNotice) this.notice = undefined; + this.showPendingConfigError(); this.input.onChange(); } + private showPendingConfigError(): void { + if ( + this.pendingConfigErrorPath === undefined || + (this.phase.kind !== 'list' && this.phase.kind !== 'busy') + ) + return; + this.phase = { kind: 'config_error', path: this.pendingConfigErrorPath }; + this.pendingConfigErrorPath = undefined; + this.notice = undefined; + this.top = 0; + } + private clearEditor(): void { if (!this.editor) return; this.editor.focused = false; diff --git a/packages/storage/src/__tests__/mcp-config-store.test.ts b/packages/storage/src/__tests__/mcp-config-store.test.ts index be1d0c40d4..c21a56d19e 100644 --- a/packages/storage/src/__tests__/mcp-config-store.test.ts +++ b/packages/storage/src/__tests__/mcp-config-store.test.ts @@ -683,3 +683,53 @@ test('corrupt persisted MCP JSON has a safe actionable error and mutations canno }, ); }); + +for (const prefix of ['', '\uFEFF']) { + test(`reads MCP UTF-8 JSON ${prefix ? 'with' : 'without'} a BOM without rewriting it`, async () => { + const root = await tempRoot(); + const path = join(root, 'mcp.json'); + const config = { + version: 3, + mcpServers: { example: { command: 'node', args: ['\uFEFFdata'] } }, + }; + const bytes = Buffer.from(prefix + JSON.stringify(config)); + await writeFile(path, bytes); + assert.deepEqual(await createMcpConfigStore(root).get(), normalizeMcpConfig(config)); + assert.deepEqual(await readFile(path), bytes); + for (const source of [config, config.mcpServers]) { + assert.deepEqual( + normalizeMcpImport(prefix + JSON.stringify(source)), + normalizeMcpConfig(config), + ); + } + }); +} + +test('a UTF-8 BOM does not bypass MCP syntax checks or permit overwriting corrupt data', async () => { + const root = await tempRoot(); + const path = join(root, 'mcp.json'); + const store = createMcpConfigStore(root); + for (const source of ['\uFEFF{"token":"sk-private",', '\uFEFF\uFEFF{"mcpServers":{}}']) { + const bytes = Buffer.from(source); + await writeFile(path, bytes); + for (const operation of [ + () => store.get(), + () => store.upsert('new', { command: 'node' }), + () => + store.transform(() => { + throw new Error('must not reach transform'); + }), + ]) { + await assert.rejects(operation(), (error) => { + assert.ok(error instanceof McpConfigSourceError); + assert.equal(error.reason, 'invalid-json'); + assert.equal(error.path, path); + assert.equal(error.message.includes('sk-private'), false); + assert.equal(error.cause, undefined); + return true; + }); + assert.deepEqual(await readFile(path), bytes); + } + assert.throws(() => normalizeMcpImport(source), { reason: 'invalid-json', path: undefined }); + } +}); diff --git a/packages/storage/src/mcp-config-store.ts b/packages/storage/src/mcp-config-store.ts index 56cea28a0a..e2872f223e 100644 --- a/packages/storage/src/mcp-config-store.ts +++ b/packages/storage/src/mcp-config-store.ts @@ -181,6 +181,12 @@ export function normalizeMcpConfig(value: unknown): McpConfigFile { return { version: MCP_CONFIG_VERSION, mcpServers: { ...mcpServers } }; } +// Accept an optional UTF-8 BOM at the document boundary only. Keep size checks +// on the original input and leave all other JSON/schema validation unchanged. +function parseMcpJson(source: string): unknown { + return JSON.parse(source.startsWith('\uFEFF') ? source.slice(1) : source); +} + /** Parse either a wrapped mcp.json document or a direct server map while * preserving the source wrapper version until schema validation completes. * Import presentation belongs to the caller; config interpretation lives here @@ -191,7 +197,7 @@ export function normalizeMcpImport(source: string): McpConfigFile { } let value: unknown; try { - value = JSON.parse(source); + value = parseMcpJson(source); } catch { throw new McpConfigSourceError('invalid-json', undefined, 'MCP config must be valid JSON'); } @@ -278,7 +284,7 @@ class FileMcpConfigStore implements McpConfigStore { } let persisted: unknown; try { - persisted = JSON.parse(text); + persisted = parseMcpJson(text); } catch (error) { if (!(error instanceof SyntaxError)) throw error; // JSON.parse can quote credentials in its message. Report the location From f1b26dcc06614d44df25526b60ce4322a2d6e2c3 Mon Sep 17 00:00:00 2001 From: chinawch007 Date: Fri, 25 Sep 2026 02:11:04 +0800 Subject: [PATCH 8/8] test(desktop): align MCP preload mock with runtime host identity --- apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts b/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts index fe10f51974..8e12b6eb7d 100644 --- a/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts +++ b/apps/desktop/src/main/__tests__/mcp-preload-scope.test.ts @@ -69,13 +69,15 @@ test('every MCP bridge method carries typed config failures intact through the b on: events.on.bind(events), off: events.off.bind(events), send() {}, async invoke(channel: string, ...args: unknown[]) { if (channel === 'app:bootstrapReady') return undefined; - if (channel === 'runtime-host:identities') return structuredClone([owner]); + if (channel === 'runtime-host:identities') { + return structuredClone([{ ...owner, epoch: owner.targetEpoch, isDefault: true }]); + } if (channel === 'runtime-host:awaitReady') { - assert.deepEqual(JSON.parse(JSON.stringify(args[0])), owner); + assert.deepEqual(JSON.parse(JSON.stringify(args[0])), { hostId: owner.hostId, targetEpoch: owner.targetEpoch }); return { ready: true }; } assert.ok(channel.startsWith('mcp:'), channel); - assert.deepEqual(JSON.parse(JSON.stringify(args[0])), owner); + assert.deepEqual(JSON.parse(JSON.stringify(args[0])), { hostId: owner.hostId, targetEpoch: owner.targetEpoch }); channels.push(channel); return structuredClone(failure); },