feat(auth): harden opencode import flow#128
feat(auth): harden opencode import flow#128ndycode wants to merge 4 commits intoclean/pr101-dashboard-health-summaryfrom
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. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughRisk Assessment: Medium with strong data-loss and concurrency safeguards This PR implements hardened OpenCode import flow with critical safety mechanisms: Architectural Decision: The concurrency deduplication pattern uses a module-level path-keyed Map to coalesce concurrent prune calls, reducing filesystem churn but requiring callers to understand storage path semantics. Security & Data-Loss: Self-import rejection via case-insensitive path comparison prevents data loss; Test Quality: Comprehensive coverage includes concurrent transactional updates (line 588), concurrent backup reads (line 2210), serialized restores (line 2263), deduplication (line 2685), Windows path alias rejection (line 6333), and proper mock setup (importAccountsMock configured with both mockResolvedValue and mockRejectedValueOnce throughout). The prior greptile note about mock mismatch in self-import test does not reflect actual implementation quality—mocks are properly configured. Walkthroughadds an "import-opencode" login mode that detects an existing OpenCode account pool file, assesses restore eligibility, prompts for confirmation, imports accounts into active storage, and adds snapshot retention/pruning and filesystem-error redaction logic. Changes
Sequence Diagram(s)sequenceDiagram
actor user
participant cli as cli:runAuthLogin()
participant storage as lib/storage.ts:assessOpencodeAccountPool()
participant fs as filesystem
participant importer as lib/storage.ts:importAccounts()
user->>cli: choose import-opencode (i)
cli->>storage: assessOpencodeAccountPool({ currentStorage })
storage->>fs: detectOpencodeAccountPoolPath() / read pool
fs-->>storage: pool path or error/null
alt pool found & valid
storage-->>cli: BackupRestoreAssessment
cli->>user: confirm import (shows basename)
alt user confirms
user-->>cli: yes
cli->>importer: importAccounts(poolPath)
importer->>fs: read pool, write accounts
fs-->>importer: success or error
importer-->>cli: { imported, skipped, total }
cli->>user: print import summary
else user declines
user-->>cli: no
cli->>user: return to menu
end
else no pool or assessment error
storage-->>cli: null or redacted error
cli->>user: log message and return to menu
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes key issues to scrutinize (senior staff engineer view, concise)
🚥 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/storage.ts`:
- Around line 2583-2596: The self-import guard in assessOpencodeAccountPool
currently compares the resolvedPath to getStoragePath() raw, which can be
bypassed by relative/case/separator aliases; update the check to canonicalize
both sides (e.g., run resolvePath/normalize/canonicalize on getStoragePath() as
well) before calling equalsResolvedStoragePath or replace the comparison with a
canonical-equality helper and reuse it for the other occurrences noted (the same
logic around equalsResolvedStoragePath usage at the other locations). Add a
Vitest regression that attempts to import using a Windows-style alias (case-only
or different separators, e.g., backslashes vs forward slashes) to ensure the
guard blocks self-import, and ensure any helper you add is used in
assessOpencodeAccountPool and the other mentioned call sites so the behavior is
consistent.
- Around line 2319-2329: The pruning logic incorrectly treats any object
matching the filename regex as an auto-generated snapshot; update
isAccountSnapshotName and getAccountSnapshotReason usage so pruning (the
deletion block around functions handling 2399-2458) does NOT delete based on
filename shape alone—instead require a reliable marker such as object
metadata/tagging (e.g., a metadata key like autoGenerated=true) or a dedicated
manifest entry that exportNamedBackup sets when creating auto snapshots; change
exportNamedBackup to set that marker for auto snapshots, update the pruning
routine to check that metadata/tag before considering an item for deletion, and
add/adjust vitest references (including the existing storage tests) to assert
manual backups with snapshot-shaped names are preserved and that pruning uses
the metadata gate.
- Around line 2500-2504: enforceSnapshotRetention currently calls
pruneAutoGeneratedSnapshots without the resolved storage root, causing retention
to run against getStoragePath() instead of the transaction-scoped storage root;
update enforceSnapshotRetention (and the similar call around the other
occurrence) to accept and pass the previously computed resolvedStoragePath (or a
storagePath parameter) into pruneAutoGeneratedSnapshots so pruning is pinned to
the same storage root used for the snapshot write (reference function names:
enforceSnapshotRetention, pruneAutoGeneratedSnapshots, getStoragePath,
resolvedStoragePath); update any callers to thread-through the
storagePath/transaction scope and run/adjust the vitest suites (tests around
storage snapshot pinning) to assert retention remains pinned, and ensure the
storage queue logic still handles EBUSY/429 retry semantics where applicable.
In `@test/codex-manager-cli.test.ts`:
- Around line 1081-1127: Add a deterministic test that covers the same-file
import guard by simulating assessOpencodeAccountPoolMock returning a backup
whose path equals the current storage path (use getStoragePathMock() value),
then run runCodexMultiAuthCli(["auth","login"]) and assert that
importAccountsMock and confirmMock are NOT called (and that
assessOpencodeAccountPoolMock was called once); this ensures the code branch in
the login flow that skips self-import when backup.path === getStoragePathMock()
(and any Windows normalization/case-folding) is exercised and protected against
regressions.
- Around line 1481-1518: The test currently injects already-sanitized errors
into assessOpencodeAccountPoolMock so it won't catch redaction regressions;
update the mock response in the assessOpencodeAccountPoolMock.mockResolvedValue
call to use a windows-style full path for both loadError and error (e.g.
"ENOENT: C:\\some\\path\\openai-codex-accounts.json") so the redaction layer
must strip to the basename, then update the assertion on console.log (in the
runCodexMultiAuthCli flow) to expect only "openai-codex-accounts.json" to ensure
only the basename survives; reference assessOpencodeAccountPoolMock,
runCodexMultiAuthCli, and logSpy when making the change.
🪄 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: ae72cf23-0c27-4851-bf34-94612a4e9da6
📒 Files selected for processing (9)
docs/getting-started.mdlib/cli.tslib/codex-manager.tslib/storage.tslib/ui/copy.tstest/cli.test.tstest/codex-manager-cli.test.tstest/documentation.test.tstest/storage.test.ts
👮 Files not reviewed due to content moderation or server errors (6)
- test/documentation.test.ts
- test/cli.test.ts
- lib/cli.ts
- lib/ui/copy.ts
- docs/getting-started.md
- lib/codex-manager.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
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/ui/copy.tslib/cli.tslib/codex-manager.tslib/storage.ts
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
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/documentation.test.tstest/cli.test.tstest/storage.test.tstest/codex-manager-cli.test.ts
🔇 Additional comments (1)
test/codex-manager-cli.test.ts (1)
1289-1315: good windows-path regression here.
test/codex-manager-cli.test.ts:1289-1315drives a real windows-looking path through the import failure branch and asserts the emitted error only keeps the basename. that is the right shape of regression coverage for this flow.As per coding guidelines, "test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."
ndycode
force-pushed
the
clean/pr102-opencode-import-replay-final
branch
from
3f06c8e to
e407350
Compare
| const keepLatestPerReason = Math.max( | ||
| 1, | ||
| options.keepLatestPerReason ?? 1, | ||
| ); |
There was a problem hiding this comment.
pruneAutoGeneratedSnapshots default retention inconsistent with exported constant
keepLatestPerReason defaults to 1 here, but the system retention policy is ACCOUNT_SNAPSHOT_RETENTION_PER_REASON = 3. enforceSnapshotRetention always passes the constant explicitly, so the production path is correct — but any direct caller of the exported pruneAutoGeneratedSnapshots() without specifying keepLatestPerReason silently applies a much more aggressive policy (1 kept per reason vs. 3). on windows this could prune a rollback snapshot that a concurrent snapshotAccountStorage just wrote before its marker was registered, creating a silent data-loss window if the in-flight guard is bypassed by a path-key mismatch.
the default should align with the exported constant:
| const keepLatestPerReason = Math.max( | |
| 1, | |
| options.keepLatestPerReason ?? 1, | |
| ); | |
| const keepLatestPerReason = Math.max( | |
| 1, | |
| options.keepLatestPerReason ?? ACCOUNT_SNAPSHOT_RETENTION_PER_REASON, | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/storage.ts
Line: 2468-2471
Comment:
**`pruneAutoGeneratedSnapshots` default retention inconsistent with exported constant**
`keepLatestPerReason` defaults to `1` here, but the system retention policy is `ACCOUNT_SNAPSHOT_RETENTION_PER_REASON = 3`. `enforceSnapshotRetention` always passes the constant explicitly, so the production path is correct — but any direct caller of the exported `pruneAutoGeneratedSnapshots()` without specifying `keepLatestPerReason` silently applies a much more aggressive policy (1 kept per reason vs. 3). on windows this could prune a rollback snapshot that a concurrent `snapshotAccountStorage` just wrote before its marker was registered, creating a silent data-loss window if the in-flight guard is bypassed by a path-key mismatch.
the default should align with the exported constant:
```suggestion
const keepLatestPerReason = Math.max(
1,
options.keepLatestPerReason ?? ACCOUNT_SNAPSHOT_RETENTION_PER_REASON,
);
```
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
#102clean/pr101-dashboard-health-summaryWhat Changed
Validation
npm run lintnpm run typechecknpm run buildnpm testnpm exec -- vitest run test/destructive-actions.test.ts test/quota-cache.test.ts test/storage.test.ts test/storage-flagged.test.ts test/codex-manager-cli.test.ts test/cli.test.ts test/cli-auth-menu.test.ts test/auth-menu-hotkeys.test.ts test/release-main-prs-regression.test.ts --maxWorkers=1Docs 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
3f06c8eAdditional Notes
#102.9files instead of the old43-file PR.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 wires up the opencode account import slice cleanly on top of the dashboard-health base — it adds
detectOpencodeAccountPoolPath,assessOpencodeAccountPool, and a self-import guard toimportAccountsinlib/storage.ts, a newimport-opencodebranch in the login loop inlib/codex-manager.ts, and a full suite of vitest coverage for detection, assessment, retention pruning, and the manager flow.key observations:
equalsResolvedStoragePathhandles windows case-folding and bothassessOpencodeAccountPoolandimportAccountsindependently enforce the guard; the self-import test now correctly usesmockRejectedValueOnce+errorSpy(addresses the previous review thread)pruneAutoGeneratedSnapshots— the path-keyedretentionEnforcementInFlightmap correctly deduplicates concurrent calls; the concurrent-prune test is thorough with an async unlink gatepruneAutoGeneratedSnapshotsdefaultkeepLatestPerReasonis1, notACCOUNT_SNAPSHOT_RETENTION_PER_REASON = 3— the production path (enforceSnapshotRetention) always passes the constant, so the live retention policy is correct; but any direct caller of the exported function without specifying the option silently applies a more aggressive policy than the exported constant implies (see inline comment)console.log, notconsole.error— minor channel mismatch with the throw and import-failure paths; relevant on windows where stdout/stderr are often captured separatelyredactFilesystemDetailsregex+→*change is intentional and tested but benefits from a short inline comment to explain why single-segment paths are now in scopecontinue(notbreak) for empty snapshot names — all previously flagged issues are addressedConfidence Score: 4/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant LoginLoop as runAuthLogin (loop) participant Storage as lib/storage.ts participant FS as Filesystem User->>LoginLoop: selects "i" / import-opencode LoginLoop->>Storage: assessOpencodeAccountPool({ currentStorage }) Storage->>FS: detectOpencodeAccountPoolPath()<br/>(env override OR LOCALAPPDATA/APPDATA/.opencode) alt CODEX_OPENCODE_POOL_PATH set but missing Storage-->>LoginLoop: throws Error (redacted path) LoginLoop->>User: console.error "Import assessment failed: ..." end alt path equals active storage file Storage-->>LoginLoop: throws "Import source cannot be the active storage file." LoginLoop->>User: console.error "Import assessment failed: ..." end Storage->>FS: loadBackupCandidate(resolvedPath) Storage-->>LoginLoop: BackupRestoreAssessment | null alt null (no pool found) LoginLoop->>User: console.log "No OpenCode account pool was detected." end alt !backup.valid || !eligibleForRestore LoginLoop->>User: console.log redacted error end alt wouldExceedLimit LoginLoop->>User: console.log limit message end LoginLoop->>User: confirm "Import OpenCode accounts from <file>?" alt confirmed LoginLoop->>Storage: importAccounts(assessment.backup.path) Storage->>FS: equalsResolvedStoragePath guard (self-import check) Storage->>FS: loadImportableBackupCandidate Storage->>FS: snapshotAccountStorage (pre-import backup) Storage->>FS: writeAutoSnapshotMarker Storage->>FS: enforceSnapshotRetention → pruneAutoGeneratedSnapshots(keepLatest=3) Storage->>FS: write merged accounts Storage-->>LoginLoop: { imported, skipped, total } LoginLoop->>User: console.log import summary end LoginLoop->>LoginLoop: continue (reload storage next iteration)Prompt To Fix All With AI
Last reviewed commit: "test(login): cover s..."