feat: implement MFA in taskmaster#1438
Conversation
|
WalkthroughAdds end-to-end MFA and session management: new MFA types/errors, Supabase client MFA methods, a SessionManager class for session lifecycle and MFA flows, AuthManager/AuthDomain MFA entrypoints, CLI MFA prompts/handling, and extensive tests and mocks updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Auth Command
participant Domain as AuthDomain
participant Session as SessionManager
participant Supabase
User->>CLI: start login / provide token
CLI->>Domain: authenticateWithToken(token)
Domain->>Session: authenticateWithCode(token)
Session->>Supabase: authenticate / refresh
alt MFA_REQUIRED
Supabase-->>Session: throw AuthenticationError (mfaChallenge)
Session-->>Domain: propagate error w/ mfaChallenge
Domain-->>CLI: surface MFA challenge
CLI->>User: prompt for 6-digit code
User->>CLI: enters code
CLI->>Domain: verifyMFA(factorId, code)
Domain->>Session: verifyMFA(factorId, code)
Session->>Supabase: verify code -> refreshSession
alt valid code
Supabase-->>Session: refreshed session
Session-->>Domain: AuthCredentials
Domain-->>CLI: success
else invalid code
Supabase-->>Session: throw INVALID_MFA_CODE
Session-->>Domain: propagate -> retry or fail
Domain-->>CLI: re-prompt or fail
end
else no MFA
Supabase-->>Session: session + credentials
Session-->>Domain: AuthCredentials
Domain-->>CLI: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.{spec,test}.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.ts📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Files:
**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
Files:
🧠 Learnings (33)📓 Common learnings📚 Learning: 2025-11-24T18:03:13.408ZApplied to files:
📚 Learning: 2025-11-24T18:03:13.408ZApplied to files:
📚 Learning: 2025-11-24T18:03:46.700ZApplied to files:
📚 Learning: 2025-11-24T18:03:13.408ZApplied to files:
📚 Learning: 2025-11-24T18:03:13.408ZApplied to files:
📚 Learning: 2025-11-24T17:56:37.308ZApplied to files:
📚 Learning: 2025-11-24T18:03:13.408ZApplied to files:
📚 Learning: 2025-11-24T17:57:14.728ZApplied to files:
📚 Learning: 2025-11-24T17:56:37.308ZApplied to files:
📚 Learning: 2025-11-24T18:03:13.408ZApplied to files:
📚 Learning: 2025-09-09T19:14:21.367ZApplied to files:
📚 Learning: 2025-09-26T19:28:42.612ZApplied to files:
📚 Learning: 2025-09-02T21:51:27.921ZApplied to files:
📚 Learning: 2025-09-03T12:45:30.724ZApplied to files:
📚 Learning: 2025-10-15T14:43:40.410ZApplied to files:
📚 Learning: 2025-11-24T17:58:07.977ZApplied to files:
📚 Learning: 2025-11-24T18:05:02.103ZApplied to files:
📚 Learning: 2025-11-24T18:04:43.949ZApplied to files:
📚 Learning: 2025-11-24T18:04:43.949ZApplied to files:
📚 Learning: 2025-11-24T17:58:07.977ZApplied to files:
📚 Learning: 2025-11-24T17:56:52.238ZApplied to files:
📚 Learning: 2025-11-24T17:58:07.977ZApplied to files:
📚 Learning: 2025-11-24T18:04:43.949ZApplied to files:
📚 Learning: 2025-11-24T18:04:43.949ZApplied to files:
📚 Learning: 2025-11-24T18:01:44.137ZApplied to files:
📚 Learning: 2025-11-12T20:05:10.138ZApplied to files:
📚 Learning: 2025-11-24T18:03:46.700ZApplied to files:
📚 Learning: 2025-11-24T18:03:13.408ZApplied to files:
📚 Learning: 2025-11-24T18:03:13.408ZApplied to files:
📚 Learning: 2025-11-24T18:03:46.700ZApplied to files:
📚 Learning: 2025-11-24T17:58:07.977ZApplied to files:
📚 Learning: 2025-11-24T18:03:46.700ZApplied to files:
🧬 Code graph analysis (3)packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts (2)
apps/cli/src/commands/auth.command.ts (2)
packages/tm-core/src/modules/auth/managers/auth-manager.ts (2)
⏰ 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 (6)
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: 2
🧹 Nitpick comments (4)
packages/tm-core/src/modules/integration/clients/supabase-client.ts (1)
265-278: Comment no longer matchessignOutbehaviorLine 269 still says “Sign out with global scope to revoke all refresh tokens”, but the call now uses
{ scope: 'local' }. If the intent is to only clear this CLI’s local session (and keep other devices logged in), consider updating the comment to reflect that, so future readers don’t assume a global revoke.packages/tm-core/src/modules/auth/services/session-manager.spec.ts (2)
378-411: Use a more idiomatic pattern for “should throw MFA_REQUIRED”The
try/catchpattern withexpect.fail('Should have thrown MFA_REQUIRED error')is a bit clunky (andexpect.failisn’t part of the usual Vitest/Jest API). You can make this test clearer and less brittle by asserting the rejection directly, e.g.:await expect( sessionManager.authenticateWithCode('test-code') ).rejects.toMatchObject<AuthenticationError>({ code: 'MFA_REQUIRED', mfaChallenge: { factorId: 'factor-123', factorType: 'totp' } });This both simplifies the flow and ensures the assertion message is accurate if the promise doesn’t reject.
65-68: ConsiderresetAllMocksif you hit cross-test mock leakageRight now you call
vi.clearAllMocks()inbeforeEach, which wipes call history but preserves mocked implementations (likefs.existsSyncreturn values). Since most tests explicitly configure the mocks they care about, this is probably fine, but if you ever see order-dependent behavior, switching tovi.resetAllMocks()(and re-stubbing where needed) would fully isolate tests from one another.Also applies to: 23-30
packages/tm-core/src/modules/auth/services/session-manager.ts (1)
19-214: SessionManager design aligns well with Supabase-driven session lifecycleThe new
SessionManagercleanly centralizes session init, token/credential access, MFA flows, refresh, and logout around the Supabase client andContextStore, and the initialization pattern (initializationPromise+waitForInitialization) avoids race conditions while keeping initialization failures non-fatal. This matches the repo’s pattern of letting Supabase own the active token lifecycle with local stores remaining passive.Very minor polish (non-blocking):
refreshToken()buildsAuthCredentialswithout atokenTypefield, while other builders includetokenType: 'standard'. For consistency across call sites, you may want to add it there as well.Also applies to: 222-295, 301-358, 365-389
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/cli/src/commands/auth.command.ts(6 hunks)packages/tm-core/src/modules/auth/auth-domain.ts(1 hunks)packages/tm-core/src/modules/auth/managers/auth-manager.spec.ts(3 hunks)packages/tm-core/src/modules/auth/managers/auth-manager.ts(7 hunks)packages/tm-core/src/modules/auth/services/session-manager.spec.ts(1 hunks)packages/tm-core/src/modules/auth/services/session-manager.ts(1 hunks)packages/tm-core/src/modules/auth/types.ts(2 hunks)packages/tm-core/src/modules/integration/clients/supabase-client.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/fix-tag-complexity-detection.md:0-0
Timestamp: 2025-08-02T15:33:22.656Z
Learning: For changeset files (.changeset/*.md), Crunchyman-ralph prefers to ignore formatting nitpicks about blank lines between frontmatter and descriptions, as he doesn't mind having them and wants to avoid such comments in future reviews.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1200
File: src/ai-providers/custom-sdk/grok-cli/language-model.js:96-100
Timestamp: 2025-09-19T16:06:42.182Z
Learning: The user Crunchyman-ralph prefers to keep environment variable names explicit (like GROK_CLI_API_KEY) rather than supporting multiple aliases, to avoid overlap and ensure clear separation between different CLI implementations.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1105
File: scripts/modules/supported-models.json:242-254
Timestamp: 2025-08-08T11:33:15.297Z
Learning: Preference: In scripts/modules/supported-models.json, the "name" field is optional. For OpenAI entries (e.g., "gpt-5"), Crunchyman-ralph prefers omitting "name" when the id is explicit enough; avoid nitpicks requesting a "name" in such cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1217
File: apps/cli/src/index.ts:16-21
Timestamp: 2025-09-18T16:35:35.147Z
Learning: The user Crunchyman-ralph considers suggestions to export types for better ergonomics (like exporting UpdateInfo type alongside related functions) as nitpicky and prefers not to implement such suggestions.
📚 Learning: 2025-10-15T14:43:40.410Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.410Z
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/modules/auth/managers/auth-manager.tspackages/tm-core/src/modules/auth/services/session-manager.tspackages/tm-core/src/modules/auth/managers/auth-manager.spec.tsapps/cli/src/commands/auth.command.tspackages/tm-core/src/modules/integration/clients/supabase-client.ts
📚 Learning: 2025-11-12T20:05:10.138Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1389
File: packages/tm-core/src/modules/auth/config.ts:55-59
Timestamp: 2025-11-12T20:05:10.138Z
Learning: In packages/tm-core/src/modules/auth/config.ts, the DEFAULT_AUTH_CONFIG Proxy does not require enumeration support (has, ownKeys, getOwnPropertyDescriptor traps). The codebase only uses direct property access on this config object, so the basic get trap is sufficient.
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.spec.ts
🧬 Code graph analysis (2)
packages/tm-core/src/modules/auth/types.ts (2)
packages/tm-core/src/index.ts (1)
AuthenticationError(83-83)packages/tm-core/src/modules/auth/index.ts (1)
AuthenticationError(23-23)
packages/tm-core/src/modules/auth/auth-domain.ts (3)
packages/tm-core/src/modules/auth/types.ts (1)
AuthCredentials(5-14)packages/tm-core/src/index.ts (1)
AuthCredentials(79-79)packages/tm-core/src/modules/auth/index.ts (1)
AuthCredentials(16-16)
⏰ 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 (6)
packages/tm-core/src/modules/integration/clients/supabase-client.ts (1)
5-10: Type-only imports from@supabase/supabase-jslook correctUsing
typeimports forSession,SupabaseClient, andUseris idiomatic and avoids pulling types into the runtime bundle. No issues here.packages/tm-core/src/modules/auth/auth-domain.ts (1)
99-108: AuthDomain MFA façade is consistent and minimalExposing
verifyMFA(factorId, code)here and delegating toauthManager.verifyMFAcleanly wires MFA into the public auth domain without duplicating logic. The docblock clearly explains when to call it.packages/tm-core/src/modules/auth/managers/auth-manager.ts (1)
15-16: SessionManager delegation keeps AuthManager lean and consistentRouting all session/tokens/MFA-related operations through
SessionManager(while keeping OAuth inOAuthService) nicely decouples responsibilities: AuthManager is now essentially a coordinator, and the new MFAverifyMFApath is exposed here without re-implementing any logic. This structure should make future auth changes and testing easier.Also applies to: 31-39, 42-58, 87-98, 114-124, 138-147, 153-162, 167-169
packages/tm-core/src/modules/auth/types.ts (1)
64-73: MFA error typing and payload extension are well-designedThe
MFAChallengetype, additionalAuthErrorCodevariants, and the optionalmfaChallengefield onAuthenticationErrorgive a clear, type-safe channel to surface MFA requirements (e.g.,MFA_REQUIREDwith factor metadata) without breaking existing constructor usages. The constructor signature change (adding a 4th optional param) remains backward compatible.Also applies to: 77-106, 111-127
apps/cli/src/commands/auth.command.ts (1)
117-195: Centralized auth error handling looks goodRouting all top-level auth command failures through
handleAuthError(and typing theerror: unknownparam) keeps the CLI error behavior consistent and avoids repeatingdisplayErrorlogic. No issues spotted here.packages/tm-core/src/modules/auth/managers/auth-manager.spec.ts (1)
8-78: Updated AuthManager singleton tests and mocks look correctThe logger/OAuthService/ContextStore/SessionManager/SupabaseAuthClient mocks line up with the new constructor signatures, and the tests now verify both config propagation on first
getInstanceand that subsequent calls (with or without config) don’t reconstruct dependencies, plusresetInstancebehavior. This suite gives good coverage of the singleton contract for the new auth stack.Also applies to: 83-157
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/tm-core/src/modules/auth/types.ts (1)
64-87: MFA types and AuthenticationError extension look consistent with new flowsThe MFAChallenge/MFAVerificationResult shapes and the extra AuthErrorCode variants align cleanly with
verifyMFAWithRetryand the Supabase MFA client; the additionalmfaChallengefield onAuthenticationErrorshould give callers enough context to drive CLI prompts. No correctness issues spotted here. If you later want stricter typing forfactorType, you could narrow it to known values (e.g.'totp' | string) without affecting current usage.Also applies to: 117-120, 125-141
packages/tm-core/src/modules/auth/managers/auth-manager.ts (1)
15-51: SessionManager delegation and MFA retry logic are soundWiring
SessionManagerinto the constructor and delegatinggetAccessToken,getAuthCredentials,authenticateWithCode,verifyMFA, and the session/refresh/logout helpers keepsAuthManageras a light coordinator and removes a lot of direct Supabase coupling. TheverifyMFAWithRetryimplementation correctly handles per-attempt code prompts, retries only onAuthenticationErrorwithINVALID_MFA_CODE, and returns structuredMFAVerificationResultdata; the extra fallback return is effectively just for TS exhaustiveness. The remaining direct use ofSupabaseAuthClient.getSession()ingetOrganizationServiceis slightly asymmetric with the new SessionManager role but not incorrect, so it can be revisited later if you decide to centralize all session access.Also applies to: 83-193, 202-238
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts (1)
5-395: SupabaseAuthClient MFA and sign-out tests closely match expected behaviorThe logger/session-storage mocks, the structured Supabase client mock, and the verifyMFA/checkMFARequired/signOut suites give solid coverage of the new MFA upgrade and AAL logic as well as local-scope sign-out. The various failure modes (challenge/verify/refresh errors and missing session) are all asserted as
AuthenticationError, which aligns with the shared auth error type. Only very minor nit: in the “refreshSession returns no session” case you invokeverifyMFAtwice for two expectations; you could collapse that into a single call and assert both the class and message, but it’s not required.apps/cli/src/commands/auth.command.ts (1)
70-77: Consider documenting the MFA exception in help text.The help text describes
--yesas "Non-interactive token auth (for scripts)" but doesn't mention that MFA prompts cannot be skipped when MFA is enabled on the account. From the previous review discussion, this is an intentional exception since the MFA code is genuinely required to access the API.Adding a note would help users understand the behavior:
.addHelpText( 'after', ` Examples: $ tm auth login # Browser-based OAuth flow (interactive) $ tm auth login <token> # Token-based authentication - $ tm auth login <token> -y # Non-interactive token auth (for scripts) + $ tm auth login <token> -y # Non-interactive token auth (for scripts) + # Note: MFA prompts cannot be skipped if enabled ` )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CLAUDE.md(1 hunks)apps/cli/src/commands/auth.command.ts(6 hunks)packages/tm-core/src/modules/auth/index.ts(1 hunks)packages/tm-core/src/modules/auth/managers/auth-manager.spec.ts(4 hunks)packages/tm-core/src/modules/auth/managers/auth-manager.ts(7 hunks)packages/tm-core/src/modules/auth/types.ts(2 hunks)packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts(1 hunks)packages/tm-core/src/modules/integration/clients/supabase-client.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tm-core/src/modules/integration/clients/supabase-client.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/fix-tag-complexity-detection.md:0-0
Timestamp: 2025-08-02T15:33:22.656Z
Learning: For changeset files (.changeset/*.md), Crunchyman-ralph prefers to ignore formatting nitpicks about blank lines between frontmatter and descriptions, as he doesn't mind having them and wants to avoid such comments in future reviews.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1200
File: src/ai-providers/custom-sdk/grok-cli/language-model.js:96-100
Timestamp: 2025-09-19T16:06:42.182Z
Learning: The user Crunchyman-ralph prefers to keep environment variable names explicit (like GROK_CLI_API_KEY) rather than supporting multiple aliases, to avoid overlap and ensure clear separation between different CLI implementations.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1105
File: scripts/modules/supported-models.json:242-254
Timestamp: 2025-08-08T11:33:15.297Z
Learning: Preference: In scripts/modules/supported-models.json, the "name" field is optional. For OpenAI entries (e.g., "gpt-5"), Crunchyman-ralph prefers omitting "name" when the id is explicit enough; avoid nitpicks requesting a "name" in such cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1217
File: apps/cli/src/index.ts:16-21
Timestamp: 2025-09-18T16:35:35.147Z
Learning: The user Crunchyman-ralph considers suggestions to export types for better ergonomics (like exporting UpdateInfo type alongside related functions) as nitpicky and prefers not to implement such suggestions.
📚 Learning: 2025-07-18T17:08:48.695Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/commands.mdc:0-0
Timestamp: 2025-07-18T17:08:48.695Z
Learning: Applies to scripts/modules/commands.js : Provide specific error handling for common issues, include troubleshooting hints for each error type, and use consistent error formatting across all commands.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-09T19:14:21.367Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1194
File: .github/workflows/ci.yml:21-21
Timestamp: 2025-09-09T19:14:21.367Z
Learning: User Crunchyman-ralph considers suggestions about adding safer default shell options (like bash -euo pipefail) to GitHub Actions workflows as too nitpicky and prefers not to implement them.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-02T21:51:27.921Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-26T19:28:42.612Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: packages/ai-sdk-provider-grok-cli/package.json:21-35
Timestamp: 2025-09-26T19:28:42.612Z
Learning: Crunchyman-ralph is concerned about preventing production failures when developers add external dependencies to internal packages but forget to add them to the root package.json in their tsdown bundling setup. He wants automated safety mechanisms to catch this during development or CI rather than discovering it in production.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-03T12:45:30.724Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-10-15T14:43:40.410Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.410Z
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:
apps/cli/src/commands/auth.command.tspackages/tm-core/src/modules/auth/managers/auth-manager.tspackages/tm-core/src/modules/auth/managers/auth-manager.spec.ts
📚 Learning: 2025-11-12T20:05:10.138Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1389
File: packages/tm-core/src/modules/auth/config.ts:55-59
Timestamp: 2025-11-12T20:05:10.138Z
Learning: In packages/tm-core/src/modules/auth/config.ts, the DEFAULT_AUTH_CONFIG Proxy does not require enumeration support (has, ownKeys, getOwnPropertyDescriptor traps). The codebase only uses direct property access on this config object, so the basic get trap is sufficient.
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.spec.ts
🧬 Code graph analysis (3)
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts (1)
packages/tm-core/src/modules/auth/types.ts (1)
AuthenticationError(125-142)
apps/cli/src/commands/auth.command.ts (3)
packages/tm-core/src/modules/auth/types.ts (2)
AuthenticationError(125-142)AuthCredentials(5-14)packages/tm-core/src/index.ts (2)
AuthenticationError(83-83)AuthCredentials(79-79)apps/cli/src/utils/error-handler.ts (1)
displayError(25-60)
packages/tm-core/src/modules/auth/managers/auth-manager.spec.ts (2)
packages/tm-core/src/modules/auth/managers/auth-manager.ts (1)
AuthManager(33-352)packages/tm-core/src/modules/auth/types.ts (1)
AuthenticationError(125-142)
⏰ 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 (5)
packages/tm-core/src/modules/auth/managers/auth-manager.spec.ts (2)
16-71: Mocks and singleton behavior tests align with new AuthManager wiringThe logger/OAuthService/ContextStore/SessionManager/SupabaseAuthClient mocks and the singleton tests correctly reflect the new constructor shape and config propagation rules for
AuthManager. The OAuthService spy pattern effectively asserts “config on first call, ignored thereafter” without overfitting to implementation details. No issues from the mocking strategy given the existing “code smell” note in the header.Also applies to: 92-167
169-324: MFA retry tests thoroughly exercise verifyMFAWithRetry behaviorThe
verifyMFAWithRetrysuite cleanly covers first-try success, retry-on-INVALID_MFA_CODE, failure after max attempts, immediate failure on non-INVALID_MFA_CODE, and custom/defaultmaxAttempts. Spying onverifyMFAand usingAuthenticationErrorcodes matches the runtime logic inAuthManager. This gives good confidence in the retry semantics; no additional edge cases seem necessary right now.packages/tm-core/src/modules/auth/index.ts (1)
15-22: Public MFAVerificationResult export is appropriateRe-exporting
MFAVerificationResultalongside the existing auth types is consistent with exposingverifyMFAWithRetryas part of the public API and doesn’t introduce runtime changes. Looks good.apps/cli/src/commands/auth.command.ts (2)
520-548: MFA flow integrated correctly, skipMFAPrompt parameter intentionally unused.The MFA handling is well-implemented:
- Properly detects
MFA_REQUIREDerrors before showing failure message- Stops spinner before prompting (avoiding UI conflicts)
- Only shows "Authentication failed" for genuine failures
The
skipMFAPromptparameter is defined but never set totrueanywhere (line 645 calls it without arguments). Based on the previous review discussion, this is intentional—MFA codes are required to access the API even in non-interactive mode with--yes, making this the one exception where user input cannot be bypassed.
550-632: MFA verification properly bounded—previous recursion issue resolved.The unbounded recursive retry issue from the previous review (point 2) is now fixed. The new implementation:
- Delegates retry logic to
authManager.verifyMFAWithRetry(factorId, promptFn, 3)with a hard cap of 3 attempts- Properly separates concerns: CLI handles presentation (prompts, validation, cancellation), core handles retry orchestration
- Validates 6-digit format with clear error messages
- Handles user cancellation gracefully (lines 604-615)
The flow is clean and the error messages provide good context (including attempt count on line 629).
Crunchyman-ralph
force-pushed
the
ralph/fix/improve.auth.session
branch
from
ebdf636 to
5b5d497
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/cli/src/commands/auth.command.ts (1)
521-548: Token + MFA flow is sound; consider either using or droppingskipMFAPromptThe MFA-aware branch in
authenticateWithTokencorrectly:
- Detects
AuthenticationErrorwithcode === 'MFA_REQUIRED'before showing a failure message.- Stops the spinner cleanly and hands off to the MFA-specific flow when prompts are allowed.
- Falls back to a standard failure path (spinner.fail + rethrow) for all other errors.
The
skipMFAPromptparameter is currently never passed astrueby call sites (e.g.,performTokenAuthstill callsthis.authenticateWithToken(token)), so the!skipMFAPromptguard doesn’t change behavior today. To avoid confusion, you might either:
- Wire the caller’s
yesflag through, if you eventually want a non-interactive “fail-fast on MFA_REQUIRED” mode; or- Remove
skipMFAPromptfor now and reintroduce it when you have a concrete non-interactive scenario.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/cli/src/commands/auth.command.ts(7 hunks)packages/tm-core/src/modules/auth/managers/auth-manager.ts(8 hunks)packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
TypeScript test files must achieve minimum code coverage thresholds: 80% lines/functions and 70% branches globally, 90% for utilities, and 85% for middleware; new features must meet or exceed these thresholds
Files:
apps/cli/src/commands/auth.command.tspackages/tm-core/src/modules/auth/managers/auth-manager.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
**/*.{js,ts}: Import and use specific getters from config-manager.js (e.g., getMainProvider(), getLogLevel(), getMainMaxTokens()) to access configuration values needed for application logic
Use isApiKeySet(providerName, session) from config-manager.js to check if a provider's key is available before potentially attempting an AI call
Do not add direct console.log calls outside the logging utility - use the central log function instead
Ensure silent mode is disabled in a finally block to prevent it from staying enabled
Do not access the global silentMode variable directly - use the exported silent mode control functions instead
Do not duplicate task ID formatting logic across modules - centralize formatting utilities
Use ContextGatherer class from utils/contextGatherer.js for AI-powered commands that need project context, supporting tasks, files, custom text, and project tree context
Use FuzzyTaskSearch class from utils/fuzzyTaskSearch.js for automatic task relevance detection with configurable search parameters
Use fuzzy search to supplement user-provided task IDs and display discovered task IDs to users for transparency
Do not replace explicit user task selections with fuzzy results - fuzzy search should supplement, not replace user selections
Use readJSON and writeJSON utilities for all JSON file operations instead of raw fs.readFileSync or fs.writeFileSync
Include error handling for JSON file operations and validate JSON structure after reading
Use path.join() for cross-platform path construction and path.resolve() for absolute paths, validating paths before file operations
Support both .env files and MCP session environment for environment variable resolution with fallbacks for missing values
Prefer updating the core function to accept an outputFormat parameter and check outputFormat === 'json' before displaying UI elements
Files:
apps/cli/src/commands/auth.command.tspackages/tm-core/src/modules/auth/managers/auth-manager.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/git_workflow.mdc:0-0
Timestamp: 2025-11-24T18:00:23.000Z
Learning: Pull Request descriptions must include: Task Overview, Subtasks Completed (checklist), Implementation Details, Testing approach, Breaking Changes (if any), and Related Tasks.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/fix-tag-complexity-detection.md:0-0
Timestamp: 2025-08-02T15:33:22.656Z
Learning: For changeset files (.changeset/*.md), Crunchyman-ralph prefers to ignore formatting nitpicks about blank lines between frontmatter and descriptions, as he doesn't mind having them and wants to avoid such comments in future reviews.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1200
File: src/ai-providers/custom-sdk/grok-cli/language-model.js:96-100
Timestamp: 2025-09-19T16:06:42.182Z
Learning: The user Crunchyman-ralph prefers to keep environment variable names explicit (like GROK_CLI_API_KEY) rather than supporting multiple aliases, to avoid overlap and ensure clear separation between different CLI implementations.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1105
File: scripts/modules/supported-models.json:242-254
Timestamp: 2025-08-08T11:33:15.297Z
Learning: Preference: In scripts/modules/supported-models.json, the "name" field is optional. For OpenAI entries (e.g., "gpt-5"), Crunchyman-ralph prefers omitting "name" when the id is explicit enough; avoid nitpicks requesting a "name" in such cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1217
File: apps/cli/src/index.ts:16-21
Timestamp: 2025-09-18T16:35:35.147Z
Learning: The user Crunchyman-ralph considers suggestions to export types for better ergonomics (like exporting UpdateInfo type alongside related functions) as nitpicky and prefers not to implement such suggestions.
📚 Learning: 2025-09-09T19:14:21.367Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1194
File: .github/workflows/ci.yml:21-21
Timestamp: 2025-09-09T19:14:21.367Z
Learning: User Crunchyman-ralph considers suggestions about adding safer default shell options (like bash -euo pipefail) to GitHub Actions workflows as too nitpicky and prefers not to implement them.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-02T21:51:27.921Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-26T19:28:42.612Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: packages/ai-sdk-provider-grok-cli/package.json:21-35
Timestamp: 2025-09-26T19:28:42.612Z
Learning: Crunchyman-ralph is concerned about preventing production failures when developers add external dependencies to internal packages but forget to add them to the root package.json in their tsdown bundling setup. He wants automated safety mechanisms to catch this during development or CI rather than discovering it in production.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-03T12:45:30.724Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-10-15T14:43:40.410Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.410Z
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:
apps/cli/src/commands/auth.command.tspackages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:58:07.977Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-11-24T17:58:07.977Z
Learning: The Task Master CLI uses a modular architecture with distinct modules responsible for different aspects: commands.js (CLI command handling), task-manager.js (task data & core logic), dependency-manager.js (dependency management), ui.js (output formatting), ai-services-unified.js (unified AI service layer), config-manager.js (configuration management), utils.js (core utility functions), and mcp-server/ (MCP interface)
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:05:02.103Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: assets/.windsurfrules:0-0
Timestamp: 2025-11-24T18:05:02.103Z
Learning: Use the global CLI command `task-master` instead of `node scripts/dev.js` for all task management operations
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Place utilities used primarily by the core task-master CLI logic and command modules into scripts/modules/utils.js
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to scripts/commands/**/*.{js,ts} : Handle potential ConfigurationError if the .taskmasterconfig file is missing or invalid when accessed via getConfig
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:58:07.977Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-11-24T17:58:07.977Z
Learning: Applies to scripts/modules/config-manager.js : config-manager.js should load and validate .taskmasterconfig, provide getter functions (getMainProvider, getLogLevel, getDefaultSubtasks, etc.) for accessing settings, manage global.defaultTag and tags section for tag system settings, and NOT directly store or handle API keys
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:56:52.238Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: assets/GEMINI.md:0-0
Timestamp: 2025-11-24T17:56:52.238Z
Learning: Applies to assets/.gemini/settings.json : Configure Task Master MCP server in ~/.gemini/settings.json with the command 'npx' and args ['-y', 'task-master-ai']
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:58:07.977Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-11-24T17:58:07.977Z
Learning: Applies to scripts/modules/task-manager.js : task-manager.js should handle reading/writing tasks.json with tagged task lists support, implement CRUD operations, delegate AI interactions to ai-services-unified.js layer, and access non-AI configuration via config-manager.js getters
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to **/*.{js,ts} : Import and use specific getters from config-manager.js (e.g., getMainProvider(), getLogLevel(), getMainMaxTokens()) to access configuration values needed for application logic
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to **/*.{js,ts} : Use isApiKeySet(providerName, session) from config-manager.js to check if a provider's key is available before potentially attempting an AI call
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
🧬 Code graph analysis (1)
apps/cli/src/commands/auth.command.ts (2)
packages/tm-core/src/modules/auth/types.ts (2)
AuthenticationError(125-142)AuthCredentials(5-14)apps/cli/src/utils/error-handler.ts (1)
displayError(25-60)
⏰ 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 (5)
packages/tm-core/src/modules/auth/managers/auth-manager.ts (2)
31-52: SessionManager-centric session handling looks consistent and keeps AuthManager thin
AuthManagernow consistently delegates session lifecycle concerns (getAccessToken,getAuthCredentials,authenticateWithCode,refreshToken,logout,hasValidSession,getSession,getStoredContext) toSessionManager, andgetOrganizationServicevalidates a session viasessionManager.getSession()before instantiatingOrganizationService. This matches the intended pattern where Supabase + the session storage handle active tokens and the rest of the stack treats them as a single source of truth. I don’t see behavioral regressions here.
Based on learnings, this aligns with the passive credential store + Supabase-driven refresh model.Also applies to: 83-99, 207-238, 273-287
118-193: MFA verification and retry logic are bounded and error-selectiveThe new
verifyMFA/verifyMFAWithRetrypair cleanly isolates MFA from the rest of auth flows, and the retry loop is bounded (maxAttempts) and only retries onAuthenticationErrorwith code'INVALID_MFA_CODE', immediately surfacing any other error types. The final fallbackMFA_VERIFICATION_FAILEDresult covers the TS exhaustiveness case without affecting runtime behavior. This also addresses the earlier concern about unbounded recursive MFA retries higher up in the stack.apps/cli/src/commands/auth.command.ts (3)
69-79: Help text clearly documents MFA behavior with--yesThe added note that MFA prompts cannot be skipped even in
-y/--yesmode makes the non-interactive story much clearer and aligns with the earlier discussion about MFA being a hard requirement.
118-129: CentralizedhandleAuthErrorkeeps command error paths consistentRouting the top-level command catch blocks through
handleAuthErrorstandardizes how authentication failures are surfaced while still leaning ondisplayErrorfor actual rendering/exit behavior. The current implementation treatsAuthenticationErrorand genericErroridentically, which is fine and keeps things simple givendisplayErroralready handles structured vs generic errors.Also applies to: 150-152, 166-168, 178-180, 194-196
551-633: MFA verification UI cleanly delegates retries to core and handles cancellation
handleMFAVerificationchecks formfaChallengepresence, prints clear MFA instructions, and then usesauthManager.verifyMFAWithRetryas the single place that enforces attempt limits (3) and error selection (INVALID_MFA_CODEonly). The inquirer prompt enforces a strict 6‑digit code, and user cancellation yields anAuthenticationError('MFA_VERIFICATION_FAILED')after a warning, which appropriately short‑circuits the retry loop. Overall this replaces the prior unbounded recursive retry pattern with a bounded, core-driven flow and keeps CLI code focused on UX.
Crunchyman-ralph
force-pushed
the
ralph/fix/improve.auth.session
branch
from
5b5d497 to
c9cf321
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts (3)
129-161: Consider verifying specific error codes in error assertions.The error tests currently only check that
AuthenticationErroris thrown, but don't verify the specific error code. According to the implementation insupabase-client.ts,verifyMFAcan throw different error codes (MFA_VERIFICATION_FAILED,INVALID_RESPONSE,INVALID_MFA_CODE).Consider enhancing assertions to verify error codes:
it('should throw AuthenticationError when MFA challenge fails', async () => { const mockChallenge = { data: null, error: { message: 'Challenge failed' } }; mockSupabaseClient.auth.mfa.challenge.mockResolvedValue(mockChallenge); (authClient as any).client = mockSupabaseClient; - await expect( - authClient.verifyMFA('factor-123', '123456') - ).rejects.toThrow(AuthenticationError); + const error = await authClient + .verifyMFA('factor-123', '123456') + .catch((e) => e); + expect(error).toBeInstanceOf(AuthenticationError); + expect(error.code).toBe('MFA_VERIFICATION_FAILED'); });Apply similar changes to the "MFA verification fails" test to check for
INVALID_MFA_CODE.
230-378: Add error path coverage forcheckMFARequired.The test suite currently covers three success scenarios but lacks error path tests. According to the implementation in
supabase-client.ts,checkMFARequiredcan encounter errors fromgetSession(),getAuthenticatorAssuranceLevel(), orlistFactors(), and returns{ required: false }as a fallback.Consider adding test cases for these error scenarios:
it('should return required: false when getSession fails', async () => { const mockGetSessionResponse = { data: { session: null }, error: { message: 'Session error' } }; mockSupabaseClient.auth.getSession.mockResolvedValue( mockGetSessionResponse ); (authClient as any).client = mockSupabaseClient; const result = await authClient.checkMFARequired(); expect(result).toEqual({ required: false }); }); it('should return required: false when getAuthenticatorAssuranceLevel fails', async () => { const mockSession = { /* ... valid session ... */ }; const mockGetSessionResponse = { data: { session: mockSession }, error: null }; const mockAALResponse = { data: null, error: { message: 'AAL error' } }; mockSupabaseClient.auth.getSession.mockResolvedValue(mockGetSessionResponse); mockSupabaseClient.auth.mfa.getAuthenticatorAssuranceLevel.mockResolvedValue( mockAALResponse ); (authClient as any).client = mockSupabaseClient; const result = await authClient.checkMFARequired(); expect(result).toEqual({ required: false }); }); it('should return required: false when listFactors fails', async () => { // Similar setup with listFactors returning error });
380-395: Add error handling test forsignOut.The test suite verifies the happy path but lacks coverage for error scenarios. Consider adding a test for when
signOutfails:it('should handle signOut errors gracefully', async () => { const mockSignOutResponse = { error: { message: 'Sign out failed' } }; mockSupabaseClient.auth.signOut.mockResolvedValue(mockSignOutResponse); (authClient as any).client = mockSupabaseClient; await expect(authClient.signOut()).rejects.toThrow(); });packages/tm-core/src/modules/auth/managers/auth-manager.ts (1)
154-154: Consider importingMFAVerificationResultat the top for consistency.The inline type import
import('../types.js').MFAVerificationResultis valid but less readable than a top-level import. Consider adding it to the existing import block at lines 16-28 alongside other types from../types.js.import { AuthConfig, AuthCredentials, AuthenticationError, + MFAVerificationResult, OAuthFlowOptions, UserContext, UserContextWithBrief } from '../types.js';Then update line 154:
-): Promise<import('../types.js').MFAVerificationResult> { +): Promise<MFAVerificationResult> {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CLAUDE.md(2 hunks)apps/cli/src/commands/auth.command.ts(7 hunks)packages/tm-core/src/modules/auth/managers/auth-manager.ts(8 hunks)packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CLAUDE.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
TypeScript test files must achieve minimum code coverage thresholds: 80% lines/functions and 70% branches globally, 90% for utilities, and 85% for middleware; new features must meet or exceed these thresholds
Files:
packages/tm-core/src/modules/auth/managers/auth-manager.tsapps/cli/src/commands/auth.command.tspackages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
**/*.{js,ts}: Import and use specific getters from config-manager.js (e.g., getMainProvider(), getLogLevel(), getMainMaxTokens()) to access configuration values needed for application logic
Use isApiKeySet(providerName, session) from config-manager.js to check if a provider's key is available before potentially attempting an AI call
Do not add direct console.log calls outside the logging utility - use the central log function instead
Ensure silent mode is disabled in a finally block to prevent it from staying enabled
Do not access the global silentMode variable directly - use the exported silent mode control functions instead
Do not duplicate task ID formatting logic across modules - centralize formatting utilities
Use ContextGatherer class from utils/contextGatherer.js for AI-powered commands that need project context, supporting tasks, files, custom text, and project tree context
Use FuzzyTaskSearch class from utils/fuzzyTaskSearch.js for automatic task relevance detection with configurable search parameters
Use fuzzy search to supplement user-provided task IDs and display discovered task IDs to users for transparency
Do not replace explicit user task selections with fuzzy results - fuzzy search should supplement, not replace user selections
Use readJSON and writeJSON utilities for all JSON file operations instead of raw fs.readFileSync or fs.writeFileSync
Include error handling for JSON file operations and validate JSON structure after reading
Use path.join() for cross-platform path construction and path.resolve() for absolute paths, validating paths before file operations
Support both .env files and MCP session environment for environment variable resolution with fallbacks for missing values
Prefer updating the core function to accept an outputFormat parameter and check outputFormat === 'json' before displaying UI elements
Files:
packages/tm-core/src/modules/auth/managers/auth-manager.tsapps/cli/src/commands/auth.command.tspackages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
**/*.{spec,test}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{spec,test}.ts: Place package tests inpackages/<package-name>/src/<module>/<file>.spec.tsor apps inapps/<app-name>/src/<module>/<file>.spec.tsalongside source, or usepackages/<package-name>/tests/integration/<module>/<file>.test.tsfor integration tests
Always use.tsfor TypeScript tests, never.js
NEVER use async/await in test functions unless testing actual asynchronous operations; use synchronous top-level imports instead of dynamicawait import()
Write tests for bug fixes (add regression test), business logic (complex calculations, validations, transformations), edge cases (boundary conditions, error handling), public APIs, and integration points; skip tests for simple getters/setters, trivial pass-through functions, pure configuration objects, and code that just delegates to another tested function
Files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
🧠 Learnings (27)
📓 Common learnings
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/git_workflow.mdc:0-0
Timestamp: 2025-11-24T18:00:23.000Z
Learning: Pull Request descriptions must include: Task Overview, Subtasks Completed (checklist), Implementation Details, Testing approach, Breaking Changes (if any), and Related Tasks.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/fix-tag-complexity-detection.md:0-0
Timestamp: 2025-08-02T15:33:22.656Z
Learning: For changeset files (.changeset/*.md), Crunchyman-ralph prefers to ignore formatting nitpicks about blank lines between frontmatter and descriptions, as he doesn't mind having them and wants to avoid such comments in future reviews.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1200
File: src/ai-providers/custom-sdk/grok-cli/language-model.js:96-100
Timestamp: 2025-09-19T16:06:42.182Z
Learning: The user Crunchyman-ralph prefers to keep environment variable names explicit (like GROK_CLI_API_KEY) rather than supporting multiple aliases, to avoid overlap and ensure clear separation between different CLI implementations.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1105
File: scripts/modules/supported-models.json:242-254
Timestamp: 2025-08-08T11:33:15.297Z
Learning: Preference: In scripts/modules/supported-models.json, the "name" field is optional. For OpenAI entries (e.g., "gpt-5"), Crunchyman-ralph prefers omitting "name" when the id is explicit enough; avoid nitpicks requesting a "name" in such cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1217
File: apps/cli/src/index.ts:16-21
Timestamp: 2025-09-18T16:35:35.147Z
Learning: The user Crunchyman-ralph considers suggestions to export types for better ergonomics (like exporting UpdateInfo type alongside related functions) as nitpicky and prefers not to implement such suggestions.
📚 Learning: 2025-11-24T17:58:07.977Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-11-24T17:58:07.977Z
Learning: The Task Master CLI uses a modular architecture with distinct modules responsible for different aspects: commands.js (CLI command handling), task-manager.js (task data & core logic), dependency-manager.js (dependency management), ui.js (output formatting), ai-services-unified.js (unified AI service layer), config-manager.js (configuration management), utils.js (core utility functions), and mcp-server/ (MCP interface)
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:05:02.103Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: assets/.windsurfrules:0-0
Timestamp: 2025-11-24T18:05:02.103Z
Learning: Use the global CLI command `task-master` instead of `node scripts/dev.js` for all task management operations
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Place utilities used primarily by the core task-master CLI logic and command modules into scripts/modules/utils.js
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to scripts/commands/**/*.{js,ts} : Handle potential ConfigurationError if the .taskmasterconfig file is missing or invalid when accessed via getConfig
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:58:07.977Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-11-24T17:58:07.977Z
Learning: Applies to scripts/modules/config-manager.js : config-manager.js should load and validate .taskmasterconfig, provide getter functions (getMainProvider, getLogLevel, getDefaultSubtasks, etc.) for accessing settings, manage global.defaultTag and tags section for tag system settings, and NOT directly store or handle API keys
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:56:52.238Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: assets/GEMINI.md:0-0
Timestamp: 2025-11-24T17:56:52.238Z
Learning: Applies to assets/.gemini/settings.json : Configure Task Master MCP server in ~/.gemini/settings.json with the command 'npx' and args ['-y', 'task-master-ai']
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:58:07.977Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-11-24T17:58:07.977Z
Learning: Applies to scripts/modules/task-manager.js : task-manager.js should handle reading/writing tasks.json with tagged task lists support, implement CRUD operations, delegate AI interactions to ai-services-unified.js layer, and access non-AI configuration via config-manager.js getters
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to **/*.{js,ts} : Import and use specific getters from config-manager.js (e.g., getMainProvider(), getLogLevel(), getMainMaxTokens()) to access configuration values needed for application logic
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-10-15T14:43:40.410Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.410Z
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/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to **/*.{js,ts} : Use isApiKeySet(providerName, session) from config-manager.js to check if a provider's key is available before potentially attempting an AI call
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-09-09T19:14:21.367Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1194
File: .github/workflows/ci.yml:21-21
Timestamp: 2025-09-09T19:14:21.367Z
Learning: User Crunchyman-ralph considers suggestions about adding safer default shell options (like bash -euo pipefail) to GitHub Actions workflows as too nitpicky and prefers not to implement them.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-26T19:28:42.612Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: packages/ai-sdk-provider-grok-cli/package.json:21-35
Timestamp: 2025-09-26T19:28:42.612Z
Learning: Crunchyman-ralph is concerned about preventing production failures when developers add external dependencies to internal packages but forget to add them to the root package.json in their tsdown bundling setup. He wants automated safety mechanisms to catch this during development or CI rather than discovering it in production.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-02T21:51:27.921Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-03T12:45:30.724Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to **/*.integration.test.ts : Integration tests must use supertest for API endpoint testing, verify database state changes after operations, clean test data before each test, and include full request/response validation with expected HTTP status codes
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Use established mocking patterns from auth.test.ts as templates: mock bcrypt with proper TypeScript typing, mock Prisma client with transaction support, and always clear mocks between tests
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T17:56:37.308Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:37.308Z
Learning: Applies to **/*.{spec,test}.ts : Write tests for bug fixes (add regression test), business logic (complex calculations, validations, transformations), edge cases (boundary conditions, error handling), public APIs, and integration points; skip tests for simple getters/setters, trivial pass-through functions, pure configuration objects, and code that just delegates to another tested function
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to src/**/*.ts : Source code must have corresponding test files either colocated (*.test.ts) or in tests/unit/ directory following established patterns from src/utils/auth.test.ts with proper mocking for external dependencies
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to tests/fixtures/**/*.ts : Test fixture files must export reusable test data creators and constants (createTestUser, adminUser, invalidUser, etc.) for use across unit, integration, and e2e tests to ensure consistency
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:46.700Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-11-24T18:03:46.700Z
Learning: Applies to **/*.test.js : Do not import or instantiate real AI service clients. Create fully mocked versions that return predictable responses. Mock the entire module with controlled behavior.
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T17:56:37.308Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:37.308Z
Learning: Applies to **/*.{spec,test}.ts : Place package tests in `packages/<package-name>/src/<module>/<file>.spec.ts` or apps in `apps/<app-name>/src/<module>/<file>.spec.ts` alongside source, or use `packages/<package-name>/tests/integration/<module>/<file>.test.ts` for integration tests
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to tests/e2e/**/*.test.ts : End-to-end tests must test complete user workflows across multiple API endpoints in sequence, verify state changes between workflow steps, use extended timeouts (30000ms), and validate final outcomes without mocking business logic
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:46.699Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-11-24T18:03:46.699Z
Learning: Applies to tests/integration/**/*.test.js : Locate integration tests in `tests/integration/` directory. Test interactions between modules, focus on component interfaces rather than implementation details, use more realistic but controlled test environments.
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T17:57:14.728Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/ai_providers.mdc:0-0
Timestamp: 2025-11-24T17:57:14.728Z
Learning: Applies to tests/unit/ai-providers/*.test.js : Create unit tests in `tests/unit/ai-providers/<provider-name>.test.js` that mock the provider's AI SDK module and test each exported function for correct client instantiation, parameter passing, result handling, and error handling
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to **/*.test.ts : Unit tests must follow the describe/it pattern, use beforeEach for mock setup with jest.clearAllMocks(), include specific assertions with expect(), and test both success and error scenarios including edge cases
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:01:44.137Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-11-24T18:01:44.137Z
Learning: Applies to **/*.test.{js,ts} : Follow the mock-first-then-import pattern for Jest mocking; use jest.spyOn() for spy functions; clear mocks between tests; verify mocks with the pattern described in `tests.mdc`
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
🧬 Code graph analysis (3)
packages/tm-core/src/modules/auth/managers/auth-manager.ts (3)
packages/tm-core/src/modules/auth/services/context-store.ts (1)
ContextStore(30-181)packages/tm-core/src/modules/auth/services/session-manager.ts (1)
SessionManager(18-390)packages/tm-core/src/modules/auth/types.ts (2)
AuthCredentials(5-14)AuthenticationError(125-142)
apps/cli/src/commands/auth.command.ts (4)
packages/tm-core/src/modules/auth/index.ts (2)
AuthenticationError(24-24)AuthCredentials(16-16)packages/tm-core/src/modules/auth/types.ts (2)
AuthenticationError(125-142)AuthCredentials(5-14)packages/tm-core/src/index.ts (2)
AuthenticationError(83-83)AuthCredentials(79-79)apps/cli/src/utils/error-handler.ts (1)
displayError(25-60)
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts (1)
packages/tm-core/src/modules/auth/types.ts (1)
AuthenticationError(125-142)
⏰ 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/modules/integration/clients/supabase-client.spec.ts (1)
1-68: LGTM! Test setup follows best practices.The mock setup and test structure are well-organized:
- Mocks are declared before imports (correct Vitest pattern)
beforeEachproperly sets environment variables and clears mocks- Mock Supabase client includes comprehensive MFA method coverage
- Async/await usage is appropriate for testing actual asynchronous operations
packages/tm-core/src/modules/auth/managers/auth-manager.ts (3)
45-50: LGTM! Clean architectural delegation to SessionManager.The SessionManager initialization properly encapsulates session lifecycle concerns, establishing a clear separation between coordination (AuthManager) and session management (SessionManager).
150-193: LGTM! Properly addresses unbounded retry concern.The
verifyMFAWithRetry()implementation correctly uses a loop with a hard cap (maxAttempts = 3) instead of unbounded recursion. Error discrimination ensures onlyINVALID_MFA_CODEerrors trigger retries, while other errors fail immediately. The attempt tracking and result structure provide clear feedback to callers.Based on learnings, this addresses the concern raised in previous reviews about unbounded recursive MFA retries.
87-98: LGTM! Clean delegation pattern throughout.The AuthManager now properly delegates session lifecycle operations to SessionManager, maintaining a clear separation of concerns. All async operations are correctly awaited, and the delegation is consistent across methods.
Also applies to: 207-238
apps/cli/src/commands/auth.command.ts (3)
77-77: LGTM! Clearly documents the MFA exception.The help text appropriately informs users that MFA prompts cannot be skipped even with
--yes, setting correct expectations for CI/CD scenarios where MFA is enabled. Based on learnings, this addresses the discussion from previous reviews where Crunchyman-ralph confirmed MFA genuinely requires user input.
529-545: LGTM! Proper MFA detection with good UX consideration.The implementation correctly distinguishes between MFA being required (which is not a failure) and actual authentication failures. Stopping the spinner without showing a failure message before entering the MFA flow provides a better user experience.
547-629: LGTM! Well-structured MFA verification with proper separation of concerns.The
handleMFAVerification()method properly orchestrates the MFA flow:
- Validates MFA challenge information presence
- Provides clear user messaging about MFA requirements
- Implements proper input validation (6-digit code via regex
/^\d{6}$/)- Handles user cancellation gracefully
- Delegates retry logic to
authManager.verifyMFAWithRetry(), maintaining clean separation between presentation (CLI prompts) and business logic (retry handling)The implementation correctly surfaces success with credentials or failure with attempt details.
Crunchyman-ralph
force-pushed
the
ralph/fix/improve.auth.session
branch
from
c9cf321 to
5eb9267
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts (3)
8-35: Logger and storage mocks correctly isolate auth client behaviorThe logger and SupabaseSessionStorage mocks cleanly avoid side effects and keep tests deterministic. If you ever need to assert that signOut clears storage, you could swap the SupabaseSessionStorage methods to
vi.fn()implementations, but the current minimal mock is fine for these specs.Based on learnings, central mocking of external services is preferred over real instantiation.
70-232: verifyMFA tests cover key paths; consider asserting error codes and INVALID_RESPONSE pathThe
verifyMFAsuite exercises the happy path plus challenge failure, verification failure, refresh failure, and “no session” after refresh, which maps well onto the implementation’s branches. Two small improvements you might consider:
- In the “refreshSession fails” case (Lines 167-196), also assert
error.code === 'REFRESH_FAILED'to lock in the specific error contract.- Add a test where
mfa.verifyreturns{ data: null, error: null }to hit the'INVALID_RESPONSE'branch and ensure that scenario stays correctly handled.
234-486: checkMFARequired tests are comprehensive; duplication could move into a small helperThese tests nicely cover AAL1 vs AAL2, presence/absence of verified factors, and each of the early-return error conditions from
getSession,getAuthenticatorAssuranceLevel, andlistFactors, which gives strong confidence in the MFA-required decision logic. The only minor concern is repeated construction of nearly identicalSessionobjects; if this pattern grows, consider a tinycreateMockSession()helper or shared fixture to reduce duplication and keep future changes to the session shape centralized.Based on learnings, reusable fixtures are encouraged when similar test data appears in many tests.
CLAUDE.md (1)
175-179: Clarify the turbo:typecheck instruction slightlyTo keep this bullet as actionable as the others, consider tightening the wording, e.g.:
- - npm run turbo:typecheck to typecheck + - Run `npm run turbo:typecheck` before pushing to ensure TypeScript type checks passThat makes it explicit when and why to run it.
apps/cli/src/commands/auth.command.ts (1)
118-129: Centralized auth error handling is good; preserve details for unknown valuesRouting all top-level auth errors through
handleAuthErrorsimplifies the subcommand implementations and ensures everything flows viadisplayError. One small improvement: for non‑Error values you currently replace the original with a generic message, which can hide useful detail. You could keep the value by doing:- private handleAuthError(error: unknown): void { - if (error instanceof AuthenticationError) { - displayError(error); - } else if (error instanceof Error) { - displayError(error); - } else { - displayError(new Error('An unknown authentication error occurred')); - } - } + private handleAuthError(error: unknown): void { + if (error instanceof Error) { + displayError(error); + } else { + displayError(new Error(String(error ?? 'An unknown authentication error occurred'))); + } + }This keeps type safety while preserving unexpected error details.
Also applies to: 150-152, 167-168, 179-180, 195-196
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CLAUDE.md(2 hunks)apps/cli/src/commands/auth.command.ts(7 hunks)packages/tm-bridge/src/bridge-types.ts(2 hunks)packages/tm-core/src/modules/auth/managers/auth-manager.ts(7 hunks)packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
TypeScript test files must achieve minimum code coverage thresholds: 80% lines/functions and 70% branches globally, 90% for utilities, and 85% for middleware; new features must meet or exceed these thresholds
Files:
apps/cli/src/commands/auth.command.tspackages/tm-core/src/modules/integration/clients/supabase-client.spec.tspackages/tm-bridge/src/bridge-types.tspackages/tm-core/src/modules/auth/managers/auth-manager.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
**/*.{js,ts}: Import and use specific getters from config-manager.js (e.g., getMainProvider(), getLogLevel(), getMainMaxTokens()) to access configuration values needed for application logic
Use isApiKeySet(providerName, session) from config-manager.js to check if a provider's key is available before potentially attempting an AI call
Do not add direct console.log calls outside the logging utility - use the central log function instead
Ensure silent mode is disabled in a finally block to prevent it from staying enabled
Do not access the global silentMode variable directly - use the exported silent mode control functions instead
Do not duplicate task ID formatting logic across modules - centralize formatting utilities
Use ContextGatherer class from utils/contextGatherer.js for AI-powered commands that need project context, supporting tasks, files, custom text, and project tree context
Use FuzzyTaskSearch class from utils/fuzzyTaskSearch.js for automatic task relevance detection with configurable search parameters
Use fuzzy search to supplement user-provided task IDs and display discovered task IDs to users for transparency
Do not replace explicit user task selections with fuzzy results - fuzzy search should supplement, not replace user selections
Use readJSON and writeJSON utilities for all JSON file operations instead of raw fs.readFileSync or fs.writeFileSync
Include error handling for JSON file operations and validate JSON structure after reading
Use path.join() for cross-platform path construction and path.resolve() for absolute paths, validating paths before file operations
Support both .env files and MCP session environment for environment variable resolution with fallbacks for missing values
Prefer updating the core function to accept an outputFormat parameter and check outputFormat === 'json' before displaying UI elements
Files:
apps/cli/src/commands/auth.command.tspackages/tm-core/src/modules/integration/clients/supabase-client.spec.tspackages/tm-bridge/src/bridge-types.tspackages/tm-core/src/modules/auth/managers/auth-manager.ts
**/*.{spec,test}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{spec,test}.ts: Place package tests inpackages/<package-name>/src/<module>/<file>.spec.tsor apps inapps/<app-name>/src/<module>/<file>.spec.tsalongside source, or usepackages/<package-name>/tests/integration/<module>/<file>.test.tsfor integration tests
Always use.tsfor TypeScript tests, never.js
NEVER use async/await in test functions unless testing actual asynchronous operations; use synchronous top-level imports instead of dynamicawait import()
Write tests for bug fixes (add regression test), business logic (complex calculations, validations, transformations), edge cases (boundary conditions, error handling), public APIs, and integration points; skip tests for simple getters/setters, trivial pass-through functions, pure configuration objects, and code that just delegates to another tested function
Files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
**/*.md
📄 CodeRabbit inference engine (.cursor/rules/ai_providers.mdc)
Update relevant documentation (like
README.md) mentioning supported providers or configuration when adding a new AI provider
Files:
CLAUDE.md
🧠 Learnings (53)
📓 Common learnings
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/git_workflow.mdc:0-0
Timestamp: 2025-11-24T18:00:23.000Z
Learning: Pull Request descriptions must include: Task Overview, Subtasks Completed (checklist), Implementation Details, Testing approach, Breaking Changes (if any), and Related Tasks.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/fix-tag-complexity-detection.md:0-0
Timestamp: 2025-08-02T15:33:22.656Z
Learning: For changeset files (.changeset/*.md), Crunchyman-ralph prefers to ignore formatting nitpicks about blank lines between frontmatter and descriptions, as he doesn't mind having them and wants to avoid such comments in future reviews.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1200
File: src/ai-providers/custom-sdk/grok-cli/language-model.js:96-100
Timestamp: 2025-09-19T16:06:42.182Z
Learning: The user Crunchyman-ralph prefers to keep environment variable names explicit (like GROK_CLI_API_KEY) rather than supporting multiple aliases, to avoid overlap and ensure clear separation between different CLI implementations.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1105
File: scripts/modules/supported-models.json:242-254
Timestamp: 2025-08-08T11:33:15.297Z
Learning: Preference: In scripts/modules/supported-models.json, the "name" field is optional. For OpenAI entries (e.g., "gpt-5"), Crunchyman-ralph prefers omitting "name" when the id is explicit enough; avoid nitpicks requesting a "name" in such cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1217
File: apps/cli/src/index.ts:16-21
Timestamp: 2025-09-18T16:35:35.147Z
Learning: The user Crunchyman-ralph considers suggestions to export types for better ergonomics (like exporting UpdateInfo type alongside related functions) as nitpicky and prefers not to implement such suggestions.
📚 Learning: 2025-09-09T19:14:21.367Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1194
File: .github/workflows/ci.yml:21-21
Timestamp: 2025-09-09T19:14:21.367Z
Learning: User Crunchyman-ralph considers suggestions about adding safer default shell options (like bash -euo pipefail) to GitHub Actions workflows as too nitpicky and prefers not to implement them.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-26T19:28:42.612Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: packages/ai-sdk-provider-grok-cli/package.json:21-35
Timestamp: 2025-09-26T19:28:42.612Z
Learning: Crunchyman-ralph is concerned about preventing production failures when developers add external dependencies to internal packages but forget to add them to the root package.json in their tsdown bundling setup. He wants automated safety mechanisms to catch this during development or CI rather than discovering it in production.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-02T21:51:27.921Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-03T12:45:30.724Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to **/*.integration.test.ts : Integration tests must use supertest for API endpoint testing, verify database state changes after operations, clean test data before each test, and include full request/response validation with expected HTTP status codes
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.tsCLAUDE.md
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Use established mocking patterns from auth.test.ts as templates: mock bcrypt with proper TypeScript typing, mock Prisma client with transaction support, and always clear mocks between tests
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:46.700Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-11-24T18:03:46.700Z
Learning: Applies to **/*.test.js : Do not import or instantiate real AI service clients. Create fully mocked versions that return predictable responses. Mock the entire module with controlled behavior.
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to tests/fixtures/**/*.ts : Test fixture files must export reusable test data creators and constants (createTestUser, adminUser, invalidUser, etc.) for use across unit, integration, and e2e tests to ensure consistency
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to src/**/*.ts : Source code must have corresponding test files either colocated (*.test.ts) or in tests/unit/ directory following established patterns from src/utils/auth.test.ts with proper mocking for external dependencies
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T17:56:37.308Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:37.308Z
Learning: Applies to **/*.{spec,test}.ts : Write tests for bug fixes (add regression test), business logic (complex calculations, validations, transformations), edge cases (boundary conditions, error handling), public APIs, and integration points; skip tests for simple getters/setters, trivial pass-through functions, pure configuration objects, and code that just delegates to another tested function
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.tsCLAUDE.md
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to tests/e2e/**/*.test.ts : End-to-end tests must test complete user workflows across multiple API endpoints in sequence, verify state changes between workflow steps, use extended timeouts (30000ms), and validate final outcomes without mocking business logic
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.tsCLAUDE.md
📚 Learning: 2025-11-24T17:57:14.728Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/ai_providers.mdc:0-0
Timestamp: 2025-11-24T17:57:14.728Z
Learning: Applies to tests/unit/ai-providers/*.test.js : Create unit tests in `tests/unit/ai-providers/<provider-name>.test.js` that mock the provider's AI SDK module and test each exported function for correct client instantiation, parameter passing, result handling, and error handling
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T17:56:37.308Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:37.308Z
Learning: Applies to **/*.{spec,test}.ts : Place package tests in `packages/<package-name>/src/<module>/<file>.spec.ts` or apps in `apps/<app-name>/src/<module>/<file>.spec.ts` alongside source, or use `packages/<package-name>/tests/integration/<module>/<file>.test.ts` for integration tests
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:01:44.137Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-11-24T18:01:44.137Z
Learning: Applies to **/*.test.{js,ts} : Test new features with both legacy and tagged task data formats; verify tag resolution behavior; test migration compatibility; ensure pre-migration projects are handled correctly
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to **/*.test.ts : Unit tests must follow the describe/it pattern, use beforeEach for mock setup with jest.clearAllMocks(), include specific assertions with expect(), and test both success and error scenarios including edge cases
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.tsCLAUDE.md
📚 Learning: 2025-09-26T19:05:47.555Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: packages/ai-sdk-provider-grok-cli/package.json:11-13
Timestamp: 2025-09-26T19:05:47.555Z
Learning: In the eyaltoledano/claude-task-master repository, internal tm/ packages use a specific export pattern where the "exports" field points to TypeScript source files (./src/index.ts) while "main" points to compiled output (./dist/index.js) and "types" points to source files (./src/index.ts). This pattern is used consistently across internal packages like tm/core and tm/ai-sdk-provider-grok-cli because they are consumed directly during build-time bundling with tsdown rather than being published as separate packages.
Applied to files:
packages/tm-bridge/src/bridge-types.ts
📚 Learning: 2025-09-26T19:10:32.906Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: tsconfig.json:22-28
Timestamp: 2025-09-26T19:10:32.906Z
Learning: In the eyaltoledano/claude-task-master repository, all internal tm/ package path mappings in tsconfig.json consistently point to TypeScript source files (e.g., "./packages/*/src/index.ts") rather than built JavaScript. This is intentional architecture because tsdown bundles internal packages directly from source during build time, eliminating the need for separate compilation of internal packages.
Applied to files:
packages/tm-bridge/src/bridge-types.ts
📚 Learning: 2025-10-08T19:57:00.982Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1282
File: packages/tm-core/src/utils/index.ts:16-34
Timestamp: 2025-10-08T19:57:00.982Z
Learning: For the tm-core package in the eyaltoledano/claude-task-master repository, the team prefers a minimal, need-based export strategy in index files rather than exposing all internal utilities. Exports should only be added when functions are actually consumed by other packages in the monorepo.
Applied to files:
packages/tm-bridge/src/bridge-types.ts
📚 Learning: 2025-09-26T19:03:33.225Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: package.json:130-132
Timestamp: 2025-09-26T19:03:33.225Z
Learning: In the eyaltoledano/claude-task-master repository, packages are bundled using tsdown during the build process, which means dependencies imported by the source code (including tm internal packages like tm/ai-sdk-provider-grok-cli) are included in the final bundle and don't need to be available as separate runtime dependencies, so they should remain as devDependencies rather than being moved to dependencies.
Applied to files:
packages/tm-bridge/src/bridge-types.ts
📚 Learning: 2025-09-03T12:16:15.866Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/package.json:13-64
Timestamp: 2025-09-03T12:16:15.866Z
Learning: For internal packages in the claude-task-master project, Crunchyman-ralph prefers pointing package.json "types" entries to src .ts files rather than dist .d.ts files for better developer experience (DX), as the packages are not being exported as SDKs.
Applied to files:
packages/tm-bridge/src/bridge-types.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to **/{utils,utilities,helpers}/**/*.{js,ts} : Support multiple log levels (debug, info, warn, error) with appropriate icons for different log levels and respect the configured log level
Applied to files:
packages/tm-bridge/src/bridge-types.ts
📚 Learning: 2025-11-24T18:00:32.587Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/glossary.mdc:0-0
Timestamp: 2025-11-24T18:00:32.587Z
Learning: Refer to changeset.mdc for guidelines on using Changesets (npm run changeset) to manage versioning and changelogs
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-06T21:13:05.240Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1091
File: assets/claude/agents/task-orchestrator.md:69-78
Timestamp: 2025-08-06T21:13:05.240Z
Learning: For Claude agent instruction files in .claude/agents/ and assets/claude/agents/ directories, ignore markdownlint issues (like MD031 blanks-around-fences, MD040 fenced-code-language, MD041 missing top-level heading) as these files follow Claude's specific agent format requirements rather than standard markdown documentation conventions.
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T18:03:46.699Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-11-24T18:03:46.699Z
Learning: Applies to **/*.test.js : Test quality guidelines: Write tests before implementing features (TDD). Test edge cases and error conditions, not just happy paths. Keep tests independent and isolated. Use descriptive test names. Maintain test fixtures separately. Aim for 80%+ code coverage with critical paths at 100%.
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to **/*.ts : TypeScript test files must achieve minimum code coverage thresholds: 80% lines/functions and 70% branches globally, 90% for utilities, and 85% for middleware; new features must meet or exceed these thresholds
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-02T14:54:52.216Z
Learnt from: eyaltoledano
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/floppy-news-buy.md:7-38
Timestamp: 2025-08-02T14:54:52.216Z
Learning: For major feature additions like new CLI commands, eyaltoledano prefers detailed changesets with comprehensive descriptions, usage examples, and feature explanations rather than minimal single-line summaries.
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T18:01:44.137Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-11-24T18:01:44.137Z
Learning: Every new feature must include comprehensive tests following guidelines in `tests.mdc`, including unit tests, integration tests, edge cases, and at least 80% code coverage
Applied to files:
CLAUDE.md
📚 Learning: 2025-08-06T21:11:37.259Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1091
File: .claude/agents/task-orchestrator.md:0-0
Timestamp: 2025-08-06T21:11:37.259Z
Learning: For files in .claude/agents/ and assets/claude/agents/ directories, ignore markdownlint issues like MD041 (missing top-level heading) as these are Claude Code agent instruction files that follow a specific format with YAML frontmatter and don't need to conform to standard markdown documentation conventions.
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T18:03:46.699Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-11-24T18:03:46.699Z
Learning: Applies to **/*.test.js : Follow Jest test file structure: 1) Imports, 2) Mock setup before importing modules under test, 3) Import modules after mocks, 4) Set up spies on mocked modules, 5) Describe suite with descriptive name, 6) Setup/teardown hooks, 7) Grouped tests for related functionality, 8) Individual test cases with clear descriptions using Arrange-Act-Assert pattern.
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T18:03:46.699Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-11-24T18:03:46.699Z
Learning: Applies to **/*.test.js : When testing CLI commands built with Commander.js: test command action handlers directly rather than mocking the entire Commander chain; create simplified test-specific implementations of command handlers; explicitly handle all options including defaults and shorthand flags; include null/undefined checks for optional parameters; use fixtures for consistent sample data.
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T18:03:46.699Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-11-24T18:03:46.699Z
Learning: Applies to **/*.test.js : When testing UI functions: mock console output and verify correct formatting; test conditional output logic; use `toContain()` or `toMatch()` rather than exact `toBe()` for strings with emojis or formatting; create separate tests for different behavior modes; test structure of formatted output rather than exact string matching.
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T18:01:44.137Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-11-24T18:01:44.137Z
Learning: Applies to **/*.test.{js,ts} : Follow the mock-first-then-import pattern for Jest mocking; use jest.spyOn() for spy functions; clear mocks between tests; verify mocks with the pattern described in `tests.mdc`
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T17:56:37.308Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:37.308Z
Learning: ALL business logic must live in `tm/core` (packages/tm-core/), not in presentation layers (tm/cli, tm/mcp, or apps/extension); presentation layers should be thin and only call tm-core methods
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T17:56:37.308Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:37.308Z
Learning: Add a changeset for code changes by running `npx changeset` after making code changes (not needed for docs-only PRs); changesets should be user-facing, describing what end-users get or what is fixed rather than implementation details
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T17:58:19.822Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/changeset.mdc:0-0
Timestamp: 2025-11-24T17:58:19.822Z
Learning: Run `npm run changeset` after staging a logical set of changes that should be communicated in the next release's `CHANGELOG.md` for new features, bug fixes, breaking changes, performance improvements, significant refactoring, user-facing documentation updates, dependency updates, or build/tooling changes
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T17:58:19.822Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/changeset.mdc:0-0
Timestamp: 2025-11-24T17:58:19.822Z
Learning: When running changeset, select the appropriate semantic version bump type: `Major` for breaking changes, `Minor` for new features, and `Patch` for bug fixes, performance improvements, user-facing documentation changes, significant refactoring, relevant dependency updates, or impactful build/tooling changes
Applied to files:
CLAUDE.md
📚 Learning: 2025-10-08T12:21:14.455Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1285
File: .changeset/nice-ways-hope.md:5-17
Timestamp: 2025-10-08T12:21:14.455Z
Learning: For changeset files (.changeset/*.md): The first line should be concise and in imperative mood, but the body after the first line can include as many user-facing details as desired (bullets, explanations, links, etc.) to provide context for the CHANGELOG.md.
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T17:58:19.822Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/changeset.mdc:0-0
Timestamp: 2025-11-24T17:58:19.822Z
Learning: Provide a concise, single-line changeset summary in imperative mood (e.g., 'Add feature X', 'Fix bug Y') that describes what changed from a user/consumer perspective, not implementation details
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T17:58:19.822Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/changeset.mdc:0-0
Timestamp: 2025-11-24T17:58:19.822Z
Learning: Do not create changesets for internal documentation updates in `.cursor/rules/`, trivial chores, non-impactful test updates, or local configuration changes that have no relevance to external consumers or public-facing documentation
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T17:58:19.822Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/changeset.mdc:0-0
Timestamp: 2025-11-24T17:58:19.822Z
Learning: Create and stage generated changeset files from the `.changeset/` directory (e.g., `.changeset/random-name.md`) along with relevant code changes in the same commit
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Before committing code, verify all tests pass with coverage using npm run test:coverage, run unit tests with npm run test:unit, and integration tests with npm run test:integration where applicable
Applied to files:
CLAUDE.md
📚 Learning: 2025-11-24T18:01:44.137Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-11-24T18:01:44.137Z
Learning: Run the full test suite with `npm test` when submitting new features to ensure nothing was broken
Applied to files:
CLAUDE.md
📚 Learning: 2025-10-15T14:43:40.410Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.410Z
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/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:58:07.977Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-11-24T17:58:07.977Z
Learning: The Task Master CLI uses a modular architecture with distinct modules responsible for different aspects: commands.js (CLI command handling), task-manager.js (task data & core logic), dependency-manager.js (dependency management), ui.js (output formatting), ai-services-unified.js (unified AI service layer), config-manager.js (configuration management), utils.js (core utility functions), and mcp-server/ (MCP interface)
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:05:02.103Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: assets/.windsurfrules:0-0
Timestamp: 2025-11-24T18:05:02.103Z
Learning: Use the global CLI command `task-master` instead of `node scripts/dev.js` for all task management operations
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Place utilities used primarily by the core task-master CLI logic and command modules into scripts/modules/utils.js
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to scripts/commands/**/*.{js,ts} : Handle potential ConfigurationError if the .taskmasterconfig file is missing or invalid when accessed via getConfig
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:58:07.977Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-11-24T17:58:07.977Z
Learning: Applies to scripts/modules/config-manager.js : config-manager.js should load and validate .taskmasterconfig, provide getter functions (getMainProvider, getLogLevel, getDefaultSubtasks, etc.) for accessing settings, manage global.defaultTag and tags section for tag system settings, and NOT directly store or handle API keys
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:56:52.238Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: assets/GEMINI.md:0-0
Timestamp: 2025-11-24T17:56:52.238Z
Learning: Applies to assets/.gemini/settings.json : Configure Task Master MCP server in ~/.gemini/settings.json with the command 'npx' and args ['-y', 'task-master-ai']
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:58:07.977Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-11-24T17:58:07.977Z
Learning: Applies to scripts/modules/task-manager.js : task-manager.js should handle reading/writing tasks.json with tagged task lists support, implement CRUD operations, delegate AI interactions to ai-services-unified.js layer, and access non-AI configuration via config-manager.js getters
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to **/*.{js,ts} : Import and use specific getters from config-manager.js (e.g., getMainProvider(), getLogLevel(), getMainMaxTokens()) to access configuration values needed for application logic
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to **/*.{js,ts} : Use isApiKeySet(providerName, session) from config-manager.js to check if a provider's key is available before potentially attempting an AI call
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
🧬 Code graph analysis (2)
apps/cli/src/commands/auth.command.ts (2)
packages/tm-core/src/modules/auth/types.ts (2)
AuthenticationError(125-142)AuthCredentials(5-14)apps/cli/src/utils/error-handler.ts (1)
displayError(25-60)
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts (2)
packages/tm-core/src/modules/integration/clients/supabase-client.ts (1)
SupabaseAuthClient(15-506)packages/tm-core/src/modules/auth/types.ts (1)
AuthenticationError(125-142)
⏰ 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 (9)
packages/tm-bridge/src/bridge-types.ts (2)
5-5: LGTM! Good refactoring to explicit import.Converting the inline import type to an explicit
import typestatement improves code maintainability and follows TypeScript best practices.
45-45: LGTM! Clean type reference.The type reference now uses the imported TmCore type, making the code cleaner and more maintainable without changing the semantics of the interface.
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts (1)
36-68: beforeEach setup gives each test a clean, controlled SupabaseAuthClient instanceRe-initializing
SupabaseAuthClient, rebuildingmockSupabaseClient, and clearing mocks inbeforeEachprovides good isolation between tests while letting you inject the mock client via the privateclientfield without touching real Supabase configuration.Based on learnings, this aligns with the expected describe/it + beforeEach test pattern.
CLAUDE.md (1)
90-116: Testing guidelines expansion is clear and consistentThe FIRST/AAA/Right‑BICEP framing plus the concrete “what to mock / never mock” rules are well scoped and match the existing testing rules from
tests.mdcandnew_features.mdc. This should make expectations for unit vs integration tests much clearer.
Based on learnings, all new features needing tests now have stronger guidance.packages/tm-core/src/modules/auth/managers/auth-manager.ts (2)
20-52: SessionManager integration and delegation look solidCentralizing session concerns into
SessionManagerand havingAuthManagerdelegategetAccessToken,getAuthCredentials,authenticateWithCode,refreshToken,logout,hasValidSession,getSession, andgetStoredContextkeeps this class as a thin coordinator, which matches the documented architecture. UsingsessionManager.getSession()to gategetOrganizationService()also enforces auth checks in a single place. This aligns with the “Supabase owns active token lifecycle, credential store is passive storage” pattern.
Based on learnings, this is consistent with the intended Supabase/credential-store design.Also applies to: 88-99, 115-117, 208-239, 275-278
119-194: MFA verification with bounded retries correctly replaces recursive flow
verifyMFAas a thin proxy intoSessionManager.verifyMFAplusverifyMFAWithRetryusing a simpleforloop withmaxAttemptsgives you:
- A hard cap on invalid MFA code retries.
- Clear separation between “invalid code, retryable” (
INVALID_MFA_CODE) and all other errors, which are surfaced immediately.- A structured
MFAVerificationResultthat the CLI can interpret without embedding business logic.This neatly solves the previous unbounded-recursion MFA retry concern while keeping the retry policy in tm-core rather than the CLI.
apps/cli/src/commands/auth.command.ts (3)
69-79: MFA note in help text resolves the--yesbehavior ambiguityCalling out explicitly that MFA prompts cannot be skipped even with
-y/--yesmatches the earlier discussion and sets the right expectation for CI/scripts using token auth.
521-543: Token auth MFA branching cleanly hands off to MFA flowSpecial‑casing
MFA_REQUIREDhere, stopping the spinner without failing it, and delegating tohandleMFAVerificationavoids showing a misleading “Authentication failed” when MFA is actually needed. Non‑MFA errors still surface via the spinner fail + rethrow path, which continues to behave as before.
547-629: CLI MFA flow is presentation-only and now relies on bounded core retries
handleMFAVerificationdoes a good job of:
- Validating that an
mfaChallengeexists before proceeding.- Providing clear user messaging and input validation for the 6‑digit code.
- Handling user cancellation explicitly and converting it into an
AuthenticationErrorthe rest of the flow understands.- Delegating all retry logic to
authManager.verifyMFAWithRetrywith a hardmaxAttemptscap, instead of implementing recursion or retry counters in the CLI.This keeps MFA behavior in tm-core (as required by the “all business logic in @tm/core” guideline) and leaves this file focused on UX and prompts only.
Based on learnings, this matches the intended separation between tm-core and CLI.
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
Show resolved
Hide resolved
Crunchyman-ralph
force-pushed
the
ralph/fix/improve.auth.session
branch
from
5eb9267 to
398888c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/tm-core/src/modules/auth/managers/auth-manager.ts (1)
119-194: MFA retry loop is well‑scoped; consider guarding pathologicalmaxAttemptsvaluesThe
verifyMFAWithRetryloop cleanly distinguishes retryableINVALID_MFA_CODEerrors from all otherAuthenticationErrors and returns structuredMFAVerificationResult, which is a solid fix over the previous unbounded recursion.One small edge case: if a caller ever passes
maxAttempts <= 0, the loop won’t run and you fall through to the syntheticMFA_VERIFICATION_FAILEDresult. It may be clearer to clamp to at least 1 attempt or throw on invalidmaxAttemptsup front, so misuse is caught early instead of silently changing behavior.apps/cli/src/commands/auth.command.ts (1)
547-629: MFA verification wrapper correctly delegates retries and handles cancellations
handleMFAVerificationvalidates the presence ofmfaChallenge.factorId, prints focused MFA instructions, and then:
- Uses
authManager.verifyMFAWithRetryrather than its own recursion, enforcing a hard cap on attempts.- Prompts with inquirer using strict 6‑digit validation, returning trimmed codes.
- Treats prompt cancellation (
ExitPromptError/ “force closed”) as a deliberateAuthenticationError('MFA_VERIFICATION_FAILED'), which the caller can surface cleanly.On success it returns credentials; otherwise it throws with an attempt‑counted failure message, which
performTokenAuthalready reports viadisplayError. This fully addresses the earlier unbounded‑retry concern in a clear separation of concerns between core and CLI.packages/tm-core/src/modules/integration/clients/supabase-client.ts (1)
361-436: MFA requirement check is robust; tweak doc comment for return type clarity
checkMFARequired:
- Validates there is a session.
- Short‑circuits if AAL is already
aal2.- Enumerates factors and looks for verified TOTP entries.
- Returns a
{ required: boolean, factorId?, factorType? }object that the rest of the stack can consume.This is a solid encapsulation. The leading comment still says “returns MFA factor information … null otherwise”, which no longer matches the actual return type; updating that to describe the
{ required: false }shape would avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CLAUDE.md(2 hunks)apps/cli/src/commands/auth.command.ts(7 hunks)packages/tm-bridge/src/bridge-types.ts(2 hunks)packages/tm-core/src/modules/auth/managers/auth-manager.ts(7 hunks)packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts(1 hunks)packages/tm-core/src/modules/integration/clients/supabase-client.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CLAUDE.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
TypeScript test files must achieve minimum code coverage thresholds: 80% lines/functions and 70% branches globally, 90% for utilities, and 85% for middleware; new features must meet or exceed these thresholds
Files:
packages/tm-core/src/modules/auth/managers/auth-manager.tsapps/cli/src/commands/auth.command.tspackages/tm-core/src/modules/integration/clients/supabase-client.spec.tspackages/tm-bridge/src/bridge-types.tspackages/tm-core/src/modules/integration/clients/supabase-client.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
**/*.{js,ts}: Import and use specific getters from config-manager.js (e.g., getMainProvider(), getLogLevel(), getMainMaxTokens()) to access configuration values needed for application logic
Use isApiKeySet(providerName, session) from config-manager.js to check if a provider's key is available before potentially attempting an AI call
Do not add direct console.log calls outside the logging utility - use the central log function instead
Ensure silent mode is disabled in a finally block to prevent it from staying enabled
Do not access the global silentMode variable directly - use the exported silent mode control functions instead
Do not duplicate task ID formatting logic across modules - centralize formatting utilities
Use ContextGatherer class from utils/contextGatherer.js for AI-powered commands that need project context, supporting tasks, files, custom text, and project tree context
Use FuzzyTaskSearch class from utils/fuzzyTaskSearch.js for automatic task relevance detection with configurable search parameters
Use fuzzy search to supplement user-provided task IDs and display discovered task IDs to users for transparency
Do not replace explicit user task selections with fuzzy results - fuzzy search should supplement, not replace user selections
Use readJSON and writeJSON utilities for all JSON file operations instead of raw fs.readFileSync or fs.writeFileSync
Include error handling for JSON file operations and validate JSON structure after reading
Use path.join() for cross-platform path construction and path.resolve() for absolute paths, validating paths before file operations
Support both .env files and MCP session environment for environment variable resolution with fallbacks for missing values
Prefer updating the core function to accept an outputFormat parameter and check outputFormat === 'json' before displaying UI elements
Files:
packages/tm-core/src/modules/auth/managers/auth-manager.tsapps/cli/src/commands/auth.command.tspackages/tm-core/src/modules/integration/clients/supabase-client.spec.tspackages/tm-bridge/src/bridge-types.tspackages/tm-core/src/modules/integration/clients/supabase-client.ts
**/*.{spec,test}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{spec,test}.ts: Place package tests inpackages/<package-name>/src/<module>/<file>.spec.tsor apps inapps/<app-name>/src/<module>/<file>.spec.tsalongside source, or usepackages/<package-name>/tests/integration/<module>/<file>.test.tsfor integration tests
Always use.tsfor TypeScript tests, never.js
NEVER use async/await in test functions unless testing actual asynchronous operations; use synchronous top-level imports instead of dynamicawait import()
Write tests for bug fixes (add regression test), business logic (complex calculations, validations, transformations), edge cases (boundary conditions, error handling), public APIs, and integration points; skip tests for simple getters/setters, trivial pass-through functions, pure configuration objects, and code that just delegates to another tested function
Files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
🧠 Learnings (32)
📓 Common learnings
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/git_workflow.mdc:0-0
Timestamp: 2025-11-24T18:00:23.000Z
Learning: Pull Request descriptions must include: Task Overview, Subtasks Completed (checklist), Implementation Details, Testing approach, Breaking Changes (if any), and Related Tasks.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1069
File: .changeset/fix-tag-complexity-detection.md:0-0
Timestamp: 2025-08-02T15:33:22.656Z
Learning: For changeset files (.changeset/*.md), Crunchyman-ralph prefers to ignore formatting nitpicks about blank lines between frontmatter and descriptions, as he doesn't mind having them and wants to avoid such comments in future reviews.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1132
File: .github/workflows/weekly-metrics-discord.yml:81-93
Timestamp: 2025-08-13T22:10:46.958Z
Learning: Crunchyman-ralph ignores YAML formatting nitpicks about trailing spaces when there's no project-specific YAML formatter configured, preferring to focus on functionality over cosmetic formatting issues.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1200
File: src/ai-providers/custom-sdk/grok-cli/language-model.js:96-100
Timestamp: 2025-09-19T16:06:42.182Z
Learning: The user Crunchyman-ralph prefers to keep environment variable names explicit (like GROK_CLI_API_KEY) rather than supporting multiple aliases, to avoid overlap and ensure clear separation between different CLI implementations.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1105
File: scripts/modules/supported-models.json:242-254
Timestamp: 2025-08-08T11:33:15.297Z
Learning: Preference: In scripts/modules/supported-models.json, the "name" field is optional. For OpenAI entries (e.g., "gpt-5"), Crunchyman-ralph prefers omitting "name" when the id is explicit enough; avoid nitpicks requesting a "name" in such cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1217
File: apps/cli/src/index.ts:16-21
Timestamp: 2025-09-18T16:35:35.147Z
Learning: The user Crunchyman-ralph considers suggestions to export types for better ergonomics (like exporting UpdateInfo type alongside related functions) as nitpicky and prefers not to implement such suggestions.
📚 Learning: 2025-10-15T14:43:40.410Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1314
File: packages/tm-core/src/auth/credential-store.ts:92-94
Timestamp: 2025-10-15T14:43:40.410Z
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/modules/auth/managers/auth-manager.tspackages/tm-core/src/modules/integration/clients/supabase-client.ts
📚 Learning: 2025-11-24T17:58:07.977Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-11-24T17:58:07.977Z
Learning: The Task Master CLI uses a modular architecture with distinct modules responsible for different aspects: commands.js (CLI command handling), task-manager.js (task data & core logic), dependency-manager.js (dependency management), ui.js (output formatting), ai-services-unified.js (unified AI service layer), config-manager.js (configuration management), utils.js (core utility functions), and mcp-server/ (MCP interface)
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:05:02.103Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: assets/.windsurfrules:0-0
Timestamp: 2025-11-24T18:05:02.103Z
Learning: Use the global CLI command `task-master` instead of `node scripts/dev.js` for all task management operations
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Place utilities used primarily by the core task-master CLI logic and command modules into scripts/modules/utils.js
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to scripts/commands/**/*.{js,ts} : Handle potential ConfigurationError if the .taskmasterconfig file is missing or invalid when accessed via getConfig
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:58:07.977Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-11-24T17:58:07.977Z
Learning: Applies to scripts/modules/config-manager.js : config-manager.js should load and validate .taskmasterconfig, provide getter functions (getMainProvider, getLogLevel, getDefaultSubtasks, etc.) for accessing settings, manage global.defaultTag and tags section for tag system settings, and NOT directly store or handle API keys
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:56:52.238Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: assets/GEMINI.md:0-0
Timestamp: 2025-11-24T17:56:52.238Z
Learning: Applies to assets/.gemini/settings.json : Configure Task Master MCP server in ~/.gemini/settings.json with the command 'npx' and args ['-y', 'task-master-ai']
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T17:58:07.977Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-11-24T17:58:07.977Z
Learning: Applies to scripts/modules/task-manager.js : task-manager.js should handle reading/writing tasks.json with tagged task lists support, implement CRUD operations, delegate AI interactions to ai-services-unified.js layer, and access non-AI configuration via config-manager.js getters
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to **/*.{js,ts} : Import and use specific getters from config-manager.js (e.g., getMainProvider(), getLogLevel(), getMainMaxTokens()) to access configuration values needed for application logic
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to **/*.{js,ts} : Use isApiKeySet(providerName, session) from config-manager.js to check if a provider's key is available before potentially attempting an AI call
Applied to files:
packages/tm-core/src/modules/auth/managers/auth-manager.ts
📚 Learning: 2025-09-09T19:14:21.367Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1194
File: .github/workflows/ci.yml:21-21
Timestamp: 2025-09-09T19:14:21.367Z
Learning: User Crunchyman-ralph considers suggestions about adding safer default shell options (like bash -euo pipefail) to GitHub Actions workflows as too nitpicky and prefers not to implement them.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-26T19:28:42.612Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: packages/ai-sdk-provider-grok-cli/package.json:21-35
Timestamp: 2025-09-26T19:28:42.612Z
Learning: Crunchyman-ralph is concerned about preventing production failures when developers add external dependencies to internal packages but forget to add them to the root package.json in their tsdown bundling setup. He wants automated safety mechanisms to catch this during development or CI rather than discovering it in production.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-02T21:51:27.921Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/auth/config.ts:5-7
Timestamp: 2025-09-02T21:51:27.921Z
Learning: The user Crunchyman-ralph prefers not to use node: scheme imports (e.g., 'node:os', 'node:path') for Node.js core modules and considers suggestions to change bare imports to node: scheme as too nitpicky.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-09-03T12:45:30.724Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/src/subpath-exports.test.ts:6-9
Timestamp: 2025-09-03T12:45:30.724Z
Learning: The user Crunchyman-ralph prefers to avoid overly nitpicky or detailed suggestions in code reviews, especially for test coverage of minor import paths. Focus on more substantial issues rather than comprehensive coverage of all possible edge cases.
Applied to files:
apps/cli/src/commands/auth.command.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to **/*.integration.test.ts : Integration tests must use supertest for API endpoint testing, verify database state changes after operations, clean test data before each test, and include full request/response validation with expected HTTP status codes
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Use established mocking patterns from auth.test.ts as templates: mock bcrypt with proper TypeScript typing, mock Prisma client with transaction support, and always clear mocks between tests
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:46.700Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-11-24T18:03:46.700Z
Learning: Applies to **/*.test.js : Do not import or instantiate real AI service clients. Create fully mocked versions that return predictable responses. Mock the entire module with controlled behavior.
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to tests/fixtures/**/*.ts : Test fixture files must export reusable test data creators and constants (createTestUser, adminUser, invalidUser, etc.) for use across unit, integration, and e2e tests to ensure consistency
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to src/**/*.ts : Source code must have corresponding test files either colocated (*.test.ts) or in tests/unit/ directory following established patterns from src/utils/auth.test.ts with proper mocking for external dependencies
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T17:56:37.308Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:37.308Z
Learning: Applies to **/*.{spec,test}.ts : Write tests for bug fixes (add regression test), business logic (complex calculations, validations, transformations), edge cases (boundary conditions, error handling), public APIs, and integration points; skip tests for simple getters/setters, trivial pass-through functions, pure configuration objects, and code that just delegates to another tested function
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to tests/e2e/**/*.test.ts : End-to-end tests must test complete user workflows across multiple API endpoints in sequence, verify state changes between workflow steps, use extended timeouts (30000ms), and validate final outcomes without mocking business logic
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T17:57:14.728Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/ai_providers.mdc:0-0
Timestamp: 2025-11-24T17:57:14.728Z
Learning: Applies to tests/unit/ai-providers/*.test.js : Create unit tests in `tests/unit/ai-providers/<provider-name>.test.js` that mock the provider's AI SDK module and test each exported function for correct client instantiation, parameter passing, result handling, and error handling
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T17:56:37.308Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:56:37.308Z
Learning: Applies to **/*.{spec,test}.ts : Place package tests in `packages/<package-name>/src/<module>/<file>.spec.ts` or apps in `apps/<app-name>/src/<module>/<file>.spec.ts` alongside source, or use `packages/<package-name>/tests/integration/<module>/<file>.test.ts` for integration tests
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:01:44.137Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-11-24T18:01:44.137Z
Learning: Applies to **/*.test.{js,ts} : Test new features with both legacy and tagged task data formats; verify tag resolution behavior; test migration compatibility; ensure pre-migration projects are handled correctly
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-11-24T18:03:13.408Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-11-24T18:03:13.408Z
Learning: Applies to **/*.test.ts : Unit tests must follow the describe/it pattern, use beforeEach for mock setup with jest.clearAllMocks(), include specific assertions with expect(), and test both success and error scenarios including edge cases
Applied to files:
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts
📚 Learning: 2025-09-26T19:05:47.555Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: packages/ai-sdk-provider-grok-cli/package.json:11-13
Timestamp: 2025-09-26T19:05:47.555Z
Learning: In the eyaltoledano/claude-task-master repository, internal tm/ packages use a specific export pattern where the "exports" field points to TypeScript source files (./src/index.ts) while "main" points to compiled output (./dist/index.js) and "types" points to source files (./src/index.ts). This pattern is used consistently across internal packages like tm/core and tm/ai-sdk-provider-grok-cli because they are consumed directly during build-time bundling with tsdown rather than being published as separate packages.
Applied to files:
packages/tm-bridge/src/bridge-types.ts
📚 Learning: 2025-09-26T19:10:32.906Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: tsconfig.json:22-28
Timestamp: 2025-09-26T19:10:32.906Z
Learning: In the eyaltoledano/claude-task-master repository, all internal tm/ package path mappings in tsconfig.json consistently point to TypeScript source files (e.g., "./packages/*/src/index.ts") rather than built JavaScript. This is intentional architecture because tsdown bundles internal packages directly from source during build time, eliminating the need for separate compilation of internal packages.
Applied to files:
packages/tm-bridge/src/bridge-types.ts
📚 Learning: 2025-10-08T19:57:00.982Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1282
File: packages/tm-core/src/utils/index.ts:16-34
Timestamp: 2025-10-08T19:57:00.982Z
Learning: For the tm-core package in the eyaltoledano/claude-task-master repository, the team prefers a minimal, need-based export strategy in index files rather than exposing all internal utilities. Exports should only be added when functions are actually consumed by other packages in the monorepo.
Applied to files:
packages/tm-bridge/src/bridge-types.ts
📚 Learning: 2025-09-26T19:03:33.225Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1252
File: package.json:130-132
Timestamp: 2025-09-26T19:03:33.225Z
Learning: In the eyaltoledano/claude-task-master repository, packages are bundled using tsdown during the build process, which means dependencies imported by the source code (including tm internal packages like tm/ai-sdk-provider-grok-cli) are included in the final bundle and don't need to be available as separate runtime dependencies, so they should remain as devDependencies rather than being moved to dependencies.
Applied to files:
packages/tm-bridge/src/bridge-types.ts
📚 Learning: 2025-09-03T12:16:15.866Z
Learnt from: Crunchyman-ralph
Repo: eyaltoledano/claude-task-master PR: 1178
File: packages/tm-core/package.json:13-64
Timestamp: 2025-09-03T12:16:15.866Z
Learning: For internal packages in the claude-task-master project, Crunchyman-ralph prefers pointing package.json "types" entries to src .ts files rather than dist .d.ts files for better developer experience (DX), as the packages are not being exported as SDKs.
Applied to files:
packages/tm-bridge/src/bridge-types.ts
📚 Learning: 2025-11-24T18:04:43.949Z
Learnt from: CR
Repo: eyaltoledano/claude-task-master PR: 0
File: .cursor/rules/utilities.mdc:0-0
Timestamp: 2025-11-24T18:04:43.949Z
Learning: Applies to **/{utils,utilities,helpers}/**/*.{js,ts} : Support multiple log levels (debug, info, warn, error) with appropriate icons for different log levels and respect the configured log level
Applied to files:
packages/tm-bridge/src/bridge-types.ts
🧬 Code graph analysis (2)
packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts (2)
packages/tm-core/src/modules/integration/clients/supabase-client.ts (1)
SupabaseAuthClient(15-506)packages/tm-core/src/modules/auth/types.ts (1)
AuthenticationError(125-142)
packages/tm-core/src/modules/integration/clients/supabase-client.ts (3)
packages/tm-core/src/modules/auth/index.ts (1)
AuthenticationError(24-24)packages/tm-core/src/modules/auth/types.ts (1)
AuthenticationError(125-142)packages/tm-core/src/index.ts (1)
AuthenticationError(83-83)
⏰ 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 (11)
packages/tm-bridge/src/bridge-types.ts (2)
5-5: LGTM! Clean type-only import.Using a top-level
import typestatement is cleaner and more maintainable than inline import types, following TypeScript best practices.
45-45: LGTM! Improved type reference readability.The direct type reference is cleaner than the inline import type while maintaining identical semantics. This refactoring improves code maintainability without any breaking changes.
packages/tm-core/src/modules/auth/managers/auth-manager.ts (2)
39-52: SessionManager wiring keeps auth concerns centralized; looks consistent
sessionManageris built from the sharedSupabaseAuthClientandContextStore, and all session/token helpers now delegate through it. This matches the intended “AuthManager as coordinator” role and avoids duplicating Supabase/session logic here.
274-287: Organization service now correctly enforces auth via SessionManagerUsing
this.sessionManager.getSession()as the gate before creatingOrganizationServicekeeps org/brief APIs aligned with the new session lifecycle and ensures “not authenticated” is surfaced viaAuthenticationError. No issues spotted.packages/tm-core/src/modules/integration/clients/supabase-client.spec.ts (1)
1-545: MFA and sign‑out tests are thorough and aligned with client behaviorThe spec exercises happy paths and key error branches for
verifyMFA,checkMFARequired, andsignOutusing well‑scoped mocks (logger, session storage, Supabase client). Assertions onAuthenticationError.codeandsignOutscope match the implementation, and the suite follows the project’s describe/it +beforeEach/vi.clearAllMocks()patterns.apps/cli/src/commands/auth.command.ts (3)
70-82: Login help now clearly documents the MFA exception to--yesThe added note under the
tm auth login <token> -yexample makes it explicit that MFA prompts cannot be skipped even in non‑interactive mode, which matches the product constraint discussed earlier.
118-197: Centralized auth error handling improves consistency across subcommands
handleAuthErrorsafely normalizes unknown throwables intoErrorinstances and is now used byexecuteLogin,executeLogout,executeStatus, andexecuteRefresh. This keeps CLI error display uniform and avoids duplicated try/catch logic without changing command semantics.
521-545: Token auth now cleanly branches into MFA flow without mislabeling it as failureCatching
AuthenticationErrorwith codeMFA_REQUIRED, stopping the spinner (instead of failing it), and delegating tohandleMFAVerificationavoids showing a misleading “Authentication failed” message when MFA is merely the next required step. Non‑MFA errors still surface as failed spinners and propagate correctly.packages/tm-core/src/modules/integration/clients/supabase-client.ts (3)
5-18: Type‑only Supabase imports are a safe, bundle‑friendly refinementSwitching
Session,SupabaseClient, andUsertotypeimports while keepingcreateClientas the sole runtime import is idiomatic and should reduce unnecessary bundling without changing behavior.
265-281: Local‑scope sign‑out aligns with tests and avoids over‑revoking sessionsUsing
client.auth.signOut({ scope: 'local' })and then clearingSupabaseSessionStorageensures the CLI only logs out the current device/session, matching the updated tests and avoiding unintended global revocation.
438-505: MFA verification correctly separates verification from session upgrade
verifyMFAnow:
- Creates a challenge, fails fast via
AuthenticationError('MFA_VERIFICATION_FAILED')when that fails.- Maps invalid codes to
INVALID_MFA_CODE(the only error treated as retryable upstream).- Treats missing data as
INVALID_RESPONSE.- Refreshes the session and throws
REFRESH_FAILEDif the upgraded session is missing or errors, otherwise returning the upgradedSession.This error‑code mapping matches what
AuthManager.verifyMFAWithRetryand the CLI expect, and the logic is straightforward and side‑effect‑contained.
Crunchyman-ralph
force-pushed
the
ralph/fix/improve.auth.session
branch
from
398888c to
f05eed7
Compare
This PR was automatically generated to update documentation based on recent changes. Original commit: feat: implement MFA in taskmaster (#1438)\n\n\n Co-authored-by: Claude <claude-assistant@anthropic.com>
1 participant
What type of PR is this?
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
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.