fix(settings): preserve provider state across async completions

[ewen-poch]

Problem

Late provider operations can contradict a completed user action: validation can restore an old key or recreate a deleted provider; xAI login can persist credentials after cancellation; normal token rotation can discard a model catalog while reporting success; and post-save preflight failure can make a committed creation appear retryable.

Renderer reconciliation also needs to preserve the latest readiness outcome and reflect authoritative provider edits/deletions after a discarded catalog refresh.

Proposed change

• Apply validation fields to the current provider only after checking existence, credentials, effective target, framework and operation generation inside the serialized storage mutation.
• Bind xAI login persistence and caching to the current login operation. Cancel and bound optional account-info retrieval; condition credential updates inside storage.
• Carry the access token's committed credential reference into model refresh. Report success only when the catalog is applied, reconcile the renderer on unsuccessful results too, and preserve the original refresh failure if that reconciliation fails.
• Return the committed provider ID before the derived preflight refresh. Reuse the translated retry notice across startup, refresh and environment-check outcomes.
• Share one renderer-only preflight request generation across those entry points so older successes/failures cannot replace newer readiness or its failure flag. Retain the existing startup-load and environment/framework guards.
• Make the Notebook queued-shell cancellation test wait for the existing process-start signal and drain pending work on assertion failure; no Notebook runtime code changes.

Scope and non-goals

No changes to the settings file format, persistent data model, storage queue or public IPC contracts. No dependencies or migrations. Additions are conditional repository operations, an internal credential-bound token result, transient preflight UI state and an in-memory request generation. There are no new interaction steps beyond the independent preflight retry already described above.

Acceptance criteria and validation

Regression assertions were exercised against the unfixed behavior before implementation. File-backed provider tests use the existing public mutation boundary with mocked HTTP and system credential encryption; renderer tests use the public store actions and DOM. No production test seam was added.

Checks after the final material edits on 9579c4a:

Behavior / contract	Check	Result
Startup/environment failure tracking; discarded catalog reconciliation; out-of-order preflight successes/failures	npm test -- src/renderer/src/stores/settings-store.test.ts src/renderer/src/stores/settings-runtime-slice.test.ts src/renderer/src/stores/settings-provider-auth-slice.test.ts src/renderer/src/stores/settings-store.architecture.test.ts src/renderer/src/pages/settings/SettingsPage.render.test.tsx	315 passed; includes nine preflight race cases that failed before the fix
Renderer interfaces	npm run typecheck:web	Passed
Modified renderer files and architecture inventory	Changed-file ESLint, Prettier and git diff --check	Passed

Earlier validation of the unchanged provider/Notebook implementation: the full local suite passed 25,029 tests (226 skipped), and the rebase onto main at 5cdde8fa passed 1,440 selected provider, Notebook, renderer, architecture and i18n tests. Both sides' locale keys/translations were retained. CI run 34004550620 passed all complete-test shards, coverage, static checks and platform lanes on the earlier 699ea561 head; those results are historical, not current-head CI evidence.

The final follow-up changes are renderer-only, so their affected checks cover the store owners, facade contract and SettingsPage consumer. Live xAI/model services and a full Electron window were not exercised locally. Current-head platform and full-suite checks remain with CI.

Review focus

Verify transactional conditional writes, credential identity through rotation, independent committed-save/readiness outcomes, and shared preflight fencing without discarding unrelated environment information. The architecture inventory records the new startup-local request guard; no architecture rule is relaxed.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Fence background preflight refreshes against stale completions

src/renderer/src/stores/settings-provider-auth-slice.ts:130

Impact: Saving a provider starts an unawaited refresh, and post-save validation starts another. If the older request finishes last, it can overwrite the newer readiness result and incorrectly show the active provider or runtime as unready.

Recommendation: Serialize/coalesce preflight refreshes or add a request generation so only the latest completion updates preflight and preflightFailed.

[P2] Startup preflight failures never activate the retry banner

src/renderer/src/stores/settings-runtime-slice.ts:319

Impact: The startup load path calls getPreflight directly and preserves the initial preflight when that call rejects, but never sets preflightFailed. Settings therefore opens with stale false readiness and no Retry preflight control after a startup probe failure.

Recommendation: Set preflightFailed from the startup preflight result, or route startup hydration through the same failure-tracking state transition as refreshPreflight.

Summary: Static inspection only; no tests or project commands were run. Branch and title checks are valid.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Prevent stale preflight failures from overwriting newer results

src/renderer/src/stores/settings-provider-auth-slice.ts:130

