fix(config): validate the remembered export directory before saving settings - #2311

What

Fixes the settings-export save location: the remembered export path is validated before it is used as the save-dialog default, the location is remembered only after a successful write, and export failures/successes are reported to the user instead of only logged.

Closes #2310

Why

The user story from the issue: Settings → About → Export, the save dialog is aimed at a USB drive, the picker is confirmed — and the file lands in C:\\Users\\<user>\\Downloads.

resolveDefaultSaveUri handed lastSettingsExportPath straight to vscode.window.showSaveDialog without checking that the directory still exists, is still a directory, or is writable. When that remembered directory is gone (removable drive unmounted or remounted under another letter), the Windows dialog silently substitutes its own default folder — Downloads. Nothing on disk or on screen reports the substitution.

Two adjacent defects made it worse and invisible:

• saveLastExportPath ran before safeWriteJson, so a failed export pinned the next dialog too.
• the only failure reporting was console.error, so the user had no way to learn that the chosen location failed. Import already has toast feedback; export had none.

Behaviour change

How

• src/utils/export.ts: resolveDefaultSaveUri is async and gates the remembered path behind isUsableSaveDirectory (stat().isDirectory() + access(dir, constants.W_OK | constants.X_OK) — write permission and search permission, not just existence). Fallback priorities are unchanged, so no caller loses a location it can still use.

• src/core/config/importExport.ts: saveLastExportPath moved after the successful safeWriteJson; the catch now shows common:errors.settings_export_failed with the error message; a successful export shows common:info.settings_exported with uri.fsPath; importSettings awaits the resolver.

