Repository navigation
fix(setup): a failed setup-state write is restored and reported (#1250 rows 6, 13) - #1403
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rows 6, 13) saveSetupState assigned its cache before the write and never restored it, so a failed write left main acting on an unsaved value until a restart reverted it. The provider enablement and CLI update behaviour rows swallowed the rejection (or showed raw IPC text); they now say, in fixed words, that nothing changed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… changes (#1403 review round 1) Saves are update functions applied at write time to the last state known on disk, so a failed save can neither linger in memory nor ride along with a later write. Provider toggles update one key; a refresh failure after a landed write no longer claims nothing changed; a rejected detection probe no longer sticks in flight. SetupGate's write failures are said in fixed words, the panel answer after the close. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review disposition, round 1 (head
|
| Finding | Disposition |
|---|---|
| a1 / c1: two queued saves both fail, and the cache is left at the first one's unwritten value | Fixed by redesign. Saves are update functions applied at write time to the last state known on disk (updateSetupState). The cache is that state plus pending updates, so a failed update drops out. Test: keeps the durable state when two queued saves both fail. |
| a2 / b2: a failed save is carried to disk by the next queued save while the row says "Nothing was changed" | Fixed by the same design, which withdraws the plan's cache === snapshot ruling. Tests: never lets a later save carry a failed one to disk (the reviewers' timing) and does not persist a failed provider toggle with a later one. Provider toggles now update one key instead of a whole map built off the optimistic cache. |
| a3 / b1: a reset persists, re-detection fails, and the row says nothing changed | Fixed. mutate rejects only when the write fails; a refresh failure after a landed write resolves against the last good detection. Found on the way: a rejected probe stayed in flight for good; it is now cleared in finally. Test: providerEnablement.test.ts (new), red on 6707e7cf both with the whole file reverted and with only the finally reverted. |
| b3: SetupGate shows raw IPC text or hides write failures | Fixed (same file, same write path). A manual path shows the fixed sentence. A skip or acknowledgment failure is said as a toast after the panel's #1047 close. A check failure shows fixed text, with the raw error in the console. Tests in firstRun.renderer.test.tsx, both red on the old SetupGate. |
b4 / c2: test gaps (reset alert masked, clearing unpinned, cache === snapshot guard unpinned) |
Fixed. Reset is tested on its own render; clearing after a later success is pinned in all three rows. b's four surviving mutations are each killed now. The guard no longer exists; its case, an earlier good save kept when a later one fails, has its own test. |
| c3: the body's test count | Fixed in the body. |
Verification:
rm -rf .tsc-out; npx tsc -b: clean.NODE_ENV=test npx vitest run src/main/setup src/renderer/src/features/{setup,cli-updates,providers,settings}: 104/104 before the tsc-only fix;src/main/setup17/17 after it.
🤖 Generated with Claude Code
… fixed install text (#1403 verification) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review disposition, verification pass (head
|
| Finding | Disposition |
|---|---|
| b1: setup answers (skip, acknowledgment, manual path) were saved, then reported unsaved when the check's tool-path write-back failed | Fixed at the root. checkPrerequisites persists its probe as a best-effort cache (warned), and returns the probe's answer either way. This also covers Install and a provider reset. New src/main/ipc/setup.test.ts drives the real handlers, setup state and check: each answer resolves with the answer on disk, and a failing answer write still rejects. It fails with the old prerequisites.ts. |
| b2: Install rendered the raw rejection | Fixed: "Could not install ." The test fails on the old SetupGate. |
| a1: an older provider refresh could overwrite and broadcast a newer one | Fixed: refreshes are numbered, and a stale one answers with the newer snapshot. The test holds the first row's credential probe; the cache and the last broadcast keep Claude off. It fails without the ordering. |
| a/b/c survivors: the shared first read, and the last-good-detection fallback | Pinned. A held first read finishing after a save no longer resets the durable baseline (fails with loading =). A Claude-only detection plus a failed reset probe shows Codex not installed (fails with new Set(ALL) and with cachedDetected). |
| c (suspicion): a throwing update stays pending | Closed: the update leaves pending on every path. Test added. |
Verification:
rm -rf .tsc-out; npx tsc -b: clean.src/main/setup,src/main/ipc/setup.test.tsand the setup, cli-updates, providers and settings renderer features: 112/112 before the last fix;src/main/setupplus the IPC test: 25/25 after it.
🤖 Generated with Claude Code
…uses unsaved probed paths (#1403 recheck) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review disposition, recheck (head
|
| Finding | Disposition |
|---|---|
b, a2: a provider snapshot published a toggle whose write then failed. The refresh read the optimistic cache while another toggle was in flight, whether from a second toggle's refresh or an overlapping get. |
Fixed: published snapshots are built from the durable state (loadDurableSetupState). Each refresh runs after its own write lands. Two real-write tests (a successful toggle beside a failing one, and an overlapping read) both fail with the optimistic read. |
| a1: a failed write-back left the toolchain on the persisted, possibly dead, path while the check said "found at X" | Fixed: refreshToolchainFromState(unsaved) applies the probed paths in memory when they could not be persisted. New prerequisites.test.ts (real check, toolchain and setup state) fails without it. |
a (survivor): the refreshToolchainFromState call was unpinned |
Killed by both prerequisites.test.ts cases. |
Verification:
npx tsc -b: clean.- Focused suites: 116/117. The one failure is
ProviderEnablementRow"renders one row per provider kind", which is unchanged from main. It times out on the first in-test dynamic import at machine load 40. Alone, under the same load, the first two tests time out at about 5 s and the rest pass in milliseconds. Reviewers a and c reported the same load behaviour in earlier passes. CI decides.
Rounds: this is the recheck after verification. A final focused check by a and b follows.
🤖 Generated with Claude Code
|
Final check (a, b) at Per q104 the review loop is closed here: round 1, verification, recheck and final check; no further rounds. The PR waits for exact-head CI and the manager gate. 🤖 Generated with Claude Code |
Refs #1250 (rows 6 and 13). #1250 is a batch issue, so the other rows stay open.
Problem
A failed setup-state write (a full disk, or a read-only or permission-broken state directory) was invisible:
saveSetupStateset the in-memory cache BEFORE the write and never restored it on rejection. Main then behaved as if the change were saved, and silently reverted on the next launch.catch(called viavoid), so an unhandled rejection with nothing shown. The OpenCode usage-source row showed the raw IPCerror.message, which can carry a filesystem path (q22).setCliUpdateBehaviorhad no rejection handler, andCliUpdateBehaviorRowhad nowhere to say it failed.Change
setupState.ts): each save is an update function applied at write time to the last state known on disk (updateSetupState).providerEnablement.mutate: rejects only when the write fails. A refresh failure after a landed write resolves against the last good detection. A rejected detection probe no longer sticks in flight. Refreshes are ordered, so an older one never overwrites a newer one. Published snapshots are built from the durable state, so a toggle still in flight, or one that failed, is never broadcast.checkPrerequisites: its tool-path write-back is best effort. It is a cache of the probe, and its failure no longer makes a saved answer (skip, acknowledgment, manual path, Install, reset) reject. When the write-back fails, the toolchain applies the probed paths in memory, so "found at X" is also what a launch uses.role="alert"). No IPC or filesystem text reaches the UI.setCliUpdateBehaviorreturns its promise.Plan:
docs/plans/2026-09-27-setup-state-write-failures.md.Tests (all red on main or on the round-0 head
6707e7cf)setupState.test.ts(new, real filesystem):setup.json, and a one-shot rename fault, make writes fail;providerEnablement.test.ts(new): a reset whose re-probe fails after the write, the stuck in-flight probe, the last-good-detection fallback, refresh ordering, and no publication of a failing toggle (through another toggle's refresh or an overlapping read).prerequisites.test.ts(new): the real check and toolchain; a launch uses what the check found whether or not the write-back lands.src/main/ipc/setup.test.ts(new): the real setup handlers and check. A saved answer resolves when only the check's write-back fails; a failing answer write rejects.setupState.test.ts: also the shared first read, and a throwing update.firstRun.renderer.test.tsx: the skip-failure toast, and the manual-path and Install failure text.Verification
NODE_ENV=test npx vitest run src/main/setup src/main/ipc/setup.test.ts src/renderer/src/features/{setup,cli-updates,providers,settings}: 116/117 at machine load 40. The failure isProviderEnablementRow's first test, unchanged from main, timing out on its in-test cold import under load; reviewers saw the same. It was 112/112 on the previous head at normal load.rm -rf .tsc-out; npx tsc -b: clean.🤖 Generated with Claude Code