Impact: Saving a provider starts a background preflight, then validation can start another one via line 168. These requests are independent; if the newer request succeeds before the older one fails, the older failure sets preflightFailed and leaves a misleading retry banner visible.

Recommendation: Deduplicate or serialize preflight refreshes, or associate each request with a generation and only apply the latest completion’s success/failure state.

Summary: Static inspection found one user-visible race in the new background preflight refresh flow. Branch and PR title checks are valid.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Track preflight failures during normal settings loading

src/renderer/src/stores/settings-runtime-slice.ts:319

Impact: preflightFailed is updated only by refreshPreflight, while startup loading and environment checks call getPreflight directly. Initial preflight failures therefore never show the new retry alert, and successful loads do not clear a previous failure flag.

Recommendation: Update all preflight result paths, including createRuntimeSetupLoadPatch and checkEnvironment, to set preflightFailed consistently, or centralize those calls through the state-owning refresh action.

[P2] Reconcile settings when a model refresh is discarded

src/renderer/src/stores/settings-provider-auth-slice.ts:259

Impact: The main-process change now returns ok: false when a catalog completion loses a provider-target race, but the renderer only fetches the authoritative settings snapshot when result.ok is true. After an edit or deletion from another window, the settings UI can retain the obsolete provider/catalog state while displaying the race error.

Recommendation: Reconcile getSettings() on discarded refreshes as well, or have the IPC result include the authoritative snapshot and apply it before returning the error.

Summary: Static inspection found two actionable integration regressions. Branch and pull-request title prechecks are valid.

[ewen-poch]

Both findings reproduce through the existing renderer-store behavior boundaries on this head:

• Preflight failure tracking: an initial failed probe leaves preflightFailed false; a successful forced load or environment check leaves an existing failure flag true. Three regression assertions fail with those exact outcomes.
• Discarded model refresh: after a simulated concurrent provider edit or deletion, the action returns the stale-target error while the renderer retains the old provider/catalog. Both edit and deletion regression cases fail against the current implementation.

The proposed changes are to reconcile the existing preflight failure flag at the startup/environment result paths, and fetch the authoritative settings snapshot when a model refresh is discarded while preserving the unsuccessful result. These would reuse the existing retry banner and settings reconciliation; no persistent fields, IPC schema changes, or new interaction steps are planned.

Startup handling also requires editing settings-store.ts, which is outside the current PR file set. Production changes are paused pending maintainer approval of that scope expansion. The reproduction tests are local and uncommitted; no fix has been pushed yet.

[ewen-poch]

Addressed in 4bc855d:

• Preflight failure tracking: startup hydration now derives the existing failure flag from the actual probe outcome. Environment checks update that flag on probe success/failure while retaining the existing generation/framework fence. A settings-read or npm-check error is not mislabeled as a failed preflight.
• Discarded model refresh: both successful and unsuccessful refresh results now reconcile the authoritative settings snapshot. A concurrent edit/deletion is reflected in the renderer, and a failed reconciliation does not replace the original unsuccessful refresh result.

Six regression assertions first failed on the unfixed code with the reported stale flags and stale provider/catalog values. They now pass. All 306 tests across the store, provider-auth slice, runtime slice, store architecture and SettingsPage DOM suites pass, as do Web typecheck, changed-file ESLint and formatting checks. The startup file scope expansion was approved by the maintainer. No new state fields, persisted data, IPC schemas or interaction steps were added.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Fence concurrent preflight refresh results

src/renderer/src/stores/settings-runtime-slice.ts:319

Impact: Overlapping getPreflight calls can resolve out of order, causing an older success or failure to overwrite the latest preflight and preflightFailed state. Users may see a retry error after readiness succeeded, or have a real failure hidden.

Recommendation: Track a monotonically increasing preflight request generation or share one in-flight request, and only let the current request update preflight/preflightFailed. Apply the same fencing to preflight outcomes recorded by checkEnvironment.

Summary: The renderer can expose stale preflight readiness after overlapping refreshes introduced by the background save/validation flow.

[ewen-poch]

The concurrent-preflight finding is reproducible on 4bc855d. Four deterministic store tests hold an older refresh/environment preflight pending, complete a newer successful refresh, and then release the older success or failure. All four fail because the older completion overwrites either readiness or the retry flag.

A shared in-memory preflight request generation is the proposed fix: only the latest request may publish readiness or its failure flag, consistently across background refresh, startup hydration and environment checks, while preserving their existing load/framework guards. Sharing an older in-flight result after settings change would not provide the same guarantee.

This would add transient coordination state, with no persisted fields, data-model or IPC changes, or new interaction steps. The maintainer explicitly requires approval before adding state, so production changes are paused for that decision. The four reproduction tests remain local and uncommitted.

