Conversation
🦋 Changeset detectedLatest commit: 93097df The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAuth flows were changed to return credentials and context synchronously; automatic token refresh was removed from getCredentials/getContext (requires explicit refreshToken calls); credential checks were renamed/broadened; TM_TOKEN_EXPIRY_MINUTES override added; Supabase session wiring and related tests updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User / CLI
participant CLI as CLI Command
participant AM as AuthManager
participant CS as CredentialStore
participant SB as Supabase
note over AM,CS: New synchronous getCredentials flow — no automatic refresh
U->>CLI: status / context show
CLI->>AM: getCredentials()
AM->>CS: getCredentials({ allowExpired: true })
CS-->>AM: AuthCredentials (may be expired)
AM-->>CLI: credentials/context (sync)
alt Token expired, need refresh
CLI->>AM: refreshToken() (explicit)
AM->>SB: refreshSession / token exchange
SB-->>AM: new tokens + expiresAt
AM->>CS: save updated credentials
AM-->>CLI: refreshed credentials
else Token valid
CLI-->>U: display status/context
end
sequenceDiagram
autonumber
participant Old as Old getCredentials (async)
participant New as New getCredentials (sync)
note over Old: Previously could auto-refresh expired tokens (async)
Old->>Old: Check credentials
alt Expired
Old->>Old: trigger refresh (concurrency-guarded)
Old-->>Caller: fresh credentials (async)
else Valid
Old-->>Caller: valid credentials (async)
end
note over New: Now returns credentials synchronously (may be expired)
New->>New: Check credentials (allowExpired: true)
New-->>Caller: credentials (sync)
note over Caller: Caller must call refreshToken() if needed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-10-15T14:43:40.383ZApplied to files:
🧬 Code graph analysis (2)packages/tm-core/src/auth/credential-store.ts (1)
packages/tm-core/src/auth/auth-manager.ts (1)
⏰ 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)
🔇 Additional comments (9)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/tm-core/tests/auth/auth-refresh.test.ts (4)
53-73: Remove unnecessaryawaiton synchronousgetCredentials().The test correctly expects expired credentials to be returned, but
getCredentials()is now synchronous (as changed inauth-manager.tsline 86), so theawaiton line 68 is unnecessary.Apply this diff:
- const credentials = await authManager.getCredentials(); + const credentials = authManager.getCredentials();
75-92: Remove unnecessaryawaiton synchronousgetCredentials().Same issue as the previous test -
awaitis unnecessary on line 88.Apply this diff:
- const credentials = await authManager.getCredentials(); + const credentials = authManager.getCredentials();
93-112: Remove unnecessaryawaiton synchronousgetCredentials().Same issue -
awaitis unnecessary on line 107.Apply this diff:
- const credentials = await authManager.getCredentials(); + const credentials = authManager.getCredentials();
119-138: Remove unnecessaryawaiton synchronousgetCredentials().Same issue -
awaitis unnecessary on line 132.Apply this diff:
- const credentials = await authManager.getCredentials(); + const credentials = authManager.getCredentials();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
.changeset/dirty-hairs-know.md(1 hunks)apps/cli/src/commands/auth.command.ts(4 hunks)apps/cli/src/commands/context.command.ts(14 hunks)packages/tm-core/src/auth/auth-manager.ts(4 hunks)packages/tm-core/src/auth/credential-store.ts(3 hunks)packages/tm-core/src/auth/oauth-service.ts(2 hunks)packages/tm-core/src/auth/supabase-session-storage.ts(2 hunks)packages/tm-core/src/clients/supabase-client.ts(1 hunks)packages/tm-core/src/storage/storage-factory.ts(2 hunks)packages/tm-core/tests/auth/auth-refresh.test.ts(5 hunks)packages/tm-core/tests/integration/auth-token-refresh.test.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/git_workflow.mdc)
**/*.{test,spec}.{js,ts,jsx,tsx}: Create a test file and ensure all tests pass when all subtasks are complete; commit tests if added or modified
When all subtasks are complete, run final testing using the appropriate test runner (e.g., npm test, jest, or manual testing)
Files:
packages/tm-core/tests/auth/auth-refresh.test.tspackages/tm-core/tests/integration/auth-token-refresh.test.ts
**/*.{test,spec}.*
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Test files should follow naming conventions: .test., .spec., or _test. depending on the language
Files:
packages/tm-core/tests/auth/auth-refresh.test.tspackages/tm-core/tests/integration/auth-token-refresh.test.ts
**/?(*.)+(spec|test).ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
In JavaScript/TypeScript projects using Jest, test files should match *.test.ts and *.spec.ts patterns
Files:
packages/tm-core/tests/auth/auth-refresh.test.tspackages/tm-core/tests/integration/auth-token-refresh.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
**/*.test.ts: Use established mocking patterns for dependencies such as bcrypt and Prisma in test files
Test all code paths, including edge cases and error scenarios, in test files
Use descriptive names for test functions, such as should_return_error_for_invalid_input
Files:
packages/tm-core/tests/auth/auth-refresh.test.tspackages/tm-core/tests/integration/auth-token-refresh.test.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: NEVER use async/await in test functions unless testing actual asynchronous operations
Use synchronous top-level imports in tests; avoid dynamic await import()
Keep test bodies synchronous whenever possible
Files:
packages/tm-core/tests/auth/auth-refresh.test.tspackages/tm-core/tests/integration/auth-token-refresh.test.ts
.changeset/*.md
📄 CodeRabbit inference engine (.cursor/rules/changeset.mdc)
.changeset/*.md: When runningnpm run changesetornpx changeset add, provide a concise summary of the changes for theCHANGELOG.mdin imperative mood, typically a single line, and not a detailed Git commit message.
The changeset summary should be user-facing, describing what changed in the released version that is relevant to users or consumers of the package.
Do not use your detailed Git commit message body as the changeset summary.
Files:
.changeset/dirty-hairs-know.md
.changeset/*
📄 CodeRabbit inference engine (.cursor/rules/new_features.mdc)
Create appropriate changesets for new features, use semantic versioning, include tagged system information in release notes, and document breaking changes if any.
Files:
.changeset/dirty-hairs-know.md
.changeset/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Changeset entries should be user-facing, describing the end-user impact rather than code specifics
Files:
.changeset/dirty-hairs-know.md
🧠 Learnings (1)
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{integration,e2e}/**/*.test.js : Properly mock session objects when required by functions, and reset environment variables between tests if modified.
Applied to files:
packages/tm-core/tests/integration/auth-token-refresh.test.ts
🧬 Code graph analysis (6)
apps/cli/src/commands/auth.command.ts (1)
packages/tm-core/src/auth/types.ts (1)
AuthCredentials(5-14)
packages/tm-core/tests/integration/auth-token-refresh.test.ts (1)
packages/tm-core/src/auth/auth-manager.ts (1)
AuthManager(26-284)
apps/cli/src/commands/context.command.ts (1)
packages/tm-core/src/auth/types.ts (1)
UserContext(16-22)
packages/tm-core/src/auth/oauth-service.ts (3)
packages/tm-core/src/auth/types.ts (1)
AuthCredentials(5-14)apps/cli/src/commands/auth.command.ts (1)
refreshToken(271-307)packages/tm-core/src/auth/auth-manager.ts (1)
refreshToken(109-150)
packages/tm-core/src/clients/supabase-client.ts (2)
packages/tm-core/src/auth/credential-store.ts (1)
CredentialStore(21-266)packages/tm-core/src/auth/supabase-session-storage.ts (1)
SupabaseSessionStorage(16-158)
packages/tm-core/src/auth/auth-manager.ts (1)
packages/tm-core/src/auth/types.ts (2)
AuthCredentials(5-14)UserContext(16-22)
🪛 LanguageTool
.changeset/dirty-hairs-know.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...tch --- Improve auth token refresh flow
(QB_NEW_EN_OTHER)
🪛 markdownlint-cli2 (0.18.1)
.changeset/dirty-hairs-know.md
5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
⏰ 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: Test
🔇 Additional comments (25)
packages/tm-core/src/auth/auth-manager.ts (4)
178-181: LGTM: Synchronous context retrieval.The synchronous implementation of
getContext()is clean and correctly uses the now-synchronousgetCredentials()method.
186-207: LGTM: Synchronous context update.The synchronous implementation correctly merges the context and saves credentials atomically.
212-221: LGTM: Synchronous context clearing.The implementation correctly removes the context while preserving other credential fields.
227-243: LGTM: Async method correctly uses sync credentials.The
getOrganizationService()method remains async (for Supabase initialization) but correctly uses the synchronousgetCredentials()for the credential check.packages/tm-core/src/storage/storage-factory.ts (2)
76-76: LGTM: Correct API migration.The change from
getCredentialsSync()togetCredentials()correctly reflects the API consolidation inauth-manager.tswheregetCredentials()is now synchronous andgetCredentialsSync()has been removed.
106-106: LGTM: Consistent API usage.Same correct migration as line 76.
packages/tm-core/src/clients/supabase-client.ts (1)
20-25: LGTM: Clean refactoring to class-level credential store.Promoting
credentialStorefrom a local variable to a class field enables better sharing withSupabaseSessionStorageand improves the overall architecture by centralizing credential management.packages/tm-core/src/auth/supabase-session-storage.ts (1)
101-118: LGTM: Improved logging and aligned with credential store defaults.The added logging improves observability of the refresh flow. The change from
getCredentials({ allowExpired: true })togetCredentials()maintains the same behavior because, according to the changes incredential-store.ts,allowExpirednow defaults totrueinternally.packages/tm-core/src/auth/oauth-service.ts (2)
284-295: LGTM: Token expiry override for testing.The configurable token expiration via
TM_TOKEN_EXPIRY_MINUTESis a useful debugging/testing feature. The warning log appropriately alerts users when the override is active.
354-365: LGTM: Consistent override implementation.The token expiry override is consistently implemented in both the PKCE flow and the direct token flow, with appropriate logging.
apps/cli/src/commands/auth.command.ts (3)
144-152: LGTM: Correct synchronous status execution.The method correctly calls
displayStatus()synchronously, reflecting the API change where status display no longer requires async operations.
174-241: LGTM: Clean synchronous status display.The
displayStatus()method is correctly implemented as synchronous and properly uses the synchronousgetCredentials()API. The status formatting and display logic is well-structured.
493-495: LGTM: Public API correctly mirrors core changes.The public
getCredentials()method correctly reflects the synchronous signature fromAuthManager..changeset/dirty-hairs-know.md (1)
1-5: Consider adding a markdown heading for better structure.While the changeset content is adequate for documenting the patch-level change, adding a top-level heading would improve the markdown structure and address the static analysis hint.
Apply this diff to add a heading:
--- "task-master-ai": patch --- +# Auth Token Refresh Flow Improvement + Improve auth token refresh flowLikely an incorrect or invalid review comment.
apps/cli/src/commands/context.command.ts (5)
118-119: LGTM!The removal of
awaitis correct. ThedisplayContext()method now returnsContextResultsynchronously (line 129), so noawaitis needed. Error handling remains intact.
129-129: LGTM!The signature change from
asyncto synchronous is correct. The method no longer performs any asynchronous operations sincegetContext()is now synchronous (line 142).
142-142: LGTM!The removal of
awaitis correct. Based on the auth-manager.ts changes,getContext()now returnsUserContext | nullsynchronously instead of a Promise.
253-253: LGTM!All removals of
awaitonupdateContext()andgetContext()calls are correct. Based on the auth-manager.ts changes, these methods are now synchronous. The subsequent operations in each case don't depend on asynchronous completion, so there are no timing issues.Also applies to: 266-266, 346-346, 356-356, 361-361, 371-371, 494-494, 511-511, 616-616, 634-634
685-687: LGTM!The signature change to synchronous is correct. The method simply delegates to
authManager.getContext(), which is now synchronous, so this wrapper should also be synchronous.packages/tm-core/tests/integration/auth-token-refresh.test.ts (6)
79-100: LGTM!The test accurately reflects the new behavior where
getCredentials()returns expired credentials without automatic refresh. The test name, assertions, and comments clearly document this expected behavior.
102-121: LGTM!The test correctly verifies that valid credentials are returned synchronously without modification. The removal of
awaitis appropriate.
124-163: LGTM!The test correctly demonstrates the new manual refresh pattern. It explicitly calls
refreshToken()and verifies that new credentials are saved while preserving the existing context. This is excellent test coverage for the updated refresh flow.
165-187: LGTM!The test correctly verifies error handling when manual refresh fails. The explicit
refreshToken()call and error expectation match the new behavior.
189-229: LGTM!Both edge case tests are correctly updated:
- Expired credentials without refresh token are returned (allowing caller to handle)
- Credentials without
expiresAtare rejected as invalidThese tests provide good coverage of boundary conditions.
232-335: LGTM!The remaining tests correctly verify:
- Clock skew tolerance: Credentials near expiry are returned for Supabase to handle refresh
- Synchronous behavior:
getCredentials()returns immediately without automatic refresh- Concurrent calls: Multiple calls return consistent credentials synchronously
All tests are well-structured and accurately reflect the new behavior.
| */ | ||
| hasValidCredentials(): boolean { | ||
| const credentials = this.getCredentials({ allowExpired: false }); | ||
| const credentials = this.getCredentials({ allowExpired: true }); |
There was a problem hiding this comment.
Method name is misleading given new behavior.
hasValidCredentials() now returns true for expired credentials, which contradicts the method name. "Valid" typically implies the credentials are currently usable, not just present.
This semantic mismatch could mislead callers into thinking expired credentials are actually valid, potentially causing authentication failures downstream.
Consider either:
- Renaming the method to
hasCredentials()to match the actual behavior, or - Reverting to
allowExpired: falseand adding a separatehasCredentials()method for existence checks
-hasValidCredentials(): boolean {
- const credentials = this.getCredentials({ allowExpired: true });
+hasCredentials(): boolean {
+ const credentials = this.getCredentials({ allowExpired: true });
return credentials !== null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const credentials = this.getCredentials({ allowExpired: true }); | |
| hasCredentials(): boolean { | |
| const credentials = this.getCredentials({ allowExpired: true }); | |
| return credentials !== null; | |
| } |
🤖 Prompt for AI Agents
In packages/tm-core/src/auth/credential-store.ts around line 206, the method
currently named hasValidCredentials() calls getCredentials({ allowExpired: true
}) so it returns true for expired credentials; rename the method to
hasCredentials() (and update its signature/comments/tests/usages across the
repo) to reflect that it only checks existence, not current validity, or
alternatively revert the call to getCredentials({ allowExpired: false }) and add
a new hasCredentials() that uses allowExpired: true; implement one of these two
fixes consistently and update all callers and documentation accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/tm-core/src/auth/credential-store.ts (1)
239-243: cleanupCorruptFiles misses quarantined files due to strict suffix check.Quarantined names include timestamp-pid-random (e.g., .corrupt---), so /^\d+$/ fails and old files are never deleted.
Apply this diff:
- const suffix = file.slice(prefix.length); - if (!/^\d+$/.test(suffix)) continue; // Fixed regex, not from variable input + // Quarantine filenames may include timestamp, pid and random suffixes; don't restrict formatpackages/tm-core/src/auth/credential-store.test.ts (1)
532-574: Broaden cleanupCorruptFiles test to cover real quarantine names.Add a case with names like auth.json.corrupt--- to ensure cleanup handles the actual suffix pattern used during quarantine.
Example addition:
it('should remove corrupt files with hyphenated suffixes (timestamp-pid-rand)', () => { const now = Date.now(); const old = `auth.json.corrupt-${now - 8 * 24 * 60 * 60 * 1000}-1234-abcd12`; const recent = `auth.json.corrupt-${now - 1000}-5678-efgh34`; vi.mocked(fs.existsSync).mockReturnValue(true); vi.mocked(fs.readdirSync).mockReturnValue([ { name: old, isFile: () => true }, { name: recent, isFile: () => true }, ] as any); vi.mocked(fs.statSync).mockImplementation((p) => p.includes(old) ? ({ mtimeMs: now - 8 * 24 * 60 * 60 * 1000 } as any) : ({ mtimeMs: now - 1000 } as any) ); vi.mocked(fs.unlinkSync).mockImplementation(() => undefined); store.cleanupCorruptFiles(); expect(fs.unlinkSync).toHaveBeenCalledWith(expect.stringContaining(old)); expect(fs.unlinkSync).not.toHaveBeenCalledWith(expect.stringContaining(recent)); });
♻️ Duplicate comments (2)
packages/tm-core/tests/auth/auth-refresh.test.ts (1)
66-73: LGTM: tests now reflect passive store and external refresh.Expired creds are returned and no unnecessary await; aligns with the refresh-flow design.
Based on learnings
packages/tm-core/src/auth/credential-store.ts (1)
92-92: Generalize inline comments (avoid service-specific coupling).Replace Supabase-specific mentions with implementation-agnostic phrasing.
Apply this diff:
- // Default to allowExpired=true so Supabase can access refresh tokens + // Default to allowExpired=true to enable refresh flows ... - // Return credentials (even if expired, so Supabase can refresh) + // Return credentials (even if expired) to enable refresh flowsBased on learnings
Also applies to: 107-107
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
packages/tm-core/src/auth/auth-manager.spec.ts(1 hunks)packages/tm-core/src/auth/auth-manager.ts(4 hunks)packages/tm-core/src/auth/credential-store.spec.ts(11 hunks)packages/tm-core/src/auth/credential-store.test.ts(5 hunks)packages/tm-core/src/auth/credential-store.ts(3 hunks)packages/tm-core/tests/auth/auth-refresh.test.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/git_workflow.mdc)
**/*.{test,spec}.{js,ts,jsx,tsx}: Create a test file and ensure all tests pass when all subtasks are complete; commit tests if added or modified
When all subtasks are complete, run final testing using the appropriate test runner (e.g., npm test, jest, or manual testing)
Files:
packages/tm-core/src/auth/auth-manager.spec.tspackages/tm-core/tests/auth/auth-refresh.test.tspackages/tm-core/src/auth/credential-store.spec.tspackages/tm-core/src/auth/credential-store.test.ts
**/*.{test,spec}.*
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Test files should follow naming conventions: .test., .spec., or _test. depending on the language
Files:
packages/tm-core/src/auth/auth-manager.spec.tspackages/tm-core/tests/auth/auth-refresh.test.tspackages/tm-core/src/auth/credential-store.spec.tspackages/tm-core/src/auth/credential-store.test.ts
**/?(*.)+(spec|test).ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
In JavaScript/TypeScript projects using Jest, test files should match *.test.ts and *.spec.ts patterns
Files:
packages/tm-core/src/auth/auth-manager.spec.tspackages/tm-core/tests/auth/auth-refresh.test.tspackages/tm-core/src/auth/credential-store.spec.tspackages/tm-core/src/auth/credential-store.test.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: NEVER use async/await in test functions unless testing actual asynchronous operations
Use synchronous top-level imports in tests; avoid dynamic await import()
Keep test bodies synchronous whenever possible
Files:
packages/tm-core/src/auth/auth-manager.spec.tspackages/tm-core/tests/auth/auth-refresh.test.tspackages/tm-core/src/auth/credential-store.spec.tspackages/tm-core/src/auth/credential-store.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
**/*.test.ts: Use established mocking patterns for dependencies such as bcrypt and Prisma in test files
Test all code paths, including edge cases and error scenarios, in test files
Use descriptive names for test functions, such as should_return_error_for_invalid_input
Files:
packages/tm-core/tests/auth/auth-refresh.test.tspackages/tm-core/src/auth/credential-store.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.383Z
Learning: In the claude-task-master codebase, `getCredentials()` in `credential-store.ts` defaults to `allowExpired: true` to enable Supabase refresh flows. The Supabase client automatically handles token refresh when credentials are expired but have a valid refresh token. The `SupabaseSessionStorage` updates the credentials after Supabase performs the refresh. This is an intentional design pattern where the credential store is passive storage and Supabase manages the active token lifecycle.
📚 Learning: 2025-10-15T14:43:40.383Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.383Z
Learning: In the claude-task-master codebase, `getCredentials()` in `credential-store.ts` defaults to `allowExpired: true` to enable Supabase refresh flows. The Supabase client automatically handles token refresh when credentials are expired but have a valid refresh token. The `SupabaseSessionStorage` updates the credentials after Supabase performs the refresh. This is an intentional design pattern where the credential store is passive storage and Supabase manages the active token lifecycle.
Applied to files:
packages/tm-core/tests/auth/auth-refresh.test.tspackages/tm-core/src/auth/credential-store.spec.tspackages/tm-core/src/auth/credential-store.tspackages/tm-core/src/auth/auth-manager.tspackages/tm-core/src/auth/credential-store.test.ts
🧬 Code graph analysis (1)
packages/tm-core/src/auth/auth-manager.ts (1)
packages/tm-core/src/auth/types.ts (2)
AuthCredentials(5-14)UserContext(16-22)
⏰ 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: Test
🔇 Additional comments (7)
packages/tm-core/src/auth/auth-manager.spec.ts (1)
38-38: LGTM: mock updated to hasCredentials().Matches the API rename and expected behavior.
packages/tm-core/src/auth/auth-manager.ts (6)
84-89: Comment clarification approved.The updated comment accurately describes the credential retrieval behavior: credentials are returned as-is (even if expired), and refresh must be triggered explicitly via
refreshToken()or happens automatically during Supabase client API operations. This directly addresses the previous review concern about misleading automatic refresh claims.The synchronous signature change is consistent with the PR's objective to remove automatic refresh from credential retrieval.
179-182: Synchronous pattern correctly applied.The conversion from async to sync is consistent with the new pattern. The logic correctly retrieves credentials synchronously and returns the selected context or null.
187-208: Logic correctly preserved in synchronous conversion.The method correctly retrieves credentials synchronously, merges the context update, and saves the updated credentials. Error handling for unauthenticated state is preserved.
213-222: Context clearing logic is correct.The synchronous implementation correctly retrieves credentials, removes the
selectedContextproperty via destructuring, and saves the updated credentials. Error handling is preserved.
228-244: Correct timing for credential check and Supabase initialization.The synchronous credential retrieval (line 231) followed by async Supabase initialization (line 237) is correct. The token presence check ensures credentials exist before initialization, and any token refresh happens during the
initialize()call. The refreshed session is available viasupabaseClient.getClient()(line 240), not through thecredentialsvariable, which aligns with the pattern where Supabase manages the active token lifecycle.Based on learnings.
173-173: Verify handling of expired credentials when gating with isAuthenticated().
isAuthenticated() now checks presence only, so flows (CLI commands, export.service, storage-factory) may proceed with expired tokens and only fail during API calls. Ensure each usage handles refresh failures gracefully or re-prompts the user to login.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tm-core/src/auth/credential-store.ts (1)
115-121: Cleanup will miss quarantined files due to naming/regex mismatch.Quarantine names include extra suffixes:
.corrupt-<timestamp>-<pid>-<rand>, but cleanup only matches^\d+$. Actual quarantined files won’t be cleaned.Fix one of:
- Relax cleanup match to accept extra hyphenated suffixes:
// replace strict numeric check const suffix = file.slice(prefix.length); if (!/^\d+(?:-.+)?$/.test(suffix)) continue;
- Or simplify quarantine naming to
.corrupt-<timestamp>only.Also consider adding a test that verifies cleanup deletes files with the current quarantine naming.
Also applies to: 239-244
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
packages/tm-core/src/auth/credential-store.spec.ts(12 hunks)packages/tm-core/src/auth/credential-store.test.ts(6 hunks)packages/tm-core/src/auth/credential-store.ts(3 hunks)packages/tm-core/tests/auth/auth-refresh.test.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/git_workflow.mdc)
**/*.{test,spec}.{js,ts,jsx,tsx}: Create a test file and ensure all tests pass when all subtasks are complete; commit tests if added or modified
When all subtasks are complete, run final testing using the appropriate test runner (e.g., npm test, jest, or manual testing)
Files:
packages/tm-core/src/auth/credential-store.spec.tspackages/tm-core/src/auth/credential-store.test.tspackages/tm-core/tests/auth/auth-refresh.test.ts
**/*.{test,spec}.*
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Test files should follow naming conventions: .test., .spec., or _test. depending on the language
Files:
packages/tm-core/src/auth/credential-store.spec.tspackages/tm-core/src/auth/credential-store.test.tspackages/tm-core/tests/auth/auth-refresh.test.ts
**/?(*.)+(spec|test).ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
In JavaScript/TypeScript projects using Jest, test files should match *.test.ts and *.spec.ts patterns
Files:
packages/tm-core/src/auth/credential-store.spec.tspackages/tm-core/src/auth/credential-store.test.tspackages/tm-core/tests/auth/auth-refresh.test.ts
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: NEVER use async/await in test functions unless testing actual asynchronous operations
Use synchronous top-level imports in tests; avoid dynamic await import()
Keep test bodies synchronous whenever possible
Files:
packages/tm-core/src/auth/credential-store.spec.tspackages/tm-core/src/auth/credential-store.test.tspackages/tm-core/tests/auth/auth-refresh.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
**/*.test.ts: Use established mocking patterns for dependencies such as bcrypt and Prisma in test files
Test all code paths, including edge cases and error scenarios, in test files
Use descriptive names for test functions, such as should_return_error_for_invalid_input
Files:
packages/tm-core/src/auth/credential-store.test.tspackages/tm-core/tests/auth/auth-refresh.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.383Z
Learning: In the claude-task-master codebase, `getCredentials()` in `credential-store.ts` defaults to `allowExpired: true` to enable Supabase refresh flows. The Supabase client automatically handles token refresh when credentials are expired but have a valid refresh token. The `SupabaseSessionStorage` updates the credentials after Supabase performs the refresh. This is an intentional design pattern where the credential store is passive storage and Supabase manages the active token lifecycle.
📚 Learning: 2025-10-15T14:43:40.383Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.383Z
Learning: In the claude-task-master codebase, `getCredentials()` in `credential-store.ts` defaults to `allowExpired: true` to enable Supabase refresh flows. The Supabase client automatically handles token refresh when credentials are expired but have a valid refresh token. The `SupabaseSessionStorage` updates the credentials after Supabase performs the refresh. This is an intentional design pattern where the credential store is passive storage and Supabase manages the active token lifecycle.
Applied to files:
packages/tm-core/src/auth/credential-store.tspackages/tm-core/src/auth/credential-store.spec.tspackages/tm-core/src/auth/credential-store.test.tspackages/tm-core/tests/auth/auth-refresh.test.ts
⏰ 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: Test
🔇 Additional comments (16)
packages/tm-core/src/auth/credential-store.test.ts (3)
200-207: Correctly asserts allowExpired=false path.Good: verifies expired tokens are rejected when explicitly disallowed.
230-253: Locks default behavior (allowExpired defaults to true).Exactly the right regression test given the new default. Also checks no spurious expiry warning.
479-555: hasCredentials tests align with new semantics.
- Returns true for expired credentials and valid ones.
- Returns false for missing file/invalid JSON/no expiry.
- Verifies internal call with allowExpired: true.
Based on learnings
packages/tm-core/tests/auth/auth-refresh.test.ts (4)
53-73: Expired creds returned to enable external refresh flows.Matches new contract: return credentials (even if expired); refresh handled elsewhere. No unnecessary awaits used.
Based on learnings
93-112: Expired creds without refreshToken are still returned.Clear separation of retrieval vs refresh; caller handles absence of refresh token.
114-116: Null when no credentials exist.Straightforward, correct expectation.
119-138: Returns credentials regardless of refresh token validity.Good: defers refresh validation to the client, asserts values unchanged.
packages/tm-core/src/auth/credential-store.ts (2)
107-109: Returning expired credentials to enable refresh flows is intentional.Comment and behavior align with the architecture (client handles refresh; store is passive).
Based on learnings
203-207: No remaining usages ofhasValidCredentialsfound.packages/tm-core/src/auth/credential-store.spec.ts (7)
55-58: Rejects expired when allowExpired=false.Matches new API. Good.
72-76: Accepts valid token with allowExpired=false.Correct assertion.
96-113: Confirms default allowExpired=true.Important contract test; prevents regressions.
130-133: Clock skew buffer tests are accurate.Rejects within 30s, accepts outside. Good.
Also applies to: 148-152
168-172: Timestamp normalization/validation coverage is solid.Covers ISO, numeric, invalid, and missing expiresAt paths.
Also applies to: 186-190, 207-210, 225-228
266-270: Persists ISO, normalizes to number on retrieval.Good persistence/normalization contract test.
273-306: hasCredentials existence semantics tested (expired/valid/none).Aligned with renamed method; behavior is clear.
This PR was automatically generated to update documentation based on recent changes. Original commit: fix: auth refresh (#1314)\n\n\n Co-authored-by: Claude <claude-assistant@anthropic.com>
1 participant
Description
Related Issues
How to Test This
# Example commands or stepsExpected result:
Contributor Checklist
npm run changesetnpm testnpm run format-check(ornpm run formatto fix)Changelog Entry
For Maintainers
Summary by CodeRabbit
New Features
Refactor
Tests
Chores