fix(storage): storage settings corrupt recovery - #5282
Conversation
aabd93c to
fab6ca4
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for following #4505 through to the read side. Reviewed at head fab6ca4. Lint and format pass; the compiled storage (52), Desktop (41) and CLI (68) suites in scope all pass here. The mcp.json half is in good shape: the typed McpConfigSourceError, the untouched file, and the mcp-ipc-commit-unknown test that runs a real store through the IPC message and asserts the secret never reaches the error are exactly the right kind of coverage. The TUI config_error phase with scrolling is needed for the guidance to be readable in small terminals, so I don't see that as scope creep.
The settings.json half I'd like to step back on before looking at more detail.
1. The recovery policy hasn't been decided yet. Your plan comment on #4285 asked maintainers to veto option A (backup, reset to defaults, notify) before PR2; nobody has replied, and silence isn't a decision. Resetting preferences, bot configuration and onboarding state without the user confirming is an interaction change, so I think it needs a maintainer answer on the issue first. It's also worth noting that this PR already contains the cheaper alternative for mcp.json (typed error, file untouched, guidance to repair), and the description doesn't say why the two files get different treatment. A reasonable middle ground for settings is: boot on in-memory defaults, show one in-app notice that names the file, and move the file aside only when the user says so. That keeps the "don't brick startup" fix and drops most of the new machinery. I'm not asking for that specific design, only that #4285 settles it.
2. On the startup path the user gets no signal. This is the path #4285 describes. settingsStore.get() first runs at runtime-host-boot.ts:465/:919, settingsRecovery.setEffects at :1014, and the main window is created at :2161. safeSendToRenderer (main-window.ts:133) drops the event when no window exists, and rendererFingerprint has already advanced, so nothing is re-sent when the window appears. Even if it were delivered, settings:clientChanged only tells the renderer to reread; none of its four subscribers knows a reset happened or where the backup is. That leaves the native notification as the only channel, and the description reports it returned failed in your own smoke run. So today the outcome is a silent reset with a backup the user doesn't know about, which is harder to recover from than the original failure. Whatever policy #4285 lands on, the fix needs an in-app surface.
3. The reporter's refresh path duplicates an existing authority. startClientSettingsWatcher (client-settings-watcher.ts:47, wired at runtime-host-boot.ts:1050) already calls clientSettingsEffects.refresh(true) whenever settings.json changes, and recovery publishes through this.write(), so a runtime recovery already triggers it. On startup there is no stale state to refresh: the boot refresh(false) at :1552 applies the recovered defaults. That makes the pending/setEffects/refresh latch in settings-recovery.ts and the fingerprint split in client-settings-effects.ts removable, together with their tests. Removing them also removes the new behavior where the first refresh(true) after boot emits settings:externalChanged even when nothing changed.
With 1–3 settled, the smallest complete shape I can see for the settings side is roughly: the SyntaxError branch in readOrCreate, a backup by rename, the recovery callback, one notification hook in boot, and the tests that prove bytes are preserved and defaults are written. The SettingsRecoveryError phase/incompleteBackupPath fields and the commit-unknown subclass, outcome and copy have no consumer that branches on them (and commit-unknown is unreachable on Windows where syncDirectory is a no-op), so they would go with it.
Line-level notes are inline. Two description items: the before/after TUI screenshots are still an HTML placeholder, and it would help to state that the realistic trigger for a corrupt settings.json after #4505 is a file written by a pre-#4505 build, since the fsync fix closed the crash window going forward.
AI assistance: I used Claude Code to trace the boot ordering, reproduce the backup and TOCTOU cases, and run the suites; each finding here was checked by me against the code.
| const backupPath = await this.backupCorruptSettings(bytes); | ||
| const settings = createDefaultSettings(); | ||
| try { | ||
| await this.write(settings); |
There was a problem hiding this comment.
P2. Between readFile and this write, the file on disk can change (the natural sequel to #4285 is a user fixing the file in an editor while the app starts). The backup is taken from the bytes read earlier and this write replaces whatever is on disk now, so a repaired file is overwritten by defaults and appears in no backup. I reproduced this by swapping in valid JSON right after the backup handle opened. readStableBoundedFile in stable-storage.ts already does the dev/ino/size/mtime check that would catch this; asserting the source is unchanged before the reset, and retrying the read otherwise, closes it.
| * 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<string> { | ||
| const backupPath = `${this.settingsPath}.corrupt-${Date.now()}-${randomUUID()}`; |
There was a problem hiding this comment.
P2. Every call gets a fresh name and a failed reset isn't remembered, so while the reset keeps failing (rename refused because the file is locked or immutable, while the directory is still writable) every get()/update() writes another full copy. Five failing reads gave me six files. Remembering the backup path on the store after a failed reset, or reusing an identical existing backup, bounds it. Separately: a plain rename(settingsPath, backupPath) would replace this whole open/write/chmod/sync/cleanup sequence with one atomic step and no fsync; the ENOENT window it opens is microseconds and its outcome (defaults on next start) is the same outcome recovery produces anyway.
| assert.equal(error.settingsPath, path); | ||
| assert.equal(error.backupPath, undefined); | ||
| assert.equal(error.incompleteBackupPath, undefined); | ||
| assert.equal(error.message.includes('do-not-log'), false); |
There was a problem hiding this comment.
P3. This assertion (and the one at 413, and never-include-this in the mcp test) can't fail on the implementation it's guarding against: for a truncated object V8 reports a position-only message with no source excerpt, so 'invalid JSON: ' + error.message also passes. A fixture whose parse error embeds the token, e.g. Buffer.from('sk-live-SECRET') or {"a":sk-live-SECRET}, makes it a real check.
| }); | ||
|
|
||
| it('creates defaults only when settings.json is missing', async () => { | ||
| it('creates defaults without a backup when settings.json is missing', async () => { |
There was a problem hiding this comment.
P3. The title now promises "without a backup" but the body didn't change and never lists the directory; the real check for that lives in the new recovery file. Suggest keeping the old title.
| * the file after the storage operation releases its own queue. */ | ||
| export function createSettingsRecoveryReporter(deps: SettingsRecoveryReporterDependencies) { | ||
| let effects: Pick<ClientSettingsEffects, 'refresh'> | undefined; | ||
| let pending: CorruptSettingsRecovery | undefined; |
There was a problem hiding this comment.
See point 3 in the top-level comment: the settings watcher already refreshes on the recovery write, and boot's refresh(false) covers startup, so this latch and refresh() below can go.
| // 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); |
There was a problem hiding this comment.
P3. This raises a second system banner for the same event (the test asserts two). The log line above already records the refresh failure; one banner is enough.
| rendererFingerprint = nextRendererFingerprint; | ||
| if (notifyRenderer && rendererChanged) dependencies.emitExternalChanged(); | ||
| return rendererChanged || keepAwakeChanged || botChanged || appIconChanged; | ||
| appliedSettingsFingerprint = nextRendererFingerprint; |
There was a problem hiding this comment.
P2. After this split, boot's refresh(false) no longer advances rendererFingerprint, so the first refresh(true) after startup emits settings:externalChanged even when settings are unchanged. The comment also says "delivery" but emitExternalChanged goes through safeSendToRenderer, which drops silently without a window, so what is tracked is the emit call, not delivery. If the reporter's refresh path goes (top-level point 3), this change isn't needed.
| * 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); |
There was a problem hiding this comment.
P3. This turns the English wording of the storage error into a cross-process contract. McpConfigSourceError already carries reason and path, and the note on McpServerExistsError in the same store describes the intended pattern (IPC matches on instanceof and answers with a typed envelope). The CLI side does it that way. The new mcp-ipc-commit-unknown test will catch wording drift, so this isn't blocking, but the typed route is cheaper to keep.
| 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; |
There was a problem hiding this comment.
P2. The footer now says read-only in the error state, but handleListInput still accepts a, p, x, Enter, Space, t, r, d there. Either gate those keys in the error state or keep the manage footer; the current combination promises less than it does.
| 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.', |
There was a problem hiding this comment.
P3. "Close the app" is Desktop wording; in the terminal this should say quit maka (same for zh-CN/zh-TW).
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed the corrupt-settings recovery path end to end (packages/storage/src/settings-store.ts, apps/desktop/src/main/settings-recovery.ts, apps/desktop/src/main/client-settings-effects.ts) plus the read-only MCP half. I did not read the TUI rendering changes line by line.
What I verified on the recovery path
Detection is deliberately narrow. readOrCreate diverts to recovery only for SyntaxError from JSON.parse (settings-store.ts:174-178). EACCES/EIO still propagate, a missing file still creates defaults with no backup, and valid JSON that fails normalization or the legacy-proxy migration is left completely alone. Covered by read ${code} does not reset or create backups, missing and valid JSON keep their normal behavior without a backup, normalization errors are not interpreted as invalid JSON, and a migration write failure (${code}) cannot trigger creation or recovery.
Nothing is destroyed before the backup is durable. Backup first, publish second: backupCorruptSettings (settings-store.ts:211-240) writes the exact bytes through an exclusive open(backupPath, 'wx', 0o600), re-chmods on POSIX, fsyncs the file and then the parent directory, and only afterwards does recoverCorruptSettings call the shared atomic writer. Because writeAtomicFile publishes with rename, settings.json is never missing and the corrupt bytes exist in two places until the rename lands. The per-phase fault injection (open / writeFile / chmod / sync / close / directory) asserts the source bytes plus a single-entry directory afterwards, and reset publication failure retains the complete backup and never reports success asserts the backup survives a failed rename.
Secrets stay out of errors and logs. The SyntaxError is intentionally not attached as cause (settings-store.ts:176) and McpConfigSourceError composes its message from the path only (mcp-config-store.ts:193-201). Tests assert the planted secret never reaches an error message, a notification body, or the reporter's log lines.
Commit-unknown is separated from success and the caller's mutation is not replayed. SettingsRecoveryCommitUnknownError (settings-store.ts:77-88) is reported to the observer before being thrown, and since recovery happens inside readOrCreate, the caller's patch is never evaluated — post-publication failure remains commit-unknown through ${mutation} asserts predicateCalls === 0 for updateIf.
Idempotent and race-safe. A second get() sees the published defaults and does not re-backup (events.length === 1, directory holds exactly two entries), and 16 concurrent get() calls produce exactly one recovery because everything runs behind withQueue. reportRecovery (settings-store.ts:244-252) neither awaits nor propagates the observer, so a re-entrant store.get() from the callback cannot deadlock the queue — there is a test for that case too.
Desktop wiring holds up. onRecovery notifies, stashes the event, and defers refresh(true) to a microtask (settings-recovery.ts:130-152) so the storage queue is released first; a recovery observed before setEffects is replayed exactly once effects exist. Deferring rendererFingerprint until an actual delivery (client-settings-effects.ts:104-111) is the right fix for the swallowed reset event — the real-store integration test asserts exactly one emitExternalChanged for both notifyRenderer values, and the notification is still suppressed under e2e while diagnostics and the refresh are not.
MCP stays read-only. The new invalid-JSON error is thrown only from FileMcpConfigStore.read (mcp-config-store.ts:186-201); normalizeMcpImport keeps its generic message, so pasted-JSON mistakes do not get "back up and repair this file" advice (asserted via error.path === undefined). get/transform/upsert/remove all refuse to write over the corrupt file and the bytes are unchanged afterwards.
Conventions. ASF headers on both new files, UiCatalog/UI_LOCALES copy shape, the package's existing t.mock.method + syncBuiltinESMExports() fault-injection style, and the generated Windows inventory is internally consistent: the diff adds exactly five process.platform === 'win32' skips and five inventory rows, with portable-candidate 32→37 and the total 95→100.
Nits
-
A UTF-8 BOM is treated as corruption and resets the file.
JSON.parse('\uFEFF{…}')throwsSyntaxError(I checked against this Node), so asettings.jsonthat a Windows editor re-saved with a BOM lands in recovery and is reset to defaults. No bytes are lost — there is a backup and a notification — but the file was recoverable. Stripping a leading BOM before the parse inreadOrCreatewould remove the false positive; the same argument applies to a UTF-16 re-encode, which decodes to NULs and also ends up in recovery. -
A symlinked
settings.jsonloses its link. Recovery publishes withrename, which replaces the symlink itself rather than its target, so a dotfile-managed config survives on disk but stops being what the app reads, and the backup stores bytes rather than the link. Following the symlink on read is pre-existing (readFile), so this is not a read regression — but the new write makes it destructive. Elsewhere in this packagereadStableBoundedFileopens final JSON documentsO_NOFOLLOWand fails closed onELOOP(stable-storage.ts:192-201); anlstatguard before the reset (or preserving the link in the backup) would match that posture. -
Backup retention. Backups are never pruned and there is no cap: each read after a failed reset retries the whole recovery and adds another byte-identical backup. Relatedly, if
syncDirectoryfails after the backup file was fully written and closed, the cleanup atsettings-store.ts:227-236deletes a complete backup and the resulting error says "No complete backup was confirmed" — accurate about the fence, but it discards a file that was actually written. -
refresh()'s return value changed meaning without a doc comment.client-settings-effects.ts:112now returnssettingsChanged || …("something was applied") instead of "the renderer was notified". No production caller reads it (runtime-host-boot.ts:1020,1053,1551all discard the boolean), so this is contract clarity only. Worth stating the second-order effect, though: because a silent apply no longer consumes the change, the first notifying refresh after one always emitssettings:clientChangedeven when the snapshot is unchanged (bootrefresh(false)atruntime-host-boot.ts:1551, then a watcher-drivenrefresh(true)), costing one extrarefreshShellSettings()in the renderer. Looks intended — confirming rather than objecting. -
Drive-by in
mcp-config-store.test.ts:['readadmin']→['read\u0001admin']replaces a raw0x01byte that had been sanitized out of the source (I diffed the base file — the control character is there). That restores the intended test input, but it is unrelated to this PR's scope and deserves a sentence in the description. -
TUI key routing:
handleListInputsends arrows and paging tohandleTextScrollwheneverservers.length === 0(pi-tui-mcp-status.ts:280-283), which also covers a legitimately empty server list rather than just the corrupt-config state. Narrowing the condition tosnapshot?.initialization === 'error'would keep the two cases distinct.
One question
Valid JSON that fails normalizeSettings still hard-fails with no backup and no guidance. Given that normalizeSettings returns defaults for non-objects and merges section by section (packages/core/src/settings.ts:974-996), the practical exposure looks small and the PR documents the choice — I just want to confirm "only SyntaxError recovers" is the intended line and not an oversight.
fab6ca4 to
374389d
Compare
|
The two reviews contain several inconsistent assessments or directions:
The last group does not explicitly withdraw the earlier simplification requests. I have addressed the concrete problems from both reviews rather than treating the later assessment as approval. References: first review, second review. Why I kept copying the backupI agree that However, the proposed rename followed by the existing default-settings writer changes the recovery failure contract:
A complete rename-based quarantine/recreate protocol is a reasonable alternative. For this read-triggered recovery, I prefer keeping the source in place and fixing the specific weaknesses of copying. The revision validates the source immediately before publication and reuses a verified backup for identical corrupt bytes. It does not retain the original unguarded, fresh-backup-on-every-retry behavior. Fingerprints and refresh ownershipI accepted the first review's point about the reporter duplicating refresh ownership: it no longer waits for effects initialization, exposes I did not collapse the applied-settings and renderer-event fingerprints. A silent The initial silent application now establishes the renderer baseline, avoiding the extra unchanged event after boot. Subsequent silent changes do not consume a pending renderer event. The code distinguishes an attempted send from a send with an available recipient; it does not claim renderer acknowledgment. The startup recovery explanation is a separate queued application dialog, so a settings-change event is no longer expected to carry recovery details. I also documented the boolean return. The pre-PR implementation already returned true for changes applied during Commit-unknown and recovery errorsI retained commit-unknown handling. If the atomic rename publishes defaults and the following directory sync fails, saying “recovery failed and the source was not replaced” would be false. Automatically replaying the caller's mutation would also be unsafe. The callback therefore reports the published-but-unconfirmed outcome before the typed error is thrown, and an interrupted The outcome has an actual consumer: the recovery notice chooses different wording for successful recovery and unconfirmed durability. Windows' directory-sync no-op limits where this particular failure can happen; it does not remove its significance on POSIX. The phase and backup-location diagnostics likewise distinguish a confirmed backup, an incomplete file that cleanup could not remove, and complete bytes retained after an unconfirmed directory fence. These are useful repair facts even when a UI displays the safe error message instead of branching on every field. Disposition of the remaining comments
Verification
AI assistance: OpenAI Codex implemented these changes, ran the checks and drafted this reply. Final submission and review remain with the human contributor. |
6851ab4 to
fdafc10
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at fdafc10a — every concrete finding from both prior rounds is addressed in the actual code path, and I verified the fixes rather than the claims.
Storage: BOM handling (UTF-8 stripped silently; UTF-16-BOM gets guidance instead of a reset), symlink refusal with an lstat + O_NOFOLLOW close on the check→open window, content-derived backup naming with full verified reuse (type/identity/0600/bytes/sync — still fsyncs on reuse), the completed backup retained on directory-sync failure, and the pre-rename revalidation honestly documented as a guard rather than CAS. The durability ordering holds: backup fsync+dirsync strictly precedes the single rename commit — write-ahead-then-commit is the right pattern here. Detection stays correctly narrow (SyntaxError only), secrets stay out of errors, commit-unknown is preserved, and the recovery tests are real per-phase fault injection (including a child-process exit(73) durability test), not fixture theater.
Desktop/CLI: single refresh authority restored (the reporter no longer touches effects), the queued application dialog with path-first banner + real click-to-focus wiring, the typed invalid-mcp-config-file IPC result replacing English-message parsing end to end, TUI error-state gating covering all mutation keys, and the "quit maka" copy in all three locales.
On the decisions you kept against review suggestions — all three hold up on inspection: copy-over-rename (a rename-then-reset failure leaves the path missing and indistinguishable from first run), the fingerprint split (the silent-nativeTheme.updated swallow race is real in base), and empty-list scrolling (the error-state gate independently prevents the harmful case). You were also right that the two earlier reviews sent mixed signals on these points — the second review's verification was the authoritative read, and your dispositions resolved the first review's asks correctly.
Only P3s remain: BOM-less UTF-16 still takes the reset path (narrower than the BOM-marked case you fixed), mcp-config-store still treats a UTF-8 BOM as corrupt (inconsistent with settings now, though not a regression vs base), Esc-during-busy can swallow a late invalid-config-file result in the TUI (self-heals), and schema-invalid mcp.json gets generic guidance rather than the repair path (the deliberate invalid-json+path boundary).
中文
fdafc10a 复审通过：两轮评审的全部具体发现均在真实代码路径修复——BOM 处理、symlink 拒绝（lstat+O_NOFOLLOW 闭环）、内容寻址备份+完整复用验证、dirsync 失败后保留完成备份、rename 前源文件重校验（诚实标注 guard 非 CAS）；持久化顺序正确（备份 fsync+dirsync 先于唯一 rename 提交点），检测仍正确收窄，测试是真·分阶段故障注入。Desktop/CLI 侧：刷新权威归一、队列化应用对话框+banner 路径前置+点击聚焦、typed IPC result 端到端替代英文解析、TUI 错误态键位门禁完整、三 locale "quit maka"。你保留的三处决定经审视全部成立（copy-over-rename、指纹拆分、空列表滚动）；前两轮评审口径不一的问题你判断得对——第二轮验证结论为准。仅剩 P3：无 BOM UTF-16 仍走重置、mcp-config-store 的 BOM 处理不一致（非回归）、Esc-during-busy 吞迟到的错误结果（自愈）、schema-invalid 走通用错误（刻意边界）。
Re-review generated with AI assistance (Devin); verified against head fdafc10a.
15b401a to
fdafc10
Compare
fdafc10 to
1832e50
Compare
|
Thanks for the re-review and for confirming the earlier fixes. I rebased the branch onto current
The rebase also carries the existing typed MCP error transport into Verification on the rebased working tree:
These are local macOS checks; no Windows or Linux application smoke run was performed for this follow-up. AI assistance: OpenAI Codex implemented the changes, ran the checks and drafted this reply. Final submission and review remain with the human contributor. |
1832e50 to
b9cfe47
Compare
github-actions
Bot
added
effort/XXL
b9cfe47 to
6b9b304
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head's storage-recovery and MCP error paths. I found no substantiated P0–P3 issue in the inspected changes.
The settings store backs up invalid JSON bytes before resetting defaults, verifies that the source has not changed before publication, and distinguishes a pre-publication failure from uncertain durability after rename (packages/storage/src/settings-store.ts:167-299,457-508; packages/storage/src/atomic-file-write.ts:100-125). The desktop reporter retains a recovery notice until a usable window exists (apps/desktop/src/main/settings-recovery.ts:116-165). MCP invalid-JSON diagnostics cross IPC as a structured failure rather than a parser message (apps/desktop/src/main/mcp-ipc-main.ts:83-104). The changed recovery tests cover interruption, repeat attempts, symlinks, cleanup failures, and concurrent reads.
The current-head test, package, and windows_recovery checks passed and git diff --check was clean. The PR currently conflicts with main in docs/windows-test-inventory.md (git merge-tree failed), so it is not merge-ready. This was a static review of the key storage, desktop notification, MCP IPC, CLI presentation, and adjacent tests; I did not run local tests (Node 18/no installed dependencies), a real disk-fault run, or a packaged Desktop recovery. No database schema/migration file changed. Resolve the conflict and revalidate the resulting head before a human merge decision.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
6b9b304 to
f937160
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
I independently reviewed the current PR head f9371607af2d974c75a62f5e995037a9b5104fcd. I found no substantiated P0–P3 issue in the inspected recovery and MCP paths.
The PR backs up corrupt settings.json bytes before publishing defaults, rechecks the source before replacement, and distinguishes pre-commit failures from uncertain durability (packages/storage/src/settings-store.ts:169-221,243-303,457-508). The Desktop reporter retains recovery guidance until a window can present it (apps/desktop/src/main/settings-recovery.ts:110-165); MCP invalid-file errors cross IPC as typed results (apps/desktop/src/main/mcp-ipc-main.ts:83-99). The focused tests cover backup faults, symlink/source races, startup notice delivery, and the preload's actual Runtime Host identity seam.
This head rebases the eight-commit series onto newer main: git range-diff shows the functional patches unchanged, with the Windows inventory totals adjusted for the newer base. I checked the resulting 41-file PR diff and affected call paths rather than treating the old-head review as current. Current-head test, package, and windows_recovery checks pass; the PR diff check and a synthetic merge onto freshly fetched main are clean. I did not run tests locally (Node 18/no installed dependencies), reproduce real disk faults, or exercise packaged Desktop recovery. No database schema/migration file changed. This is not a merge approval; the recovery policy and user-facing behavior remain for maintainers to decide.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
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 apache#4285 Generated-by: OpenAI Codex
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 apache#4285 Generated-by: OpenAI Codex
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 apache#4285 Generated-by: OpenAI Codex
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 apache#4285 Generated-by: OpenAI Codex
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 apache#4285 Generated-by: OpenAI Codex
Handle the upstream runtime-host:awaitReady handshake and verify its target scope before exercising typed MCP config failures. Generated-by: OpenAI Codex
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
f937160 to
f1b26dc
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Re-review result
Exact head: f1b26dcc06614d44df25526b60ce4322a2d6e2c3
I found no substantiated P0-P3 issue in the inspected settings-recovery and MCP paths.
For invalid JSON only, SettingsStore preserves the exact source bytes in an exclusive private backup before publishing defaults, rechecks the source snapshot immediately before replacement, and keeps pre-publication failures distinct from an uncertain post-rename durability result (packages/storage/src/settings-store.ts:169-236,238-316). A concurrent repair is reread rather than overwritten. The Desktop reporter queues recovery guidance until a usable window acknowledges it, while native notification failures remain best-effort (apps/desktop/src/main/settings-recovery.ts:108-164). MCP accepts an optional UTF-8 BOM and carries invalid-file failures across IPC as a typed, path-only result without exposing parser text (packages/storage/src/mcp-config-store.ts:184-216; apps/desktop/src/main/mcp-ipc-main.ts:83-99). No database schema or migration changes are present.
The final commit only updates the MCP preload test's Runtime Host identity/scope mock for the current protocol. The recovery behavior remains unchanged. Validation on this exact head included a clean install and test build, Storage 1,528 passed / 11 skipped, Desktop 2,804/2,804, CLI 1,323 passed / 3 skipped, and 238/238 focused recovery/MCP/notification tests. One FIFO portability test failed only during the first heavily concurrent CLI run and passed in isolation on both this head and current main; the full isolated CLI rerun passed. Typecheck, lint, format, ASF headers, renderer architecture/build/staleness, Windows inventory, E2E budget, TUI copy, locale hygiene, and git diff --check passed. Hosted test, package, and windows_recovery are successful. A production probe preserved corrupt bytes and returned defaults on this head; exact base threw SyntaxError and created no backup.
Current main is 18827d99c5704e185398b94d9d4e1ca666d2219f. The merge tree is conflict-free, and a synthetic current-main merge completed build:test plus 238 focused recovery tests. The only main commit after the earlier synthetic validation changes unrelated interrupted-resume/UI files and has no path overlap with this PR.
Residual scope: I did not run native Windows/macOS recovery, actual power-loss or filesystem-durability fault injection, packaged Electron recovery, or real third-party OAuth.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed f1b26dcc06614d44df25526b60ce4322a2d6e2c3 (8 commits; ~880 production lines across 21 files, the rest tests). The change makes an invalid-JSON settings.json recoverable — the original bytes are backed up, defaults are republished behind a precondition, the user is told what happened — and gives MCP config the same JSON-failure treatment across IPC.
No P0–P3 findings. Three areas, in the order the dispatch asked.
1. Recovery ordering — the fences compose correctly
- Only a
SyntaxErrorfromparseSettingsenters recovery (packages/storage/src/settings-store.ts:189-196), and the parse error is deliberately never attached, because its message can quote stored secrets. - Before anything is replaced,
readSettingsSnapshotre-reads and byte-compares (:198-200): an external repair wins, and that path does one reread and never recurses (:218-232), so a still-corrupt file cannot loop or fall into the first-run ENOENT path. - The backup happens before the reset and is exclusive, private and content-addressed (
:236-296):open(…, 'wx', 0o600),chmod 0600off Windows,fsync, then a directory sync. A pre-existing candidate is reused only afterverifyAndSyncBackupre-checks size,nlink === 1, mode and owner, and byte equality (:347-372). - The reset publishes through
writeAtomicFile's newbeforePublishhook, which re-checks the identical source (stat and bytes) immediately beforerename(settings-store.ts:201-208,atomic-file-write.ts:114) — so an edit made between backup and publish still wins. The hook is documented as "not an atomic compare-and-swap … a rejection leaves the target untouched and removes the temp", which is the honest boundary; two concurrent recoveries would both publish the same defaults. commit-unknownis never replayed: it is reported ascommit-unknownand rethrown asSettingsRecoveryCommitUnknownError, which tells the caller to reload rather than retry (:96-108,:209-212).reportRecoveryfires only after publication and is wrapped so a failing notification cannot change the write result (:298-310).
2. Cross-platform paths and permissions
- Backups are created
0o600and re-chmoded off Windows (:262-274); on Windows that is skipped, andsettings.jsonkeeps its historical umask-derived mode — the store says explicitly that it does not own the directory's permission policy (:402-410). readSettingsSnapshotrefuses anything that is not a regular file, opens withO_NOFOLLOW | O_NONBLOCKoff Windows, and re-checkslstat, open-stat, read-stat andlstatagain with a byte-length cross-check (:333-357). Symlinks and FIFOs therefore cannot be opened, and a concurrent swap invalidates the snapshot.verifyAndSyncBackupadditionally requiresnlink === 1and the current euid as owner (:352-356).- Windows specifics I could only reason about:
O_NOFOLLOWis absent (handled by the win32 branch) andino/ctimeNsare weaker discriminators there. Every fence that decides whether a file may be replaced is backed by a byte comparison in the caller, so a timestamp or inode collision cannot cause a changed file to be overwritten.
3. MCP config failures crossing IPC carry no parser text
- The store drops the
SyntaxErrorand throwsMcpConfigSourceError('invalid-json', …, path)with a location and guidance, on the stated ground thatJSON.parsecan quote credentials in its message (packages/storage/src/mcp-config-store.ts:285-297).parseMcpJsonalso accepts a leading UTF-8 BOM at the document boundary (:184-187). - Main turns exactly that case into data —
{ kind: 'invalid-mcp-config-file', path }with control characters stripped (apps/desktop/src/main/mcp-ipc-main.ts:84-98) — for the stated reason that Electron IPC and the context bridge do not preserve custom Error properties, while every other error still rethrows. - The renderer unwraps that data into a local error and maps it to localized copy, walking the cause chain with a cycle guard (
packages/.../model/mcp-page-model.ts:30-52); the CLI renders a scrollableconfig_errorphase holding only the path; and the recovery reporter's log contract is explicit that it "Receives only the result and paths, never JSON contents or parser errors" (apps/desktop/src/main/settings-recovery.ts:127).
The user-facing path is not fire-and-forget either: a native banner is attempted immediately, and the persistent dialog is held pending until a usable window exists (early-window.ts — showNotice returns false while the window is missing, hidden or minimized), with the backup path placed first in the banner body because native banners truncate (settings-recovery.ts:99-110). A UTF-16 settings file is deliberately not auto-recovered: it fails with guidance to save it as UTF-8 and leaves the file untouched.
Checks on this revision: test, package and windows_recovery are all green.
What I could not judge
- No native Windows or macOS run, no real power-loss or fsync fault injection, no packaged Electron, and no live third-party OAuth.
- The Windows
ino/ctimeNsreasoning above is read from the code, not observed on Windows; the byte comparisons are what make that acceptable, and I did not test a same-tick modification there.
I did not approve, request changes, or merge.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Third independent review lineage at f1b26dcc (supplements the two earlier reviews on this head).
No P0–P2. Focused on data loss and false recovery:
- The original bytes are backed up durably before defaults are published: exclusive, private, content-addressed, with the file and directory fsynced.
- The source is re-read and compared byte for byte before publishing, and a commit-unknown outcome is never replayed or reported as success.
- Only a genuine JSON
SyntaxErrorenters recovery. Transient read errors, UTF-16 files, and valid-but-unexpected JSON are not reset. - The tests would fail on the old code.
Two non-blocking P3s (inline):
- The pre-publish re-check is not an atomic compare-and-swap. An in-place external fix landing between
beforePublishandrenamewould still be replaced by defaults. The window is tiny, and the backup still holds the corrupt original. - The recovery notice is held only in memory. If the app exits before any window can show it, the user is never told where the backup went. Persisting a marker, or re-deriving it from the backup directory on next launch, would close that gap.
Not verified: tests were not run locally; no native Windows/macOS or real power-loss run.
This review was produced with automated assistance (AI review agents) and checked by a maintainer-side reviewer before posting.
| await handle.close().catch(() => {}); | ||
| throw error; | ||
| } | ||
| await options.beforePublish?.(); |
There was a problem hiding this comment.
P3 — beforePublish followed by rename is check-then-act, not an atomic compare-and-swap. An external in-place fix to settings.json that lands after the re-check but before the rename is overwritten by defaults. The window is microseconds and the backup keeps the corrupt original, so this is not blocking. Worth a comment noting that the fence narrows the race but does not close it.
| /** 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) { | ||
| const pending: CorruptSettingsRecovery[] = []; |
There was a problem hiding this comment.
P3 — pending lives only in memory. If the app quits (or crashes) after recovery but before a usable window acknowledges the notice, the next launch reads the now-valid defaults and never tells the user their settings were reset or where the backup is. Consider persisting a small marker next to the backup, or re-deriving the notice from unacknowledged backups on startup.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: independent automated reviews of this head from three model families found no blocking (P0–P2) issues, and CI (test, package, windows_recovery) is green. The two P3 notes are non-blocking follow-ups.
3 participants
Summary
Fixes #4285
#4505 hardened future JSON-store writes, but a settings file already emptied or truncated by an older build can still prevent startup. This PR completes the read-side recovery and provides repair guidance for malformed MCP configuration.
SyntaxErrortriggers recovery; read, normalization and migration failures retain their own error behavior. Published-but-unconfirmed writes remain commit-unknown and never automatically replay the caller's mutation.mcp.jsonand refuse writes, carrying its safe repair path through typed IPC results. Desktop and TUI show localized guidance; TUI error details scroll, initialization errors disable management keys, and runtime errors preserve the live server list.Recovery policy
This PR proposes automatic settings backup/reset because settings are a startup dependency. It resets preferences, bot configuration and onboarding, including Incognito's default of off; the application notice explicitly calls out reviewing privacy preferences. MCP definitions control commands and endpoints, so they receive repair guidance without automatic replacement. This policy still needs maintainer confirmation; silence on the issue is not treated as approval.
The final source check is not an atomic compare-and-swap against arbitrary external writers. Different corrupt snapshots are retained; backup reuse bounds repeated copies of identical content rather than pruning recovery data.
Verification
build,lint,format:check,typecheck, Desktop/UIknip, locale/TUI checks, Windows inventory, and ASF headers on an exported PR-source snapshot.Same truncated settings fixture against the PR base and this revision:
One existing MCP validation fixture now spells its raw control character as
\u0001; the runtime test input is unchanged.AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex implemented the recovery, regression tests and review fixes, ran verification, and drafted this description. Retain
Generated-by: OpenAI Codexin all substantive AI-authored commits and the final squash commit.Checklist
Does this PR entail a change in behavior?