refactor: extract storage identity helpers#171
refactor: extract storage identity helpers#171ndycode wants to merge 7 commits intorefactor/pr3-storage-error-helpersfrom
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. |
📝 WalkthroughWalkthroughextracted identity normalization into a new module and re-exported selected helpers and an error helper from lib/storage.ts. tests updated to exercise email normalization, identity-key construction, refresh-token hashing, and to import toStorageError via lib/storage.ts (see lib/storage.ts:1, lib/storage/identity.ts:1, test/storage.test.ts:1). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes concerns to validate:
🚥 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
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: 1
🤖 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/identity.ts`:
- Around line 51-62: getAccountIdentityKey currently embeds the raw refresh
token (via toAccountIdentityRef -> refreshToken branch) which can leak if
logged; change getAccountIdentityKey to never return the raw token: either
remove the refreshToken fallback entirely (preferable) or, if fallback is
required, replace the raw token with a deterministic cryptographic hash (use
Node crypto.createHash('sha256').update(trimmedToken).digest('hex')) before
composing the key; update references to
toAccountIdentityRef/getAccountIdentityKey accordingly. Add Vitest unit tests
for getAccountIdentityKey covering all four branches (accountId+emailKey,
accountId only, emailKey only, refreshToken fallback) and assert the returned
key never contains the raw refresh token (and when hashing is used, assert
expected hash format), and ensure callers (e.g., functions in
oc-chatgpt-import-adapter.ts) do not log the raw token.
🪄 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: c871af2b-dd8d-4adf-a18a-a15518e75e71
📒 Files selected for processing (2)
lib/storage.tslib/storage/identity.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 (1)
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/storage.tslib/storage/identity.ts
🔇 Additional comments (3)
lib/storage/identity.ts (1)
1-49: lgtm on types and normalization helpers.the three normalize functions and
toAccountIdentityRefcorrectly handle empty/null inputs and trim whitespace.normalizeEmailKeylowercases which preserves case-insensitive dedup behavior inlib/storage.ts.lib/storage.ts (2)
18-26: clean extraction with backward-compatible re-exports.the re-exports at lines 18-21 preserve the public api surface so existing consumers of
lib/storage.tscontinue to work. the.jsextension in the import path is correct for esm module resolution.
1164-1166: new options type looks fine.
AccountMatchOptionswith the optionalallowUniqueAccountIdFallbackWithoutEmailflag is used byfindUniqueAccountIdMatchIndex(line 1285) andfindMatchingAccountIndex(line 1326). the default behavior remains unchanged since the option defaults to undefined/false.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 25-29: The import normalizeEmailKey is unused and should be
removed from the import list; keep AccountIdentityRef and toAccountIdentityRef
(which already calls normalizeEmailKey internally) and leave the existing
re-export in place. Edit the import statement that currently lists
normalizeEmailKey so it only imports the necessary symbols (AccountIdentityRef,
toAccountIdentityRef), then run the linter/tsc to verify there are no
unused-import errors left.
🪄 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: 83ecdaa2-5050-4d83-9e44-f1c13c02bd5d
📒 Files selected for processing (3)
lib/storage.tslib/storage/identity.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 (2)
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.ts
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/storage/identity.tslib/storage.ts
🔇 Additional comments (7)
lib/storage/identity.ts (2)
38-40: refresh token hashing now prevents leakage — past concern resolved.the
hashRefreshTokenKeyfunction atlib/storage/identity.ts:38-40uses sha256 to hash the refresh token before embedding it in the identity key at line 67. this addresses the previous review concern about raw token exposure in logs or storage keys.test coverage at
test/storage.test.ts:137-150validates the hashing behavior and confirms the raw token is not present in the output.Also applies to: 64-68
1-70: implementation is clean with no logging that could leak tokens or emails.the module has no
log.*calls, so there's no risk of leakingrefreshTokenorlib/storage.ts (2)
19-23: re-exports preserve the public api surface — good.barrel re-exports at
lib/storage.ts:19-23maintain backward compatibility for consumers importinggetAccountIdentityKey,normalizeEmailKey,toStorageError, andformatStorageErrorHintfromlib/storage.js.
1144-1146: localAccountMatchOptionstype is correctly scoped.type remains internal to this module and supports the identity matching logic. no issues.
test/storage.test.ts (3)
1-1: imports updated to use barrel re-exports — good.
test/storage.test.ts:16,22,30now importgetAccountIdentityKey,normalizeEmailKey, andtoStorageErrorfrom../lib/storage.jsinstead of direct paths. this validates the re-export surface and matches the refactor intent.Also applies to: 16-16, 22-22, 30-30
97-151: test coverage for identity key logic is thorough.the "account identity keys" block at
test/storage.test.ts:97-151covers:
- email normalization (trim + lowercase) at lines 98-100
- undefined/blank email handling at lines 102-105
- composite key (accountId + email) at lines 107-115
- accountId-only fallback at lines 117-125
- email-only fallback at lines 127-135
- refresh-token sha256 hashing at lines 137-150
line 149 explicitly asserts the raw token is not present in the output — good defensive check against regression.
137-150: refresh-token hashing test computes expected hash independently — deterministic.
test/storage.test.ts:139-141computes the expected sha256 hash usingcreateHashfromnode:crypto, then compares againstgetAccountIdentityKeyoutput. this avoids coupling the test to implementation details while validating correctness.
ndycode
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
All review threads are resolved and follow-up commits addressed this stale automated change request.
1 participant
Summary
lib/storage.tsinto a dedicated helper moduleWhat Changed
lib/storage/identity.tswith account identity normalization and key-building helperslib/storage.tsto import and re-export the public identity helpers so the current surface stays intactValidation
npm run test -- test/storage.test.tsnpm run lintnpm run typechecknpm run buildRisk and Rollback
c8bcf13to restore the inline identity helper implementationAdditional Notes
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 extracts account identity normalization helpers from
lib/storage.tsinto a dedicatedlib/storage/identity.tsmodule as the next slice of the ongoing storage split. it also adds sha-256 hashing on the refresh-token-only key path (previously raw plaintext) with an inline comment explaining the rationale, and backs it with explicit vitest coverage.key changes:
lib/storage/identity.tsadded:normalizeEmailKey,getAccountIdentityKey,toAccountIdentityRef,AccountIdentityRef, and private helpersnormalizeAccountIdKey,normalizeRefreshTokenKey,hashRefreshTokenKeylib/storage.tsre-exportsgetAccountIdentityKeyandnormalizeEmailKeyto keep the existing public surface intact; importstoAccountIdentityRefandAccountIdentityReffor internal use onlytoStorageErroris newly re-exported fromstorage.ts(previously importable only fromstorage/error-hints.tsdirectly); the test import is updated to matchrefresh:{plaintext}torefresh:{sha256}— the prior review thread concern about this semantic break is now documented in a code comment and covered by a test that verifies the hash and asserts the raw token does not appear in the outputtoAccountIdentityRef(null | undefined)and the all-blankgetAccountIdentityKeyreturningundefinedpaths lack vitest coverage despite the function being newly exported with a null-accepting signatureConfidence Score: 4/5
toAccountIdentityRefand the all-blankgetAccountIdentityKeyreturning undefined — neither path can cause a runtime error, making this non-blocking.Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["caller: getAccountIdentityKey(account)"] --> B["toAccountIdentityRef(account)"] B --> C["normalizeAccountIdKey(accountId)"] B --> D["normalizeEmailKey(email)"] B --> E["normalizeRefreshTokenKey(refreshToken)"] C --> F["AccountIdentityRef\n{ accountId, emailKey, refreshToken }"] D --> F E --> F F --> G{accountId AND emailKey?} G -- yes --> H["account:{id}::email:{emailKey}"] G -- no --> I{accountId only?} I -- yes --> J["account:{id}"] I -- no --> K{emailKey only?} K -- yes --> L["email:{emailKey}"] K -- no --> M{refreshToken?} M -- yes --> N["hashRefreshTokenKey(refreshToken)"] N --> O["refresh:{sha256hash}"] M -- no --> P["undefined"]Prompt To Fix All With AI
Reviews (6): Last reviewed commit: "fix: drop unused storage identity import" | Re-trigger Greptile