fix(config): load legacy Windows antigravity config so account switching settings are applied#406
fix(config): load legacy Windows antigravity config so account switching settings are applied#406ndycode wants to merge 1 commit intoNoeFabris:mainfrom
Conversation
WalkthroughThis PR adds Windows legacy config migration functionality to the config loader. It introduces two new functions: getLegacyWindowsConfigDir() to determine the legacy %APPDATA%/opencode path, and resolveUserConfigPath() to resolve the user config path and handle migration of legacy Windows configs to the new location with automatic fallback on failure. The getUserConfigPath() function is modified to delegate to resolveUserConfigPath(). A comprehensive test suite verifies migration behavior, including legacy file removal, migrated config path resolution, and loading of legacy antigravity.json settings. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/plugin/config/loader.ts`:
- Around line 68-87: The migration block mishandles a failing unlinkSync: if
copyFileSync succeeds but unlinkSync throws, the outer catch logs a full
migration failure and returns legacyPath even though newPath contains the
migrated config; fix by wrapping unlinkSync in its own try/catch (inside the
catch that handles renameSync failure), so on unlink errors you call log.warn
with a message like "Copied config but failed to remove legacy file, leaving
both files" including legacyPath/newPath/error, and still return newPath; keep
the existing outer catch for true migration failures (e.g., copyFileSync errors)
so renameSync, copyFileSync, unlinkSync are referenced accordingly and the
function returns newPath when the config was successfully copied.
🧹 Nitpick comments (4)
src/plugin/config/loader.ts (2)
56-61:getUserConfigPath()now has file-system side effects (mkdir, rename, copy, unlink) — consider documenting this.Previously callers could assume
getUserConfigPath()was a pure path lookup. It now performs I/O on first call when a legacy config is detected. A JSDoc note ongetUserConfigPath(or at least onresolveUserConfigPath) would help future maintainers.
284-287: Exposing internals via__testExportsin production code.This is a common pragmatic pattern, but it ships migration internals to production consumers. If the module bundler supports it, a cleaner alternative is using
@visibleForTesting-style annotations or re-exporting only in the test environment. Low priority given the limited surface area.src/plugin/config/loader.test.ts (2)
53-82: Consider adding a test for the migration failure fallback path.The current tests cover the happy path (successful migration) and end-to-end settings loading, which is great. However, the fallback behavior in
resolveUserConfigPath(Lines 79–87 ofloader.ts) — where migration fails and the function returnslegacyPath— is untested. A test that makes the destination directory non-writable (or uses a read-only mock) would increase confidence that the fallback works correctly and that user settings are still loaded from the legacy location.
69-72: Clarify the two-step test approach with a brief comment.Line 70 explicitly triggers migration with
"win32", then Line 72 callsloadConfig(tempRoot)which internally callsresolveUserConfigPath()withprocess.platform. On non-Windows CI this means the migration gate is bypassed (returnsnewPathdirectly since the platform isn't"win32"), which only works because the file was already migrated by Line 70. A one-line comment would prevent confusion for future readers.Suggested clarification
- // Force legacy -> new path migration even when tests run on non-Windows CI. + // Force legacy -> new path migration even when tests run on non-Windows CI. + // loadConfig() below will find the already-migrated file at OPENCODE_CONFIG_DIR. __testExports.resolveUserConfigPath("win32");
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/plugin/config/loader.test.tssrc/plugin/config/loader.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugin/config/loader.test.ts (1)
src/plugin/config/loader.ts (2)
__testExports(285-287)loadConfig(237-259)
src/plugin/config/loader.ts (2)
src/plugin/storage.ts (1)
getConfigDir(328-328)src/plugin/config/index.ts (1)
getUserConfigPath(25-25)
🔇 Additional comments (4)
src/plugin/config/loader.ts (2)
12-14: LGTM on the updated imports.All newly imported symbols (
copyFileSync,mkdirSync,renameSync,unlinkSync,dirname) are used in the new migration logic.
40-48: LGTM on legacy directory resolution.Correctly mirrors the
%APPDATA%fallback pattern and isolates the legacy path logic.src/plugin/config/loader.test.ts (2)
12-36: Good environment isolation with proper save/restore of env vars and temp directory cleanup.The
beforeEach/afterEachpattern correctly handles theundefinedcase for env vars (deleting instead of setting toundefinedstring) and usesrmSyncwithforce: truefor cleanup.
38-51: Migration test is solid — covers the happy path well.Verifies the resolved path, confirms the new file was created, and asserts the legacy file was removed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| try { | ||
| mkdirSync(dirname(newPath), { recursive: true }); | ||
| try { | ||
| renameSync(legacyPath, newPath); | ||
| log.info("Migrated Windows config via rename", { from: legacyPath, to: newPath }); | ||
| } catch { | ||
| copyFileSync(legacyPath, newPath); | ||
| unlinkSync(legacyPath); | ||
| log.info("Migrated Windows config via copy+delete", { from: legacyPath, to: newPath }); | ||
| } | ||
| return newPath; | ||
| } catch (error) { | ||
| // Fallback to legacy path if migration failed, so user settings are still respected. | ||
| log.warn("Failed to migrate legacy Windows config, falling back to legacy path", { | ||
| legacyPath, | ||
| newPath, | ||
| error: String(error), | ||
| }); | ||
| return legacyPath; | ||
| } |
There was a problem hiding this comment.
If unlinkSync fails after a successful copyFileSync, the log is misleading and the return value is suboptimal.
When copyFileSync (Line 74) succeeds but unlinkSync (Line 75) throws, the exception propagates to the outer catch (Line 79), which logs "Failed to migrate" and returns legacyPath. At that point newPath already contains the correct config, so:
- The warning is misleading — migration partially succeeded (copy worked, delete didn't).
- Returning
legacyPathis harmless this run (same content), but on the next run both files exist andnewPathwins (Line 64), so the user silently flips paths between invocations.
Consider catching unlinkSync failures separately so you can still return newPath and log a more accurate warning about the leftover legacy file.
Suggested fix
} catch {
copyFileSync(legacyPath, newPath);
- unlinkSync(legacyPath);
- log.info("Migrated Windows config via copy+delete", { from: legacyPath, to: newPath });
+ try {
+ unlinkSync(legacyPath);
+ } catch (unlinkError) {
+ log.warn("Migrated config but could not remove legacy file", {
+ legacyPath,
+ error: String(unlinkError),
+ });
+ }
+ log.info("Migrated Windows config via copy+delete", { from: legacyPath, to: newPath });
}🤖 Prompt for AI Agents
In `@src/plugin/config/loader.ts` around lines 68 - 87, The migration block
mishandles a failing unlinkSync: if copyFileSync succeeds but unlinkSync throws,
the outer catch logs a full migration failure and returns legacyPath even though
newPath contains the migrated config; fix by wrapping unlinkSync in its own
try/catch (inside the catch that handles renameSync failure), so on unlink
errors you call log.warn with a message like "Copied config but failed to remove
legacy file, leaving both files" including legacyPath/newPath/error, and still
return newPath; keep the existing outer catch for true migration failures (e.g.,
copyFileSync errors) so renameSync, copyFileSync, unlinkSync are referenced
accordingly and the function returns newPath when the config was successfully
copied.
Greptile OverviewGreptile SummaryThis PR fixes a configuration loading issue on Windows where legacy config settings in Key Changes:
Implementation Quality:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant getUserConfigPath
participant resolveUserConfigPath
participant FS as File System
participant Logger
User->>getUserConfigPath: Request config path
getUserConfigPath->>resolveUserConfigPath: Call with process.platform
alt Platform is not win32
resolveUserConfigPath-->>getUserConfigPath: Return new path only
else Platform is win32
resolveUserConfigPath->>FS: Check if legacy config exists
resolveUserConfigPath->>FS: Check if new config exists
alt Legacy exists AND new doesn't exist
resolveUserConfigPath->>FS: Create new config directory
alt Try rename first
resolveUserConfigPath->>FS: renameSync(legacy, new)
resolveUserConfigPath->>Logger: Log migration via rename
resolveUserConfigPath-->>getUserConfigPath: Return new path
else Rename fails
resolveUserConfigPath->>FS: copyFileSync(legacy, new)
resolveUserConfigPath->>FS: unlinkSync(legacy)
resolveUserConfigPath->>Logger: Log migration via copy+delete
resolveUserConfigPath-->>getUserConfigPath: Return new path
end
else Migration fails completely
resolveUserConfigPath->>Logger: Warn about migration failure
resolveUserConfigPath-->>getUserConfigPath: Return legacy path (fallback)
else Legacy doesn't exist OR new already exists
resolveUserConfigPath-->>getUserConfigPath: Return new path
end
end
getUserConfigPath-->>User: Return resolved path
|
|
This doesn't solve issue #405 and the plugin already contains the logic that migrates the config file from the legacy path to the new one. |
2 participants
Fixes #405.
What was happening
On Windows, we only read config from ~/.config/opencode/antigravity.json.
If a user still had config in the legacy path %APPDATA%\opencode\antigravity.json, their settings were effectively ignored, and defaults were used instead (scheduling_mode: cache_first, etc). That can look like getting stuck on the first rate-limited account for a long time before switching.
What changed
Files changed
Verification