fix(auth): align OpenCode import replay with current contracts#102
Open
fix(auth): align OpenCode import replay with current contracts#102
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthroughadds opencode account pool detection and import capability to the login flow. users can now discover a companion app's account pool on the same machine, assess its contents, and import accounts via a new menu option instead of creating fresh oauth sessions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AuthMenu as Auth Menu UI
participant CLI as CLI Prompt
participant Storage as Storage Layer
participant Manager as Manager
participant Settings as Settings Hub
User->>AuthMenu: select "Import OpenCode Accounts"
AuthMenu->>CLI: trigger import-opencode action
CLI->>Manager: runAuthLogin(mode: "import-opencode")
Manager->>Storage: detectOpencodeAccountPoolPath()
Storage-->>Manager: pool path (or null)
Manager->>Storage: assessOpencodeAccountPool(path)
Storage-->>Manager: BackupRestoreAssessment
Manager->>CLI: prompt for import confirmation
CLI->>User: confirm import?
User-->>CLI: yes/no
alt Import Confirmed
Manager->>Storage: importAccounts(path)
Storage-->>Manager: import results
Manager->>Settings: applyUiThemeFromDashboardSettings()
Manager->>Settings: configureUnifiedSettings()
Manager->>Settings: resolveMenuLayoutMode()
Settings-->>Manager: settings applied
Manager-->>User: accounts imported
else Import Cancelled
Manager-->>User: cancelled
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes notes for review:
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
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 |
There was a problem hiding this comment.
Pull request overview
Reintroduces and aligns the OpenCode account-pool import flow with the current restore/import contracts, updating detection, CLI recovery menu wiring, tests, and onboarding docs.
Changes:
- Add OpenCode account pool path detection + restore-style assessment helpers in storage.
- Add a Recovery menu action to preview/confirm importing OpenCode accounts via CLI.
- Extend storage/CLI tests and update getting-started guidance for the recovery import path.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/storage.test.ts | Adds detection/assessment coverage for OpenCode pool sources and malformed inputs. |
| test/codex-manager-cli.test.ts | Adds CLI flow coverage for the “import-opencode” recovery path. |
| lib/ui/copy.ts | Adds UI copy label for the new recovery menu item. |
| lib/ui/auth-menu.ts | Adds a new AuthMenuAction and menu item for importing OpenCode accounts. |
| lib/storage.ts | Implements pool-path detection and assessment built on existing restore-candidate logic. |
| lib/codex-manager.ts | Wires the new recovery import action into the login dashboard flow. |
| lib/cli.ts | Extends LoginMode + prompt mapping to include “import-opencode”. |
| docs/getting-started.md | Documents the recovery import option for companion/OpenCode pools. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/storage.test.ts
Outdated
| delete process.env.CODEX_OPENCODE_POOL_PATH; | ||
| else process.env.CODEX_OPENCODE_POOL_PATH = originalPoolPath; | ||
| setStoragePathDirect(null); | ||
| await fs.rm(tempRoot, { recursive: true, force: true }); |
lib/codex-manager.ts
Outdated
Comment on lines
+4374
to
+4384
| await runActionPanel( | ||
| "Import OpenCode Accounts", | ||
| `Importing from ${assessment.backup.path}`, | ||
| async () => { | ||
| const imported = await importAccounts(assessment.backup.path); | ||
| console.log( | ||
| `Imported ${imported.imported} account${imported.imported === 1 ? "" : "s"}. Skipped ${imported.skipped}. Total accounts: ${imported.total}.`, | ||
| ); | ||
| }, | ||
| displaySettings, | ||
| ); |
lib/codex-manager.ts
Outdated
Comment on lines
+4369
to
+4376
| `Import OpenCode accounts from ${assessment.backup.path}?`, | ||
| ); | ||
| if (!confirmed) { | ||
| continue; | ||
| } | ||
| await runActionPanel( | ||
| "Import OpenCode Accounts", | ||
| `Importing from ${assessment.backup.path}`, |
docs/getting-started.md
Outdated
|
|
||
| The restore manager shows each backup name, account count, freshness, and whether the restore would exceed the account limit before it lets you apply anything. | ||
|
|
||
| If you already have a companion app pool on the same machine, use the recovery import option from the login dashboard to preview and import those saved accounts before creating new OAuth sessions. |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai resolve |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/getting-started.md`:
- Around line 85-86: Update the sentence in docs/getting-started.md to
explicitly name the dashboard item "Import OpenCode Accounts" (so users can find
it in the login dashboard), and add companion documentation changes:
create/modify upgrade notes documenting this behavior change and list any new or
changed npm scripts (if any were introduced) so README, SECURITY, and docs/**
remain consistent with the CLI flags and workflows; reference the dashboard item
"Import OpenCode Accounts" and the upgrade notes and npm scripts sections when
making the edits.
In `@lib/cli.ts`:
- Around line 65-66: The fallback flow is missing a branch to parse the new
"import-opencode" interactive option, so update promptLoginModeFallback in
lib/cli.ts to accept and map both "i" and "import-opencode" (and
full-word/short-letter variants like "I"/"import-opencode") to the same login
mode value used by the interactive menu; then add a vitest regression that calls
promptLoginModeFallback with inputs "i" and "import-opencode" (and optionally
"I") asserting the returned mode equals the expected import-opencode mode.
Ensure any other parsing helper used by promptLoginModeFallback is updated
consistently (e.g., the same enum/constant used by the interactive menu) so
fallback and interactive paths produce identical results.
In `@lib/codex-manager.ts`:
- Around line 4375-4383: After calling importAccounts inside the runActionPanel
callback (the block using runActionPanel and importAccounts with
assessment.backup.path), ensure the Codex auth/active account selection is
refreshed: after the import completes and before finishing the action, call the
module/method that updates Codex auth state (e.g., a function like
syncCodexActiveSelection, refreshCodexAuthState, or whatever existing Codex
manager method you have) so the active Codex account reflects newly imported
accounts and the UI/auth state is refreshed; if no such helper exists, add one
that reads current account storage and sets the active selection accordingly and
invoke it right after the imported result is logged.
- Around line 4374-4389: The import flow calls importAccounts from
lib/codex-manager.ts inside runActionPanel without handling transient filesystem
locks (EBUSY/EPERM) that can be raised by fs.readFile in lib/storage.ts; add
retry logic around the read in importAccounts (or wrap the importAccounts call
here) to retry on EBUSY/EPERM with a small backoff and limited attempts, and add
a vitest regression in test/codex-manager-cli.test.ts that simulates fs.readFile
throwing EBUSY/EPERM once (then succeeding) to assert the import still
completes; reference the importAccounts function and the fs.readFile behavior in
lib/storage.ts when implementing the retry and update tests accordingly.
In `@lib/storage.ts`:
- Around line 2072-2099: The detectOpencodeAccountPoolPath function adds
candidate paths from appDataBases and homedir without normalizing them, causing
cwd-dependent behavior for relative APPDATA/LOCALAPPDATA values; update
detectOpencodeAccountPoolPath to resolve each candidate before adding/probing
(use resolvePath on values passed to join for entries added from appDataBases
and the homedir entry that uses ACCOUNTS_FILE_NAME), keep the existing explicit
env resolution logic, and add a Vitest regression that sets relative
APPDATA/LOCALAPPDATA (and a relative homedir) to verify the function returns the
resolved absolute path when the target file exists; reference
detectOpencodeAccountPoolPath, ACCOUNTS_FILE_NAME, resolvePath, join, and
existsSync when updating code and tests.
In `@test/codex-manager-cli.test.ts`:
- Around line 1018-1146: Add two deterministic regression tests to
test/codex-manager-cli.test.ts targeting runCodexMultiAuthCli: one that mocks
importAccountsMock to throw filesystem errors with error.code set to "EBUSY" and
"EPERM" (use mockRejectedValueOnce(new Error("...")) then set the code property)
and assert the CLI handles them like other login paths (exit code 0,
importAccountsMock called, and an error message containing "Import failed" or
similar via console.error spy); and a second that reproduces the in-flight
concurrency race by making importAccountsMock return a promise that first
rejects with an "in-flight" error (or a rejection) and then resolves on retry
(e.g., mockRejectedValueOnce then mockResolvedValueOnce) and assert
runCodexMultiAuthCli returns to the dashboard (exit 0), promptLoginModeMock
called the expected number of times, and importAccountsMock was
retried—reference the existing mocks assessOpencodeAccountPoolMock,
importAccountsMock, promptLoginModeMock, confirmMock, and the
runCodexMultiAuthCli invocation to place these new tests consistent with the
surrounding tests.
- Around line 1169-1229: The parameterized test currently invokes "auth
restore-backup" instead of exercising the fresh/reset actions and then checking
that a subsequent "auth login" does not show the startup restore prompt; update
the test to call runCodexMultiAuthCli with the deliberate action under test (use
the existing action mocks deleteSavedAccountsMock or resetLocalStateMock via
runCodexMultiAuthCli(["auth", "<appropriate-action>"]) or call the helper that
triggers those code paths), then invoke runCodexMultiAuthCli(["auth", "login"])
in the same session and assert promptLoginModeMock was not called and
confirmMock was called once (instead of running
restore-backup/restoreNamedBackupMock). Reference runCodexMultiAuthCli,
deleteSavedAccountsMock, resetLocalStateMock, promptLoginModeMock, confirmMock
to locate and change the two-step flow in the test so it matches the test title.
In `@test/storage.test.ts`:
- Around line 4794-4840: Add a deterministic race-regression test that simulates
the file changing between detection and assessment: after writing a valid pool
JSON to poolPath and calling detectOpencodeAccountPoolPath() (to verify
detection still returns poolPath), immediately mutate the file (e.g., overwrite
poolPath with "not valid json" or unlink it) before calling
assessOpencodeAccountPool(), then assert assessOpencodeAccountPool() reports
backup.valid === false, eligibleForRestore === false, imported === null and that
loadAccounts() remains unchanged; use the same helpers
detectOpencodeAccountPoolPath, assessOpencodeAccountPool, poolPath and
loadAccounts, await fs promises to keep the test deterministic under vitest and
perform the atomic overwrite/unlink between detect and assess to reproduce the
race.
- Around line 4769-4818: Add deterministic Windows-specific regression tests:
add one test where process.env.LOCALAPPDATA is deleted (or set to undefined),
process.env.APPDATA is set to tempRoot and platform pinned to Windows if your
test harness supports it, then write the pool file and assert
detectOpencodeAccountPoolPath() returns the APPDATA-derived pool path and
assessOpencodeAccountPool() reports valid/eligible/imported; add another test
that sets process.env.CODEX_OPENCODE_POOL_PATH to an explicit custom path
(different from APPDATA/LOCALAPPDATA), write the pool file there, and assert
detectOpencodeAccountPoolPath() prefers that path and
assessOpencodeAccountPool() uses it. Reference symbols:
detectOpencodeAccountPoolPath, assessOpencodeAccountPool,
CODEX_OPENCODE_POOL_PATH, poolPath, tempRoot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d19acb00-4c4d-427e-932a-354039727a90
📒 Files selected for processing (8)
docs/getting-started.mdlib/cli.tslib/codex-manager.tslib/storage.tslib/ui/auth-menu.tslib/ui/copy.tstest/codex-manager-cli.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/getting-started.md
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/cli.tslib/ui/auth-menu.tslib/ui/copy.tslib/codex-manager.tslib/storage.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.tstest/codex-manager-cli.test.ts
🔇 Additional comments (7)
lib/ui/copy.ts (1)
19-19: good menu copy addition
lib/ui/copy.ts:19(line 19) cleanly adds the recovery label and matches the new action id used elsewhere.lib/ui/auth-menu.ts (1)
73-73: action wiring is consistent
lib/ui/auth-menu.ts:73,lib/ui/auth-menu.ts:528, andlib/ui/auth-menu.ts:694(line 73, line 528, line 694) correctly wire the new action through type union, focus identity, and recovery menu rendering.Also applies to: 528-528, 694-698
lib/storage.ts (1)
2469-2496: assessment flow reuse looks solid
lib/storage.ts:2469(line 2469) reusesloadBackupCandidateandassessNamedBackupRestoreCandidate, so dedupe/limit/preview behavior stays aligned with existing restore contracts. no new concurrency risk is introduced in this path.test/storage.test.ts (1)
16-23: imports are aligned with the new storage api surface.good update in
test/storage.test.ts:16-23; the new tests consume both additions cleanly.test/codex-manager-cli.test.ts (1)
17-19: nice wiring for the new storage api mocks.the new
assessOpencodeAccountPoolandimportAccountsmocks are exported and reset consistently, which keeps this suite isolated and deterministic. test/codex-manager-cli.test.ts:17-19, test/codex-manager-cli.test.ts:146-148, test/codex-manager-cli.test.ts:643-684Also applies to: 146-148, 643-684
lib/codex-manager.ts (2)
65-91: import and type wiring looks consistent with current contracts.
lib/codex-manager.ts:65-91andlib/codex-manager.ts:103-107are clean: new storage/settings symbols are correctly imported and used.Also applies to: 103-107
4759-4761: type alias migration is clean and keeps the local api stable.
lib/codex-manager.ts:4759-4761correctly maps to storage metadata/assessment types without changing downstream usage.
Contributor
✅ Actions performedComments resolved and changes approved. |
ndycode
force-pushed
the
fresh/18-import-adapter-opencode
branch
from
704af0a to
e6e235a
Compare
ndycode
force-pushed
the
fresh/18-import-adapter-opencode
branch
from
5bd459c to
3b1ba04
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fresh/17-health-summary-dashboard#82What Changed
CODEX_OPENCODE_POOL_PATH.Validation
cmd /c npm test -- test/storage.test.ts -t "opencode account pool detection"cmd /c npm test -- test/codex-manager-cli.test.ts -t "OpenCode"cmd /c npm test -- test/documentation.test.tsnpm run lintnpm run typechecknpm run buildcmd /c npm testDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
fresh/18-import-adapter-opencodeback tofresh/17-health-summary-dashboardAdditional Notes
fresh/17-health-summary-dashboard, notmain.EPERMimport regression intest/codex-manager-cli.test.ts.e6e235a.note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr replays the opencode import-adapter slice on top of the health-summary base, wiring
assessOpencodeAccountPoolandimportAccountsinto therunAuthLoginloop as a newimport-opencodemode. the core mechanics — detection fallback order, explicitCODEX_OPENCODE_POOL_PATHoverride that short-circuits auto-detection, self-import guard (present in bothassessOpencodeAccountPoolandimportAccountsas defense-in-depth), basename path redaction in the ui, and non-fatal error handling via try/catch around both the assessment and the action panel — are all correctly implemented and align with the existing restore-backup patterns.assessment.erroris pre-formatted inloadBackupCandidate,basenameis used for the confirmation prompt and action subtitle,wouldExceedLimithas its own distinct message, the explicit env override now returns null instead of silently falling back, and the homedir test sets bothHOMEandUSERPROFILEto cover posix and windows.assessment.error(already the output offormatRedactedFilesystemErroras stored inloadBackupCandidate) is passed throughformatRedactedFilesystemErrora second time incodex-manager.ts:4452-4454— this is idempotent for current fixtures but fragile by design; logging the value directly is safer.mode: "import-opencode"when the pool is absent (the"No OpenCode account pool was detected."path), and no test confirms the loop continues silently when the user declines the confirmation prompt — both are relevant to windows filesystem transient-lock scenarios.Confidence Score: 3/5
Important Files Changed
import-opencodebranch torunAuthLogin: assessment is try/caught, path leaks are redacted viabasename,wouldExceedLimithas its own message, and import failures stay non-fatal. one design inconsistency:assessment.error(already formatted byformatRedactedFilesystemErrorinloadBackupCandidate) is passed through the formatter a second time.detectOpencodeAccountPoolPath(explicit env override short-circuits auto-detection and returns null when file is missing),assessOpencodeAccountPool,equalsResolvedStoragePath(case-insensitive on win32), self-import guard inimportAccounts, and promotesformatRedactedFilesystemErrorto public export. all throw paths are now guarded in callers.detectOpencodeAccountPoolPathandassessOpencodeAccountPool: explicit override, missing explicit path, homedir fallback (sets bothHOMEandUSERPROFILE— posix and windows covered), malformed source, and self-import rejection. self-import guard inimportAccountsis also covered.Sequence Diagram
sequenceDiagram participant U as User participant CM as codex-manager (runAuthLogin) participant S as storage.ts participant FS as Filesystem U->>CM: selects "import-opencode" CM->>S: assessOpencodeAccountPool({ currentStorage }) S->>S: detectOpencodeAccountPoolPath() Note over S: checks CODEX_OPENCODE_POOL_PATH first,<br/>falls back to LOCALAPPDATA/APPDATA/homedir S->>FS: existsSync(candidatePath) FS-->>S: exists / not found S->>S: equalsResolvedStoragePath(path, activeStoragePath) alt path is active storage file S-->>CM: throws "Import source cannot be the active storage file." CM->>U: console.error(redacted label) else no pool found S-->>CM: null CM->>U: "No OpenCode account pool was detected." else pool found S->>FS: loadBackupCandidate(resolvedPath) FS-->>S: candidate (error already formatRedactedFilesystemError'd) S->>S: buildBackupFileMetadata + assessNamedBackupRestoreCandidate S-->>CM: BackupRestoreAssessment alt not valid / not eligible CM->>U: assessment.error or generic message else wouldExceedLimit CM->>U: limit message with counts else eligible CM->>U: confirm("Import OpenCode accounts from <basename>?") U-->>CM: confirmed true/false alt declined CM->>U: (returns to menu) else confirmed CM->>S: importAccounts(assessment.backup.path) S->>S: equalsResolvedStoragePath guard (second check) S->>FS: readFile + merge accounts FS-->>S: merged result S-->>CM: { imported, skipped, total } CM->>U: success log end end endPrompt To Fix All With AI
Last reviewed commit: "test(auth): cover HO..."