[ewen-poch]

The approved preflight-generation change is implemented locally. Background refresh, startup hydration and environment checks now share a renderer-only request identity. Older results cannot replace newer readiness or its failure flag; existing startup-load and environment/framework guards remain in place. Superseded environment checks cannot invalidate a newer preflight request.

Nine deterministic regression cases failed before the change and now pass, covering late successes/failures from all three entry points and an older success hiding a newer failure. The focused validation currently has 314 passing tests; Web typecheck, changed-file ESLint and formatting pass.

One architecture inventory check needs a matching declaration entry for the new startup-local isCurrentPreflight binding. That is a one-line update in settings-store.architecture.test.ts, not a change to the architecture policy. Because this test file is outside the approved PR file set, the update and push are paused for maintainer scope approval. No check has been disabled or relaxed.

[ewen-poch]

Addressed the concurrent-preflight finding in 9579c4a.

• Startup hydration, background refresh and environment checks now share a renderer-only preflight request generation. Only the current request may publish readiness or its failure flag.
• Existing settings-load and environment/framework guards remain intact. An already superseded environment check does not invalidate a newer preflight request, and unrelated environment information can still reconcile when its preflight is superseded.
• No persisted fields, IPC schemas or interaction steps changed. The transient request identity and the one-line architecture inventory update were explicitly approved by the maintainer.

All nine deterministic race regressions failed against the unfixed code and now pass. The final focused run passed all 315 tests across the settings store, runtime/provider-auth slices, architecture checks and SettingsPage DOM tests. Web typecheck, changed-file ESLint, formatting and patch-whitespace checks also passed. No architecture rule was disabled or weakened.

[github-actions]

Codex Review

Verdict: mergeable

No actionable findings.

Summary: Static inspection found no concrete merge-blocking defects. Branch and pull-request title prechecks are valid.

[ewen-poch]

The remaining failure on 9579c4a is the Windows workspace E2E case keeps bottom-follow from overshooting when a paced tool turn completes in e2e/message-tool-layout-stability.spec.ts. The PR Gate failure is downstream of that job; the other checks passed.

Both the initial attempt and automatic retry report the same geometry: first top 369.34375, minimum top 325.34375, final top 345.34375. The measured upward overshoot is 20 CSS pixels against the 2-pixel limit. I inspected the failure logs, screenshots and available trace; these establish the failed layout measurement but do not yet isolate the production cause versus sampling behavior.

The relevant E2E and message-scrolling implementation are outside this PR's approved file scope. Further code changes are paused for maintainer approval to investigate and fix that area. The current branch remains unchanged.

[ewen-poch]

The Windows layout failure came from an expected loading-to-completion transition. Frame-level measurements reproduced the exact CI positions (369.34375 → 325.34375 → 345.34375): final text appeared while the 60px Thinking row remained, then the run completed and replaced it with a 40px footer. The old assertion interpreted that 20px transcript shrink as scroll overshoot.

The regression now excludes the measured transient loading-row height while retaining the 2px tolerance. It exercises both normal and reduced motion at the failing Windows viewport size. A short delay in the existing fake-agent journey makes the final-text/live-loading overlap observable across hosts. Production behavior is unchanged.

Validation: the original assertion failed three consecutive times with the exact 20px CI result; the corrected layout and tool-order journeys passed (6 tests), and both motion variants passed three consecutive runs. Temporarily removing the existing completion-row containment fix still made both variants fail with a 120px overshoot, confirming that the regression continues to catch the original bug. The Electron fixture unit tests (9), changed-file ESLint, Prettier, and diff checks passed.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Do not let a late environment check supersede a newer preflight request

src/renderer/src/stores/settings-runtime-slice.ts:366

Impact: If a user starts refreshPreflight while an environment probe is running, the older environment operation increments the generation after the probe completes and can overwrite the newer retry’s readiness or error state.

Recommendation: Reserve the environment check’s preflight identity when it starts, and apply its result only while that identity remains current.

Summary: Static inspection found one renderer preflight concurrency defect: an older environment check can supersede a newer manual preflight request.

[ewen-poch]

Reserved the environment operation's existing preflight identity before awaiting the environment probe. A manual retry started during that probe now remains newer, regardless of which response finishes first. The existing environment generation/framework checks still fence environment results; this adds no state, persistence, or interaction changes.

Added four deferred-response regressions covering successful/failed environment preflights and both retry completion orders. All four failed before the fix by reproducing overwritten readiness or error state; all now pass. The runtime, settings-store, store architecture, and provider-auth suites pass (211 tests), as do renderer typechecking and changed-file lint/format checks.