• src/i18n/locales/*/common.json: the three new keys in all 18 core locales, and src/i18n/__tests__/locale-bundles.spec.ts now pins all of them (and their {{path}} / {{error}} placeholders) in every locale, so check-translations cannot drift.

• src/core/config/importExport.ts (sanitizeGlobalSettings): the four host-local dialog memories — lastSettingsExportPath, lastTaskExportPath, lastModeExportPath, lastImageSavePath — are dropped on import. They are an absolute directory on the machine that produced the file, not configuration, and the save/open dialogs probe and default to whatever is in lastSettingsExportPath. After this, that key can only ever hold a path the user chose on this machine. The drop is silent: every export contains these keys, so warning on each import would report nothing the user can act on.

• src/core/config/ContextProxy.ts: updateGlobalState keeps stateCache a true mirror of globalState. It already wrote the cache before the underlying update settled, so a rejected write left a value in memory that disk never received — readers, export()/import() and the next restart all report a setting that does not exist. A rejection now rolls the key back to the newest value that actually persisted, and a write that lands after a newer write for the same key already failed reconciles the cache with what storage holds; the rejection still reaches the caller. Writes to one key are counted while they are in flight, so only the last write standing for a key decides what the cache holds. The optimistic cache write is kept (callers already read a value before its write settles); the update result is normalized first because Memento.update is only specified as a Thenable.

Tests

• New src/utils/__tests__/export.spec.ts (15 tests): remembered directory used; W_OK checked rather than existence alone; fallback on missing directory, missing drive, remembered path turned into a file, read-only directory; workspace priority; useWorkspace: false; empty workspace list; fallback directory; bare file name; read-only remembered directory kept when the caller only reads (accessMode: constants.R_OK); fallback when it cannot be read either; saveLastExportPath key/value.
• src/core/config/__tests__/importExport.spec.ts: fourteen new tests — five for the export dialog default and persistence (remember-after-success, stale directory, path-is-a-file, read-only directory, still-writable remembered directory), three for the import dialog default (still-writable remembered directory is used, readable-but-read-only remembered directory is kept, stale one falls back), three for failure reporting (provider profile export rejection, global settings export rejection, directory creation failure — each asserting the localized error carries the thrown reason, that no success toast is shown, and that the location is not remembered), one for the partial export report (file written, location not remembered: warning names both halves, no success toast, the failure is still logged), and one that imported settings cannot install another machine's dialog memories. The fourteenth pins the deliberate profile-less no-op: when providerSettingsManager.export() resolves to undefined nothing is written, nothing is toasted, and no location is remembered. Every remembered-default test asserts the dialog arguments (defaultUri.fsPath), not just the URI the resolver built, plus a test that a failed setValue still leaves the success confirmation in place. The existing "handle errors during the export process" test now asserts the error toast (key and interpolated reason) and that a failed location is not remembered. The fs/promises mock exposes shared instances for the default import and the named imports utils/export uses, plus stat and W_OK, and the i18n mock now echoes the interpolation arguments (same approach as webviewMessageHandler.openFile.spec.ts) so the messages that carry {{error}} / {{path}} are actually asserted. The getValue doubles are key-sensitive so a wrong setting key is caught.
• Negative controls (Windows, vitest.unit.config.ts):
  • reverting src/utils/export.ts only → 7 of the 13 new tests that existed at the time fail;
  • reverting src/core/config/importExport.ts only → the export-flow tests fail (error toast + persist-after-write);
  • with the fix: export.spec.ts 15/15, locale-bundles.spec.ts 36/36, importExport.spec.ts 66 passed / 1 failed (118 tests across the three files).
• Hand-written mutation sweep over the two changed files — 24 mutants (trust the remembered directory always/never, drop isDirectory(), W_OK → R_OK, drop the access call, forget dirname, flip useWorkspace, length > 0 → >= 0, drop the fallbackDir guard, treat a stat failure as usable, drop the remembered directory, wrong setting key at all three call sites, wrong Uri property, persist before the write, drop persistence, drop the error toast, drop {{error}}, drop the success toast, drop {{path}}, empty translation key, empty fallback directory name). All 23 are killed. Two of them initially survived (a wrong setting key, and dropping the interpolation object) and were killed by the key-sensitive getValue doubles and the echoing i18n mock rather than by a Stryker suppression. After each review commit the sweep was re-run over that commit's delta: 20 mutants and then 22 mutants, all killed (one variant that removed an await on a toast call survived — nothing observable changes, the promise is not consumed, and removing await is not a Stryker mutator). Over the ContextProxy lines: drop the rollback, roll back to undefined, do not capture the previous value, swallow the rejection, skip the cache write on success and drop the Thenable normalization are all killed (the last one breaks 46 tests that stub globalState.update as a plain object); a seventh variant that re-assigns the same value on success is equivalent and survives. For the profile-less no-op test: remove the guard, weaken it to a null check, or let it report success — each fails exactly that test and nothing else.
• Local Stryker could not run in this environment: src/node_modules is a junction to a scratch worktree, so the @stryker-mutator/core bin is not linked, and the documented Stryker-10 + vitest-runner activation defect needs the sandbox copy of vitest.config.ts; the dry run is also red from the pre-existing Windows failure below. mutation-diff will run on CI.
• The one remaining importExport.spec.ts failure — importSettings > round-trips a prompt cache TTL through export and import — is pre-existing on Windows: the same test fails at the base commit 3ee042d91 (53 passed / 1 failed). It is out of scope here and is tracked by [BUG] Six config import/export unit tests fail on Windows (green on ubuntu CI) #2059.
• pnpm exec prettier --check src is clean; none of the five touched files has an entry in src/eslint-suppressions.json, so no suppression count moved; core/config/ContextProxy.ts and its spec keep their recorded counts unchanged (no-explicit-any 1 and 5). Local ESLint initially failed for an environment reason (the @roo-code/config-eslint junction resolved to a scratch checkout whose @eslint/js and plugin links are dangling), which was repaired locally in node_modules only; eslint --max-warnings=0 core/config/importExport.ts core/config/__tests__/importExport.spec.ts utils/export.ts utils/__tests__/export.spec.ts i18n/__tests__/locale-bundles.spec.ts then exits 0.
• src/core/config/__tests__/ContextProxy.spec.ts (49 tests): a rejected update keeps the previously stored value; a rejected first update leaves the key unset; a batched same-key rejection restores the value read from storage; a superseded write failing late leaves the newer persisted value alone; an older failure cannot roll back a write still in flight (asserted while that write is still pending); a rejected newest write is dropped from the mirror while an older same-key write is still in flight and repaired when it lands; a pre-reset write that settles after resetAllState() is ignored, in both directions; and a write rejected after resetAllState() leaves the key unset.
• Whole-suite evidence (Windows, vitest.unit.config.ts): before this branch's ContextProxy change 58 failed / 11996 passed / 15 failed files; after the ContextProxy work 58 failed / 12010 passed / the same 15 files. Same pre-existing failures, no new ones.

Deliberately out of scope

• The four host-local dialog memories are still written into the exported globalSettings (globalSettingsExportSchema in packages/types/src/global-settings.ts is untouched), so an exported file still contains the paths of the machine that produced it. They are dropped on the way in, which is what stops them from steering another machine's dialogs. Whether they belong in the export at all is a maintainer decision, written up in the issue.
• exportSettings still returns silently when there are no provider profiles (importExport.ts explains why: importSettings cannot consume a profile-less export).
• No .changeset and no CHANGELOG edit, per AGENTS.md.

Review follow-up (4888f603a, 0ed9e0278, be8c3739b, bcd082270, 653661763, 36d0b6059, faa9c0b79, 9b089a217, 8a491fc53, 35a8bf339, ec8850bc6, 4b3d007a1, e265d53c4, c61e97b2c, d241d22e3)

• Read vs write for the open dialog. resolveDefaultSaveUri gained an accessMode option (an fs/promises access constant, default W_OK); importSettings passes R_OK. An open dialog only reads from the remembered folder, so a read-only remembered folder that actually holds the settings file stays the default instead of falling back to Downloads. The export default still requires write access.
• Confirmation before remembering. exportSettings shows common:info.settings_exported first and only then remembers the location, with the remember step wrapped so a setValue failure is logged instead of rejecting out of exportSettings into the unawaited onReceiveMessage listener, where the user would have got no confirmation at all for a file that was written.
• Stronger assertions. The remembered-default tests assert the showSaveDialog / showOpenDialog arguments (defaultUri.fsPath) instead of only vscode.Uri.file calls, which the resolver performs whether or not the dialog is opened there.
• Traversal counts, not just the bit asked for. isUsableSaveDirectory now checks accessMode | constants.X_OK: R_OK/W_OK alone do not test search permission on POSIX, and a directory that cannot be searched cannot resolve a file inside it. Node ignores X_OK on Windows, so Windows behaviour is unchanged.
• Remember before the toast, not after it. showInformationMessage only resolves once the notification is dismissed, so the previous ordering delayed the remembered folder until the next export was already open (or lost it if the window closed first). The guarded saveLastExportPath now runs first; a test asserts the invocation order.
• Regression Evidence (pre-merge check warning, closed by 36d0b6059). The gap was exportSettings returning silently when there are no provider profiles. The behaviour is unchanged — importSettings cannot consume a profile-less file — and is now pinned by a test. The check asked for contextProxy.export to be asserted as not called; the guard sits after both collect calls (src/core/config/importExport.ts:355-364), so the test asserts it is called and that the write and the reporting are not. A second correction, for the record: ProviderSettingsManager.export() (src/core/config/ProviderSettingsManager.ts:577-637) either returns schema-parsed profiles or throws, so the typeof providerProfiles === "undefined" guard is defensive, not reachable through real code today. The test pins the defensive branch (mock loosened with one as Mock assertion, as this file already does for the dialog mocks); removing the guard as dead code is a maintainer call, recorded in Settings export silently lands in the Downloads folder when the remembered export location is gone #2310.
• compile was red on 36d0b6059, fixed by faa9c0b79. That was my own doing: mockResolvedValue(undefined) against the typed vi.mocked<ProviderSettingsManager> mock (importExport.spec.ts(1927,57): error TS2345). Reproduced locally — pnpm --dir src run check-types gives 115 errors with that line (114 pre-existing) and 114 without it, none in any file this PR touches.
• Test that asserted only settled state (review comment 4453429221, closed by 8a491fc53). The pending-write test read the cache after the newer write settled, so an implementation that clobbered the key on the older rejection and wrote it again on success would still have passed - and that mutant did survive the earlier version of the test when it was tried. The cache is now read between the two settlements, which is the window the per-key sequence guard exists to protect.
• Batched same-key writes (review comment 4452829726, closed by 9b089a217). The rollback added in 653661763 captured a per-call previousValue, but Memento.update batches: two writes to the same key issued before the scheduled flush share one pending promise, so a rejected batch rejects both callers and the second restored the first one's unflushed value — a value storage never held. Per-key write numbering and a record of the newest value that actually persisted were introduced, and resetAllState clears that bookkeeping with the caches. Four new tests and eight killed mutants covered it. The numbering was superseded two commits later by the in-flight counting described in the out-of-order-landing bullet.
• Two settle-timing holes in that fix (review comments 4454026019 and 4454026031, closed by ec8850bc6). Both findings held. (a) A rejection only rolled the mirror back when it was the last write still running, so when the newer of two same-key writes failed while an older one was in flight, readers reported the rejected value until the older write settled; the rollback now triggers on "newest write issued for the key", and the landing repair still applies once nothing newer is in flight. (b) A write issued before resetAllState could settle after it and put its value back into persistedValues - which initialize() does not clear - so a later rejected write restored a value storage had discarded. resetAllState now bumps a generation counter and settlements from a stale generation are ignored (the rejection is still reported). Tests: 46/46 in ContextProxy.spec.ts. 14 of 15 hand-written mutants over those lines are killed; the survivor (writeSequences.clear() removed from the reset) is not observable because the generation guard already ignores that settlement, and it is disclosed rather than suppressed.
• Rejection handler retired a write it did not own (review comment 4454293128, closed by 4b3d007a1). The generation check ran after retireWrite(), so a rejection of a write issued before resetAllState() decremented the in-flight count of writes issued after it, and the next landing for that key believed nothing newer was running and overwrote the mirror. The generation check now comes first, which made a second belongsToCurrentState() in the restore condition unreachable - removed rather than left as code no test can reach. Test: keeps a rejected pre-reset write out of the in-flight count of newer writes.
• Rollback record after a reset was not covered (review follow-up on 4b3d007a1, closed by e265d53c4). Every test settled both post-reset writes, so a regression that records a landed write into persistedValues only when no newer write is still in flight passed all 47 tests. restores the write that landed after the reset when a newer one is rejected now covers it through both getGlobalState() and getValues(), and that regression is killed.
• A rejection that is last standing but not newest left a stale mirror (pre-merge check error, closed by c61e97b2c). Persisted S, writes A, B, C for one key: C rejects first, A lands next, B rejects last. Storage ends holding A, yet only the newest-issued rule was in force, so the cache kept reporting S. A rejection now reconciles the mirror when it is the newest write issued or when nothing is left in flight for that key.
• Import test asserted the summary, not the installed object (pre-merge warning, closed by d241d22e3). The host-local-path test now pins the argument to contextProxy.setValues: mode: "code" present, and none of the four path keys - verified by dropping lastSettingsExportPath and lastImageSavePath from the drop list.
• Out-of-order landing (pre-merge check error, closed by 35a8bf339). CodeRabbit’s next Persistence Integrity row described a real hole in that fix: write A starts, write B for the same key starts, B rejects first and A lands afterwards - B rolled the key back to the previously persisted value and A’s success advanced only the bookkeeping, so the cache went on reporting a value storage had already overwritten. updateGlobalState now counts the writes still in flight per key: while one is running the cache optimistically holds the requested value, and when the last write standing for that key settles the cache is reconciled with storage - a landing sets it to the value that landed, a rejection falls back to the newest value that actually persisted. A write that settles while a newer one is still running leaves the cache alone, and because the rollback target is always "the newest value that persisted", no write needs to know whether it was the newest one issued, so the sequence map became redundant and was removed. Twelve hand-written mutants over those lines are all killed.
• Reset bookkeeping untested (pre-merge check warning, closed by 35a8bf339). resetAllState clears outstandingWrites and persistedValues along with the caches; a test now asserts that the reset wrote the key to storage and that a write rejected after a reset leaves the key unset instead of resurrecting the pre-reset value.
• Weak assertion (review comment 4452312785). The host-local import test only rejected warnings containing last, which any unrelated warning would pass. It now asserts the whole warning list (expect(result.warnings).toBeUndefined()) — the silent drop is the assertion.
• Persistence Integrity (second pre-merge check error, closed by 653661763). The error named ContextProxy.updateGlobalState: the cache was written before globalState.update settled, so a rejected write left a phantom setting. It now rolls the key back to the value on disk and rethrows; Memento.update is normalized because it is only a Thenable (dropping the normalization fails 46 tests that stub it). This is the failure the export path can now report, so the reported state and the remembered dialog default stay honest. The rollback shape introduced here was refined twice more, in 9b089a217 and 35a8bf339.
• Security Boundaries (pre-merge check error). The resolver probes the remembered directory before the dialog is shown, and until now that value could arrive from another machine's settings file. sanitizeGlobalSettings drops the four host-local path keys on import, so lastSettingsExportPath can only ever be a path a user picked on this machine, and the probe is bounded to a location the dialogs already own.
• Persistence Integrity (pre-merge check error). A written file whose location could not be remembered no longer gets the plain success toast: exportSettings reports the partial result through common:warnings.settings_export_location_not_remembered with the path and the reason, keeps the console.error for the extension host logs, and does not claim the export was complete.
• Regression Evidence (pre-merge check warning). exportSettings failures are now covered per source: providerSettingsManager.export, contextProxy.export and fs.mkdir rejections each assert the localized error names the thrown reason, that no success toast is shown, and that the location is not remembered.