[github-actions]

Codex Review

Verdict: mergeable

No actionable findings.

Summary: Static inspection found no concrete merge-blocking defects in the requested changes.

[ewen-poch]

Windows E2E is now passing on the current head. Both normal-motion and reduced-motion layout regressions passed. A separate renderer startup timeout occurred before one test body began and passed on its automatic retry; rerunning that Windows shard independently passed without another timeout. The final AI review verdict is mergeable with no actionable findings.

[ewen-poch]

Raised the combined macOS build/E2E job limit from 20 to 30 minutes. GitHub explicitly reported that the previous job exceeded its 20-minute execution limit: the functional journeys (20 tests) and workspace journeys (32 tests) had passed, but accessibility was cancelled and visual checks never started. The additional budget lets the remaining selected checks finish without changing their assertions or application behavior.

Updated the existing timeout guard. The guard rejects the old 20-minute configuration; all 49 workflow/integrity/gate tests now pass, along with changed-file formatting and lint checks. The current CI run will verify the combined macOS path with the new limit.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Race tests deadlock while the first document mutation is paused

src/main/settings/provider-completion-races.test.ts:111

Impact: pauseCommit() blocks the validation mutation until commit.release() is called, but the test awaits accounts.upsertProvider() (and similarly deleteProvider() at line 113) before releasing it. Those operations enqueue behind the blocked mutation, so the parameterized tests never reach commit.release() and hang.

Recommendation: Start the edit/delete operation without awaiting it, release the paused validation commit, then await the edit/delete promise before asserting the stale validation result.

Summary: Static review found one blocking issue in the newly added race tests. No project commands or tests were run.

[ewen-poch]

CI Integrity reports protected-gate-control-plane because .github/workflows/pr-gate.yml is an established protected control-plane file. The trusted-base checker requires an explicit maintainer ruleset bypass for any change to this file, including this timeout-only change. The guard remains intact; merging this workflow change therefore requires that maintainer decision. The macOS job is still running to validate the new execution budget.

[ewen-poch]

Validation on e2163e5 is complete: the macOS build/E2E job passed, including both accessibility and visual regression. Both Windows E2E shards, portable tests, static checks, and PR Gate also passed. CI Integrity remains the sole failing check because of the protected pr-gate.yml change; the explicit maintainer ruleset-bypass requirement described above still applies.

[ewen-poch]

The reported deadlock does not occur. pauseCommit() uses mockImplementationOnce to wait before calling the bound real document.mutate(update). The paused validation has therefore not entered mutationTail. The subsequent edit/delete uses the original implementation, enters the queue, and completes before commit.release() allows validation to enter it.

This ordering is intentional: the regression verifies that validation cannot overwrite an edit/delete that has already completed. Releasing validation before awaiting the competing operation would change that scenario.

Re-ran npm test -- src/main/settings/provider-completion-races.test.ts on e2163e5: all 8 tests passed (5.37s test execution), including both reported edit/delete cases. No code change is needed for this finding.

[ewen-poch]

Rebased onto current main (e1c7e580) and resolved the layout-test conflicts by keeping main's newer implementation: matching loading/completion-row geometry, deterministic sampling gates, and coverage of both motion settings. The obsolete loading-height compensation commit was dropped. All provider race fixes were preserved unchanged during the rebase.

Restored pr-gate.yml and its timeout assertion to match main exactly, removing the temporary 30-minute override. The current main change completed macOS CI in about 15½ minutes under its 20-minute limit. CI Integrity now passes locally with no CI-sensitive diff; no ruleset bypass or guard modification is needed.

Validation on c1cf00b1: 1,685 related tests and all 5 Electron layout tests passed, as did full typechecking, lint, formatting, and diff checks. The branch was pushed with an exact force-with-lease, and fresh remote CI is running.

[github-actions]

Codex Review

Verdict: mergeable

No actionable findings.

Summary: No concrete merge-blocking defects found in the reviewed changes.

[ewen-poch]

Included the additional main commit cd36c096 that landed during validation. The final head is fe94b9a0; range-diff confirms all seven remaining commits are unchanged by this second rebase. The additional TIFF and translation checks passed (812 tests), and CI Integrity still passes locally with no CI-sensitive diff. The earlier provider, typecheck, lint, and Electron layout validation remains applicable.

[github-actions]

Codex Review

Verdict: mergeable

No actionable findings.

Summary: No concrete merge-blocking defects found during static inspection. Tests and other executable checks were not run per instruction.