chore: address oauth PR concerns#1184
Conversation
|
|
Caution Review failedThe pull request is closed. WalkthroughImplements ESM path updates across modules, adds AuthManager singleton reset and token refresh, enhances CredentialStore with expiry normalization, corruption quarantine, and cleanup, updates config interfaces (RuntimeStorageConfig, StorageType) and ConfigManager API, introduces StorageFactory.createFromStorageConfig with auto storage detection and API credential merging, and adjusts exports/tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant AM as AuthManager
participant CS as CredentialStore
participant SB as SupabaseAuthClient
participant Log as Logger
rect rgb(240,248,255)
note right of AM: Token refresh flow
App->>AM: refreshToken()
AM->>CS: getCredentials({ allowExpired: true })
alt missing refreshToken
AM-->>App: throw AuthenticationError('REFRESH_FAILED')
else have refreshToken
AM->>SB: refreshSession(refreshToken)
alt success
SB-->>AM: { access_token, refresh_token, expires_at }
AM->>CS: saveCredentials(updated)
AM-->>App: updated credentials
else failure
AM->>Log: warn("Failed to refresh session")
AM-->>App: throw AuthenticationError('REFRESH_FAILED')
end
end
end
sequenceDiagram
autonumber
actor App
participant AM as AuthManager
participant SF as StorageFactory
participant CM as ConfigManager
participant Log as Logger
rect rgb(245,255,245)
note right of SF: Storage selection (auto/configured)
App->>CM: getStorageConfig()
CM-->>App: RuntimeStorageConfig
App->>SF: createFromStorageConfig(storageConfig, projectPath)
alt storage.type == 'api'
alt required fields present
SF-->>App: ApiStorage
else missing fields
SF-->>App: throw TaskMasterError(missing fields)
end
else storage.type == 'auto'
alt API explicitly configured
SF-->>App: ApiStorage (configured)
else AuthManager has credentials
SF->>AM: getInstance().getCredentials()
SF-->>App: ApiStorage (merged credentials)
else
SF-->>App: FileStorage
end
else storage.type == 'file'
SF-->>App: FileStorage
end
end
sequenceDiagram
autonumber
actor App
participant AM as AuthManager
participant Log as Logger
rect rgb(255,250,240)
note right of AM: Singleton lifecycle
App->>AM: getInstance(configA)
AM-->>App: instance A
App->>AM: getInstance(configB)
alt already initialized and configB provided
AM->>Log: warn("Config ignored after initialization")
end
AM-->>App: same instance A
App->>AM: resetInstance()
AM-->>App: void
App->>AM: getInstance(configB)
AM-->>App: new instance B
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (16)
✨ Finishing Touches
🧪 Generate unit tests
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/tm-core/src/auth/credential-store.ts (2)
45-55: Use explicit checks for expiry; avoid truthy guards that skip 0 and NaN cases.-// Check if token is expired (API keys never expire) -const isApiKey = authData.tokenType === 'api_key'; -if ( - !isApiKey && - expiresAtMs && - expiresAtMs < Date.now() && - !options?.allowExpired -) { +// Check if token is expired (API keys never expire) +const isApiKey = authData.tokenType === 'api_key'; +if ( + !isApiKey && + expiresAtMs !== undefined && + Date.now() >= expiresAtMs && + !options?.allowExpired +) { this.logger.warn('Authentication token has expired'); return null; }
129-133: Consider exposing a booleanhasValidCredentials()variant that surfaces expiry reason.Optional: return a discriminated union with reason ('missing' | 'expired' | 'invalid') for better UX/telemetry.
packages/tm-core/src/interfaces/configuration.interface.ts (1)
107-122: Deduplicate properties and use theStorageTypealias to avoid drift.
StorageSettingsalready inheritstype,apiEndpoint,apiAccessToken. Redeclare only when changing variance. PreferStorageTypehere; keepapiConfiguredoptional but considerreadonly.-export interface StorageSettings - extends Omit<RuntimeStorageConfig, 'apiConfigured'> { - /** Storage backend type - 'auto' detects based on auth status */ - type: 'file' | 'api' | 'auto'; - /** Base path for file storage */ - basePath?: string; - /** API endpoint for API storage (Hamster integration) */ - apiEndpoint?: string; - /** Access token for API authentication */ - apiAccessToken?: string; +export interface StorageSettings + extends Omit<RuntimeStorageConfig, 'apiConfigured'> { + /** Storage backend type - 'auto' detects based on auth status */ + type: StorageType; + /** Base path for file storage */ + basePath?: string; @@ - apiConfigured?: boolean; + /** Computed internally */ + readonly apiConfigured?: boolean;packages/tm-core/src/config/config-manager.ts (3)
140-156: Regression: file-storage basePath is dropped by new RuntimeStorageConfig flow.
getStorageConfig()no longer surfacesstorage.basePath. DownstreamStorageFactory.createFromStorageConfig()wraps only this runtime object, socreateFileStorage()falls back toprojectPathand ignores user-configuredbasePath. This breaks existing setups that rely on a custom storage path.Apply both diffs to carry
basePaththrough the runtime surface.
- interfaces/configuration.interface.ts — add
basePathtoRuntimeStorageConfig:export interface RuntimeStorageConfig { /** Storage backend type */ type: StorageType; + /** Base path for file storage (if configured) */ + basePath?: string; /** API endpoint for API storage (Hamster integration) */ apiEndpoint?: string; /** Access token for API authentication */ apiAccessToken?: string; /** * Indicates whether API is configured (has endpoint or token) * @computed Derived automatically from presence of apiEndpoint or apiAccessToken * @internal Should not be set manually - computed by ConfigManager */ apiConfigured: boolean; }
- config-manager.ts — include
basePathin the returned object:getStorageConfig(): RuntimeStorageConfig { const storage = this.config.storage; // Return the configured type (including 'auto') const storageType = storage?.type || 'auto'; + const basePath = storage?.basePath; if (storageType === 'api' || storageType === 'auto') { return { type: storageType, + basePath, apiEndpoint: storage?.apiEndpoint, apiAccessToken: storage?.apiAccessToken, apiConfigured: Boolean(storage?.apiEndpoint || storage?.apiAccessToken) }; } - return { type: storageType, apiConfigured: false }; + return { type: storageType, basePath, apiConfigured: false }; }
213-222: Bug:updateConfig()doesn’t actually re-merge due toinitialize()guard.
initialize()early-returns wheninitializedis true, so updates never reload precedence stacks.Apply this minimal fix:
async updateConfig(updates: PartialConfiguration): Promise<void> { // Merge updates into current config Object.assign(this.config, updates); // Save to persistence await this.persistence.saveConfig(this.config); // Re-initialize to respect precedence - await this.initialize(); + this.initialized = false; + await this.initialize(); }
188-190: Clarify or adjustisUsingApiStorageto handle ‘auto’
Refactor: either rename this toisApiExplicitlyConfigured()(to reflect that it only checkstype === 'api') or update it to compute the runtime‐resolved storage type—e.g. via a newStorageFactory.getEffectiveStorageType(config)—so that it returns true when ‘auto’ resolves to API.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
packages/tm-core/src/auth/auth-manager.test.ts(1 hunks)packages/tm-core/src/auth/auth-manager.ts(0 hunks)packages/tm-core/src/auth/credential-store.ts(4 hunks)packages/tm-core/src/config/config-manager.ts(2 hunks)packages/tm-core/src/interfaces/configuration.interface.ts(2 hunks)packages/tm-core/src/services/task-service.ts(1 hunks)packages/tm-core/src/storage/storage-factory.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/tm-core/src/auth/auth-manager.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/git_workflow.mdc)
**/*.{test,spec}.{js,ts,jsx,tsx}: Create a test file and ensure all tests pass when all subtasks are complete; commit tests if added or modified
When all subtasks are complete, run final testing using the appropriate test runner (e.g., npm test, jest, or manual testing)
Files:
packages/tm-core/src/auth/auth-manager.test.ts
**/*.{test,spec}.*
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Test files should follow naming conventions: .test., .spec., or _test. depending on the language
Files:
packages/tm-core/src/auth/auth-manager.test.ts
**/?(*.)+(spec|test).ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
In JavaScript/TypeScript projects using Jest, test files should match *.test.ts and *.spec.ts patterns
Files:
packages/tm-core/src/auth/auth-manager.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
**/*.test.ts: Use established mocking patterns for dependencies such as bcrypt and Prisma in test files
Test all code paths, including edge cases and error scenarios, in test files
Use descriptive names for test functions, such as should_return_error_for_invalid_input
Files:
packages/tm-core/src/auth/auth-manager.test.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#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.
📚 Learning: 2025-08-03T12:13:33.875Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-08-03T12:13:33.875Z
Learning: Applies to **/*.test.ts : Use established mocking patterns for dependencies such as bcrypt and Prisma in test files
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{integration,e2e}/**/*.test.js : Properly mock session objects when required by functions, and reset environment variables between tests if modified.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-08-03T12:13:33.875Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-08-03T12:13:33.875Z
Learning: Applies to {tests/setup.ts,tests/setup/integration.ts,tests/teardown.ts} : Test setup files should be created at tests/setup.ts, tests/setup/integration.ts, and tests/teardown.ts
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:06:04.909Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/ai_providers.mdc:0-0
Timestamp: 2025-07-18T17:06:04.909Z
Learning: Applies to tests/unit/ai-providers/*.test.js : Create unit tests for the new provider in tests/unit/ai-providers/<provider-name>.test.js, mocking ai-sdk/<provider-name> and core ai module functions, and testing all exported functions for correct behavior and error handling.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
🧬 Code graph analysis (5)
packages/tm-core/src/auth/credential-store.ts (2)
tests/unit/config-manager.test.mjs (1)
baseName(192-192)packages/tm-core/src/logger/logger.ts (1)
error(163-166)
packages/tm-core/src/storage/storage-factory.ts (1)
packages/tm-core/src/interfaces/configuration.interface.ts (2)
RuntimeStorageConfig(88-101)IConfiguration(200-242)
packages/tm-core/src/services/task-service.ts (1)
packages/tm-core/src/storage/storage-factory.ts (1)
StorageFactory(19-261)
packages/tm-core/src/auth/auth-manager.test.ts (1)
packages/tm-core/src/auth/auth-manager.ts (1)
AuthManager(18-145)
packages/tm-core/src/config/config-manager.ts (1)
packages/tm-core/src/interfaces/configuration.interface.ts (1)
RuntimeStorageConfig(88-101)
🪛 ast-grep (0.38.6)
packages/tm-core/src/auth/credential-store.ts
[warning] 148-148: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${baseName}\\.corrupt-\\d+$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (8)
packages/tm-core/src/auth/auth-manager.test.ts (1)
24-30: LGTM: verifies singleton identity.packages/tm-core/src/auth/credential-store.ts (1)
63-76: Quarantine logic LGTM; good defensive handling.packages/tm-core/src/interfaces/configuration.interface.ts (3)
76-83: LGTM: clear storage type union including 'auto'.
84-102: LGTM: RuntimeStorageConfig shape is sensible;apiConfiguredas computed flag makes sense.
425-433: Verification complete – no downstream updates required.
AllgetStorageConfig()andStorageFactory.createFromStorageConfig()signatures and call sites match the newRuntimeStorageConfig, and there are no remaining direct references to moved storage settings.packages/tm-core/src/config/config-manager.ts (1)
9-12: Type import changes look good.Importing
RuntimeStorageConfigaligns this module with the new runtime storage surface.packages/tm-core/src/storage/storage-factory.ts (2)
213-217: Validation for 'auto' type LGTM.Recognizing 'auto' explicitly removes false-positive validation errors.
6-11: Import path correct – no changes needed.packages/tm-core/src/storage/file-storage/index.tsexportsFileStorageas both default and named.
Crunchyman-ralph
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/tm-core/src/auth/auth-manager.ts (4)
20-21: Type-safe singleton: avoid null casts and allow reset with undefined.Current
null as anyis unsafe. Make the static field optional and handle undefined.-export class AuthManager { - private static instance: AuthManager; +export class AuthManager { + private static instance: AuthManager | undefined; @@ static resetInstance(): void { - AuthManager.instance = null as any; + AuthManager.instance = undefined; }Also applies to: 50-52
115-135: Wrap refresh failures with a consistent AuthenticationError.Preserve cause while returning a typed error code.
- try { + try { // Use Supabase client to refresh the token const response = await this.supabaseClient.refreshSession( authData.refreshToken ); @@ this.credentialStore.saveCredentials(newAuthData); return newAuthData; } catch (error) { - throw error; + throw new AuthenticationError( + 'Failed to refresh authentication token', + 'REFRESH_FAILED', + error + ); }
140-147: Use project logger instead of console.warn.Aligns with logging strategy and testability.
- } catch (error) { - // Log but don't throw - we still want to clear local credentials - console.warn('Failed to sign out from Supabase:', error); + } catch (error) { + // Log but don't throw - we still want to clear local credentials + getLogger('AuthManager').warn('Failed to sign out from Supabase:', error); }
81-98: API key validation: consider centralizing constraints.The length check is arbitrary; if there’s a schema (e.g., prefix or charset), validate against it and document. Otherwise keep minimal to avoid false negatives.
♻️ Duplicate comments (4)
packages/tm-core/src/auth/credential-store.ts (2)
155-161: Escape baseName in dynamic regex to avoid false matches/ReDoS.- const baseName = path.basename(this.config.configFile); - const corruptPattern = new RegExp(`^${baseName}\\.corrupt-\\d+$`); + const baseName = path.basename(this.config.configFile); + const escaped = baseName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const corruptPattern = new RegExp(`^${escaped}\\.corrupt-\\d+$`);
165-186: Reduce I/O: filter Dirents before stat.- const files = fs.readdirSync(dir); + const entries = fs.readdirSync(dir, { withFileTypes: true }); const now = Date.now(); - for (const file of files) { - if (corruptPattern.test(file)) { - const filePath = path.join(dir, file); + for (const entry of entries) { + if (!entry.isFile()) continue; + const file = entry.name; + if (corruptPattern.test(file)) { + const filePath = path.join(dir, file);packages/tm-core/src/auth/auth-manager.test.ts (2)
21-24: Clear mocks between tests to avoid leakage.beforeEach(() => { // Reset singleton before each test AuthManager.resetInstance(); + vi.clearAllMocks(); });
55-57: Make warning assertion resilient to wording changes.- expect(mockLogger.warn).toHaveBeenCalledWith( - 'getInstance called with config after initialization; config is ignored.' - ); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringMatching(/config.*after initialization.*ignored/i) + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/tm-core/src/auth/auth-manager.test.ts(1 hunks)packages/tm-core/src/auth/auth-manager.ts(2 hunks)packages/tm-core/src/auth/credential-store.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/git_workflow.mdc)
**/*.{test,spec}.{js,ts,jsx,tsx}: Create a test file and ensure all tests pass when all subtasks are complete; commit tests if added or modified
When all subtasks are complete, run final testing using the appropriate test runner (e.g., npm test, jest, or manual testing)
Files:
packages/tm-core/src/auth/auth-manager.test.ts
**/*.{test,spec}.*
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Test files should follow naming conventions: .test., .spec., or _test. depending on the language
Files:
packages/tm-core/src/auth/auth-manager.test.ts
**/?(*.)+(spec|test).ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
In JavaScript/TypeScript projects using Jest, test files should match *.test.ts and *.spec.ts patterns
Files:
packages/tm-core/src/auth/auth-manager.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
**/*.test.ts: Use established mocking patterns for dependencies such as bcrypt and Prisma in test files
Test all code paths, including edge cases and error scenarios, in test files
Use descriptive names for test functions, such as should_return_error_for_invalid_input
Files:
packages/tm-core/src/auth/auth-manager.test.ts
🧠 Learnings (16)
📓 Common learnings
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#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.
📚 Learning: 2025-08-03T12:13:33.875Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-08-03T12:13:33.875Z
Learning: Applies to **/*.test.ts : Use established mocking patterns for dependencies such as bcrypt and Prisma in test files
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{integration,e2e}/**/*.test.js : Properly mock session objects when required by functions, and reset environment variables between tests if modified.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:06:04.909Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/ai_providers.mdc:0-0
Timestamp: 2025-07-18T17:06:04.909Z
Learning: Applies to tests/unit/ai-providers/*.test.js : Create unit tests for the new provider in tests/unit/ai-providers/<provider-name>.test.js, mocking ai-sdk/<provider-name> and core ai module functions, and testing all exported functions for correct behavior and error handling.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Be careful with how you mock or stub functions that depend on module state; use factory functions in mocks to ensure proper initialization order.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Use jest.spyOn() after imports to create spies on mock functions and reference these spies in test assertions.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{unit,integration,e2e}/**/*.test.js : Do not try to use the real action implementation without proper mocking, and do not mock Commander partially—either mock it completely or test the action directly.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Follow the test file organization: mocks must be set up before importing modules under test, and spies on mocked modules should be set up after imports.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Do not import real AI service clients in tests; create fully mocked versions that return predictable responses.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Troubleshoot mock functions not being called by ensuring mocks are defined before imports, referencing the correct mock instance, and verifying import paths.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:12:57.903Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to scripts/modules/**/*.test.js : Follow the mock-first-then-import pattern and use jest.spyOn() for testing in Jest, clearing mocks between tests.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{unit,integration,e2e}/**/*.test.js : When testing CLI commands built with Commander.js, test the command action handlers directly rather than trying to mock the entire Commander.js chain.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Clear mocks between tests with jest.clearAllMocks() in beforeEach.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Isolate tests by properly mocking shared resources and resetting state in beforeEach and afterEach hooks.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Do not rely on environment variables for API keys in tests; set mock environment variables in test setup.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Verify file operations use the correct paths in expect statements.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
🧬 Code graph analysis (2)
packages/tm-core/src/auth/auth-manager.ts (2)
packages/tm-core/src/logger/factory.ts (1)
getLogger(23-44)packages/tm-core/src/logger/index.ts (1)
getLogger(8-8)
packages/tm-core/src/auth/credential-store.ts (2)
packages/tm-core/src/auth/types.ts (1)
AuthenticationError(73-85)packages/tm-core/src/logger/logger.ts (1)
error(163-166)
🪛 ast-grep (0.38.6)
packages/tm-core/src/auth/credential-store.ts
[warning] 158-158: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${baseName}\\.corrupt-\\d+$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (2)
packages/tm-core/src/auth/auth-manager.test.ts (2)
8-18: Logger mock pattern LGTM.Shared singleton mock ensures assertions hit the same instance used by production code.
60-72: No-warning case LGTM.Asserts on the shared mock; clear and correct.
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/tm-core/src/interfaces/configuration.interface.ts (1)
423-427: Provide defaults for required storage booleans.StorageSettings declares enableBackup, enableCompression, and atomicOperations as required booleans, but DEFAULT_CONFIG_VALUES.STORAGE lacks defaults, risking undefined fields in defaults and downstream code.
Apply:
STORAGE: { TYPE: 'auto' as const, ENCODING: 'utf8' as BufferEncoding, - MAX_BACKUPS: 5 + MAX_BACKUPS: 5, + // New boolean defaults for required fields + ENABLE_BACKUP: false, + ENABLE_COMPRESSION: false, + ATOMIC_OPERATIONS: true },And ensure the factory maps these to:
- storage.enableBackup = DEFAULT_CONFIG_VALUES.STORAGE.ENABLE_BACKUP
- storage.enableCompression = DEFAULT_CONFIG_VALUES.STORAGE.ENABLE_COMPRESSION
- storage.atomicOperations = DEFAULT_CONFIG_VALUES.STORAGE.ATOMIC_OPERATIONS
packages/tm-core/src/storage/storage-factory.ts (1)
58-86: Clarify error when 'api' is only partially configured (better DX)If one of
apiEndpoint/apiAccessTokenis missing and there is no session, the message “not authenticated” can mislead. Include which fields are missing and actionable guidance.Apply:
case 'api': - if (!StorageFactory.isHamsterAvailable(config)) { - // Check if authenticated via AuthManager - const authManager = AuthManager.getInstance(); - if (!authManager.isAuthenticated()) { - throw new TaskMasterError( - 'API storage configured but not authenticated. Run: tm auth login', - ERROR_CODES.MISSING_CONFIGURATION, - { storageType: 'api' } - ); - } + if (!StorageFactory.isHamsterAvailable(config)) { + const missing: string[] = []; + if (!config.storage?.apiEndpoint) missing.push('apiEndpoint'); + if (!config.storage?.apiAccessToken) missing.push('apiAccessToken'); + + // Check if authenticated via AuthManager + const authManager = AuthManager.getInstance(); + if (!authManager.isAuthenticated()) { + throw new TaskMasterError( + `API storage not fully configured (${missing.join(', ') || 'credentials missing'}). Run: tm auth login, or set the missing field(s).`, + ERROR_CODES.MISSING_CONFIGURATION, + { storageType: 'api', missing } + ); + }packages/tm-core/src/auth/auth-manager.ts (1)
117-124: Use the logger instead of console.warn for consistencyRoute sign-out warnings through the logging system.
Apply:
- } catch (error) { - // Log but don't throw - we still want to clear local credentials - console.warn('Failed to sign out from Supabase:', error); - } + } catch (error) { + // Log but don't throw - we still want to clear local credentials + getLogger('AuthManager').warn('Failed to sign out from Supabase:', error); + }
♻️ Duplicate comments (5)
packages/tm-core/src/auth/auth-manager.test.ts (1)
62-64: Make warning assertion resilient to wording.Avoid brittle exact-string match; assert via regex/substring.
Apply:
- expect(mockLogger.warn).toHaveBeenCalledWith( - 'getInstance called with config after initialization; config is ignored.' - ); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringMatching(/config.*after initialization.*ignored/i) + );packages/tm-core/src/storage/storage-factory.ts (1)
27-37: Propagation of projectPath fixed — resolves prior reviewPassing
{ storage: storageConfig, projectPath }intocreate()correctly populatesApiStorage.projectId. Looks good.packages/tm-core/src/auth/auth-manager.ts (1)
20-20: Type the singleton as nullable; drop!andas anyMake
instanceAuthManager | nullto avoidnull as anyinresetInstance()and the non-null assertion on return.Apply:
-export class AuthManager { - private static instance: AuthManager; +export class AuthManager { + private static instance: AuthManager | null = null; @@ - static getInstance(config?: Partial<AuthConfig>): AuthManager { - if (!AuthManager.instance) { - AuthManager.instance = new AuthManager(config); - } else if (config) { + static getInstance(config?: Partial<AuthConfig>): AuthManager { + if (!AuthManager.instance) { + AuthManager.instance = new AuthManager(config); + } else if (config) { // Warn if config is provided after initialization const logger = getLogger('AuthManager'); logger.warn( 'getInstance called with config after initialization; config is ignored.' ); } - return AuthManager.instance!; + return AuthManager.instance; } @@ - static resetInstance(): void { - AuthManager.instance = null as any; - } + static resetInstance(): void { + AuthManager.instance = null; + }Also applies to: 34-45, 50-52
packages/tm-core/src/auth/credential-store.ts (2)
32-44: Good fix: robust timestamp normalization.Explicit finite/NaN checks handle both number and ISO string correctly and address the prior NaN truthiness bug.
100-125: Good improvements: no input mutation and strict validation.Cloning to add savedAt and validating expiresAt with Number.isNaN/finite looks solid. Matches prior suggestions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
packages/tm-core/src/auth/auth-manager.test.ts(1 hunks)packages/tm-core/src/auth/auth-manager.ts(2 hunks)packages/tm-core/src/auth/credential-store.test.ts(1 hunks)packages/tm-core/src/auth/credential-store.ts(4 hunks)packages/tm-core/src/auth/types.ts(2 hunks)packages/tm-core/src/config/config-manager.ts(4 hunks)packages/tm-core/src/interfaces/configuration.interface.ts(1 hunks)packages/tm-core/src/storage/storage-factory.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/git_workflow.mdc)
**/*.{test,spec}.{js,ts,jsx,tsx}: Create a test file and ensure all tests pass when all subtasks are complete; commit tests if added or modified
When all subtasks are complete, run final testing using the appropriate test runner (e.g., npm test, jest, or manual testing)
Files:
packages/tm-core/src/auth/credential-store.test.tspackages/tm-core/src/auth/auth-manager.test.ts
**/*.{test,spec}.*
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Test files should follow naming conventions: .test., .spec., or _test. depending on the language
Files:
packages/tm-core/src/auth/credential-store.test.tspackages/tm-core/src/auth/auth-manager.test.ts
**/?(*.)+(spec|test).ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
In JavaScript/TypeScript projects using Jest, test files should match *.test.ts and *.spec.ts patterns
Files:
packages/tm-core/src/auth/credential-store.test.tspackages/tm-core/src/auth/auth-manager.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
**/*.test.ts: Use established mocking patterns for dependencies such as bcrypt and Prisma in test files
Test all code paths, including edge cases and error scenarios, in test files
Use descriptive names for test functions, such as should_return_error_for_invalid_input
Files:
packages/tm-core/src/auth/credential-store.test.tspackages/tm-core/src/auth/auth-manager.test.ts
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/git_workflow.mdc:0-0
Timestamp: 2025-07-18T17:10:31.810Z
Learning: Pull Request descriptions must use the provided template, including Task Overview, Subtasks Completed, Implementation Details, Testing, Breaking Changes, and Related Tasks
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#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.
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{unit,integration,e2e}/**/*.test.js : Use sample task fixtures for consistent test data, mock file system operations, and test both success and error paths for task operations.
Applied to files:
packages/tm-core/src/auth/credential-store.test.ts
📚 Learning: 2025-07-18T17:12:57.903Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to scripts/modules/**/*.test.js : Test core logic independently with both data formats, mock file system operations, test tag resolution behavior, and verify migration compatibility in unit tests.
Applied to files:
packages/tm-core/src/auth/credential-store.test.ts
📚 Learning: 2025-09-03T12:16:15.866Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#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-core/src/auth/credential-store.ts
📚 Learning: 2025-08-03T12:13:33.875Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-08-03T12:13:33.875Z
Learning: Applies to **/*.test.ts : Use established mocking patterns for dependencies such as bcrypt and Prisma in test files
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Be careful with how you mock or stub functions that depend on module state; use factory functions in mocks to ensure proper initialization order.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{unit,integration,e2e}/**/*.test.js : Do not try to use the real action implementation without proper mocking, and do not mock Commander partially—either mock it completely or test the action directly.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{integration,e2e}/**/*.test.js : Properly mock session objects when required by functions, and reset environment variables between tests if modified.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Use jest.spyOn() after imports to create spies on mock functions and reference these spies in test assertions.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Do not import real AI service clients in tests; create fully mocked versions that return predictable responses.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{unit,integration,e2e}/**/*.test.js : When testing CLI commands built with Commander.js, test the command action handlers directly rather than trying to mock the entire Commander.js chain.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Follow the test file organization: mocks must be set up before importing modules under test, and spies on mocked modules should be set up after imports.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Troubleshoot mock functions not being called by ensuring mocks are defined before imports, referencing the correct mock instance, and verifying import paths.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{unit,integration,e2e}/**/*.test.js : Mock the action handlers for CLI commands and verify they're called with correct arguments.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Clear mocks between tests with jest.clearAllMocks() in beforeEach.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:12:57.903Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to scripts/modules/**/*.test.js : Follow the mock-first-then-import pattern and use jest.spyOn() for testing in Jest, clearing mocks between tests.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Isolate tests by properly mocking shared resources and resetting state in beforeEach and afterEach hooks.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Do not rely on environment variables for API keys in tests; set mock environment variables in test setup.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Verify file operations use the correct paths in expect statements.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
🧬 Code graph analysis (6)
packages/tm-core/src/auth/credential-store.test.ts (2)
packages/tm-core/src/auth/credential-store.ts (1)
CredentialStore(11-220)packages/tm-core/src/auth/types.ts (2)
AuthCredentials(5-13)AuthenticationError(75-87)
packages/tm-core/src/auth/credential-store.ts (2)
packages/tm-core/src/logger/logger.ts (1)
error(163-166)packages/tm-core/src/auth/types.ts (1)
AuthenticationError(75-87)
packages/tm-core/src/storage/storage-factory.ts (1)
packages/tm-core/src/interfaces/configuration.interface.ts (2)
RuntimeStorageConfig(88-103)IConfiguration(198-240)
packages/tm-core/src/config/config-manager.ts (1)
packages/tm-core/src/interfaces/configuration.interface.ts (1)
RuntimeStorageConfig(88-103)
packages/tm-core/src/auth/auth-manager.test.ts (1)
packages/tm-core/src/auth/auth-manager.ts (1)
AuthManager(19-136)
packages/tm-core/src/auth/auth-manager.ts (1)
packages/tm-core/src/logger/factory.ts (1)
getLogger(23-44)
🪛 ast-grep (0.38.6)
packages/tm-core/src/auth/credential-store.ts
[warning] 182-182: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${escapedBaseName}\\.corrupt-\\d+$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (13)
packages/tm-core/src/interfaces/configuration.interface.ts (2)
77-83: LGTM: StorageType addition is clear and future-proof.The union and docs read well and set up the new 'auto' mode cleanly.
115-121: Confirm persistence doesn’t serialize computed apiConfigured
StorageSettings already marks apiConfigured readonly, but verify that ConfigPersistence.saveConfig is always invoked with the merged PartialConfiguration (rather than the output of getStorageConfig) so that the computed apiConfigured field never ends up in the persisted JSON.packages/tm-core/src/auth/types.ts (3)
75-87: LGTM: AuthenticationError carries code and cause.Appending cause stack aids debugging. No changes needed.
67-71: Removal of ‘INVALID_API_KEY’ is safe—no references found in codebase
Global searches across all packages, tests, snapshots, and documentation returned zero occurrences ofINVALID_API_KEY, so there are no remaining call sites to update.
10-12: expiresAt handling is consistently normalized and parsed Supabase-client and OAuth-service both emit ISO strings; the credential-store correctly parses strings via Date.parse and uses numeric timestamps directly, then re-serializes to ISO on save; existing tests cover all string/number/invalid cases.packages/tm-core/src/auth/auth-manager.test.ts (1)
81-92: LGTM: resetInstance recreates a fresh singleton.Good coverage for the reset path.
packages/tm-core/src/storage/storage-factory.ts (2)
214-218: Validation for 'auto' looks correctTreating 'auto' as valid and deferring to runtime detection is consistent with the factory logic.
143-169: All StorageFactory.create invocations supply projectPath
The only call-site is the internal wrapper at storage-factory.ts:33–35, which passesprojectPath; no external directcreate()ornew ApiStorageinvocations were found.packages/tm-core/src/config/config-manager.ts (2)
9-12: Imports updated to include RuntimeStorageConfig — goodKeeps the return type aligned with the shared interface.
228-229: Re-initialization trigger after updates — goodResetting
initializedensures precedence is reapplied on subsequentinitialize().packages/tm-core/src/auth/credential-store.ts (1)
45-49: Policy check: rejecting credentials without expiresAt.You’re returning null (warn) when expiresAt is missing/invalid. Confirm this is intentional for all token types now that API keys are removed; otherwise consider downgrading to debug or allowing a configurable fallback.
packages/tm-core/src/auth/credential-store.test.ts (2)
24-26: Good: logger mocked to keep tests quiet and assertable.
239-263: Solid check for ISO persistence.Verifies that saved expiresAt is a valid ISO string; nicely done.
| isApiExplicitlyConfigured(): boolean { | ||
| return this.getStorageConfig().type === 'api'; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Rename is breaking; consider a deprecation shim or verify usages
isApiExplicitlyConfigured() replaces isUsingApiStorage(). Ensure no remaining callers of the old name, or add a temporary alias with a deprecation notice.
Search:
Optionally add a temporary alias:
// Deprecated: remove in the next minor
/** @deprecated Use isApiExplicitlyConfigured() */
isUsingApiStorage(): boolean {
// eslint-disable-next-line no-console
console.warn('[deprecate] ConfigManager.isUsingApiStorage() → use isApiExplicitlyConfigured()');
return this.isApiExplicitlyConfigured();
}🏁 Script executed:
#!/bin/bash
# Find usages of the old API to assess blast radius
rg -nP -C2 '\bisUsingApiStorage\s*\('Length of output: 771
Remove legacy isUsingApiStorage references in tests and finalize the rename
Update packages/tm-core/src/config/config-manager.spec.ts (around lines 311–328) to replace all isUsingApiStorage() calls with isApiExplicitlyConfigured(), then remove the now‐unused isUsingApiStorage() method from config-manager.ts.
🤖 Prompt for AI Agents
In packages/tm-core/src/config/config-manager.ts around lines 195–197, tests
still reference the legacy isUsingApiStorage() and the method should be removed
after updating tests; open packages/tm-core/src/config/config-manager.spec.ts
(around lines 311–328) and replace all calls to isUsingApiStorage() with
isApiExplicitlyConfigured(), run tests, then delete the now-unused
isUsingApiStorage() method from config-manager.ts and remove any imports/usages
elsewhere; ensure linter/test pass and update any related type exports or docs
if necessary.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
package.json (1)
117-135: Add Vitest dependencies; retain Jest until tests migrate
package.json devDependencies: add"vitest": "^2.x"and"@vitest/coverage-v8": "^2.x". Migrate test mocks fromjest.*tovi.*(or enable Jest compatibility) before removingjest,jest-environment-node, and@types/jest.packages/tm-core/src/interfaces/configuration.interface.ts (1)
118-129: Add defaults for new storage booleans or make them optionalenableBackup, enableCompression and atomicOperations are required by the interface but not defined in DEFAULT_CONFIG_VALUES.STORAGE—this breaks existing configs. Either:
- Add these defaults under STORAGE in DEFAULT_CONFIG_VALUES (e.g. ENABLE_BACKUP, ENABLE_COMPRESSION, ATOMIC_OPERATIONS), or
- Change these properties to optional (
enableBackup?: boolean;, etc.) so the factory can backfill them.packages/tm-core/src/config/config-manager.spec.ts (3)
316-327: Add a complementary negative case for 'auto' with partial API settings.To lock in the “explicitly configured” semantics, add a case where storage.type is
autobut API fields exist and assertisApiExplicitlyConfigured()remainsfalse.+ it('should keep isApiExplicitlyConfigured false for auto with API fields', async () => { + (manager as any).config = { + storage: { type: 'auto', apiEndpoint: 'https://api.example.com' } + }; + expect(manager.isApiExplicitlyConfigured()).toBe(false); + });
170-181: Fix expectation to tolerate added fields in storage config (now includes basePath).
getStorageConfig()returns additional keys (e.g.,basePath). Replace strict equality with a partial match and assert basePath separately to avoid brittle failures.- const storage = manager.getStorageConfig(); - expect(storage).toEqual({ type: 'auto', apiConfigured: false }); + const storage = manager.getStorageConfig(); + expect(storage).toMatchObject({ type: 'auto', apiConfigured: false }); + expect(storage.basePath).toBe(testProjectRoot);
333-343: Update mocks and assertions for RuntimeStateManager method rename.
ConfigManager.getActiveTag()now callsstateManager.getCurrentTag()andsetActiveTag()callsstateManager.setCurrentTag(). The test’s mock exposesgetActiveTag/setActiveTag, causing runtime errors.- vi.mocked(RuntimeStateManager).mockImplementation( - () => - ({ - loadState: vi.fn().mockResolvedValue({ activeTag: 'master' }), - saveState: vi.fn().mockResolvedValue(undefined), - getActiveTag: vi.fn().mockReturnValue('master'), - setActiveTag: vi.fn().mockResolvedValue(undefined), - getState: vi.fn().mockReturnValue({ activeTag: 'master' }), - updateMetadata: vi.fn().mockResolvedValue(undefined), - clearState: vi.fn().mockResolvedValue(undefined) - }) as any - ); + vi.mocked(RuntimeStateManager).mockImplementation( + () => + ({ + loadState: vi.fn().mockResolvedValue({ activeTag: 'master' }), + saveState: vi.fn().mockResolvedValue(undefined), + getCurrentTag: vi.fn().mockReturnValue('master'), + setCurrentTag: vi.fn().mockResolvedValue(undefined), + getState: vi.fn().mockReturnValue({ activeTag: 'master' }), + updateMetadata: vi.fn().mockResolvedValue(undefined), + clearState: vi.fn().mockResolvedValue(undefined) + }) as any + ); @@ - const stateManager = (manager as any).stateManager; - expect(stateManager.setActiveTag).toHaveBeenCalledWith('feature-branch'); + const stateManager = (manager as any).stateManager; + expect(stateManager.setCurrentTag).toHaveBeenCalledWith('feature-branch');packages/tm-core/src/storage/storage-factory.ts (1)
46-49: Ensure projectPath is always present inside config during create().
createApiStorage reads from config.projectPath; set it up-front.static create( config: Partial<IConfiguration>, projectPath: string ): IStorage { + // Ensure projectPath is available to downstream creators + config = { ...config, projectPath: config.projectPath ?? projectPath };
♻️ Duplicate comments (7)
packages/tm-core/src/interfaces/configuration.interface.ts (1)
88-103: Readonly apiConfigured: resolved as requested earlier.Marking apiConfigured as readonly matches its computed/internal nature. Thanks for addressing the prior feedback.
packages/tm-core/src/auth/auth-manager.test.ts (1)
74-79: Also restore spied implementations between tests.Helps avoid cross-test leakage when spies replace implementations.
beforeEach(() => { // Reset singleton before each test AuthManager.resetInstance(); vi.clearAllMocks(); CredentialStoreSpy.mockClear(); + vi.restoreAllMocks(); });packages/tm-core/src/storage/storage-factory.ts (2)
32-38: Good fix: propagate projectPath into wrapped config.
Addresses the earlier gap so ApiStorage.projectId is set.
5-12: Confirm TS module resolution for “.js” specifiers in TS sources.
Using ESM “.js” specifiers in .ts requires NodeNext/Bundler resolution.#!/bin/bash # Show module settings and find .js specifiers in tm-core jq '.compilerOptions | {module, moduleResolution, allowImportingTsExtensions}' packages/tm-core/tsconfig.json rg -n --type=ts -g 'packages/tm-core/**' "from '.*\\.js'"packages/tm-core/src/auth/auth-manager.ts (1)
34-45: Return non-null singleton or narrow type.
The return type is AuthManager but you return possibly null; add non-null assertion.- static getInstance(config?: Partial<AuthConfig>): AuthManager { + static getInstance(config?: Partial<AuthConfig>): AuthManager { @@ - return AuthManager.instance; + return AuthManager.instance!; }packages/tm-core/src/auth/credential-store.test.ts (2)
293-301: Nice fix: no double invocation in throw assertions.Switched to capturing the error once and asserting type/message, addressing the prior nit.
Also applies to: 312-320, 331-339
412-452: Coverage added for clearCredentials — looks good.Tests for exists/non-exists and failure-to-unlink paths close the gap noted earlier.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
package.json(1 hunks)packages/tm-core/src/auth/auth-manager.test.ts(1 hunks)packages/tm-core/src/auth/auth-manager.ts(3 hunks)packages/tm-core/src/auth/credential-store.test.ts(1 hunks)packages/tm-core/src/auth/credential-store.ts(4 hunks)packages/tm-core/src/config/config-manager.spec.ts(2 hunks)packages/tm-core/src/config/config-manager.ts(4 hunks)packages/tm-core/src/interfaces/configuration.interface.ts(1 hunks)packages/tm-core/src/storage/storage-factory.ts(5 hunks)packages/tm-core/tsconfig.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.cursor/rules/git_workflow.mdc)
**/*.{test,spec}.{js,ts,jsx,tsx}: Create a test file and ensure all tests pass when all subtasks are complete; commit tests if added or modified
When all subtasks are complete, run final testing using the appropriate test runner (e.g., npm test, jest, or manual testing)
Files:
packages/tm-core/src/config/config-manager.spec.tspackages/tm-core/src/auth/auth-manager.test.tspackages/tm-core/src/auth/credential-store.test.ts
**/*.{test,spec}.*
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Test files should follow naming conventions: .test., .spec., or _test. depending on the language
Files:
packages/tm-core/src/config/config-manager.spec.tspackages/tm-core/src/auth/auth-manager.test.tspackages/tm-core/src/auth/credential-store.test.ts
**/?(*.)+(spec|test).ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
In JavaScript/TypeScript projects using Jest, test files should match *.test.ts and *.spec.ts patterns
Files:
packages/tm-core/src/config/config-manager.spec.tspackages/tm-core/src/auth/auth-manager.test.tspackages/tm-core/src/auth/credential-store.test.ts
package.json
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Add and update test scripts in package.json to include test, test:watch, test:coverage, test:unit, test:integration, test:e2e, and test:ci
Files:
package.json
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
**/*.test.ts: Use established mocking patterns for dependencies such as bcrypt and Prisma in test files
Test all code paths, including edge cases and error scenarios, in test files
Use descriptive names for test functions, such as should_return_error_for_invalid_input
Files:
packages/tm-core/src/auth/auth-manager.test.tspackages/tm-core/src/auth/credential-store.test.ts
🧠 Learnings (30)
📓 Common learnings
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/git_workflow.mdc:0-0
Timestamp: 2025-07-18T17:10:31.810Z
Learning: Pull Request descriptions must use the provided template, including Task Overview, Subtasks Completed, Implementation Details, Testing, Breaking Changes, and Related Tasks
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#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.
📚 Learning: 2025-08-03T12:13:33.875Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-08-03T12:13:33.875Z
Learning: Applies to package.json : Add and update test scripts in package.json to include test, test:watch, test:coverage, test:unit, test:integration, test:e2e, and test:ci
Applied to files:
package.json
📚 Learning: 2025-07-18T17:12:57.903Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to scripts/modules/**/*.test.js : Test core logic independently with both data formats, mock file system operations, test tag resolution behavior, and verify migration compatibility in unit tests.
Applied to files:
package.jsonpackages/tm-core/src/auth/credential-store.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Verify modifications on the in-memory task objects passed to writeJSON.
Applied to files:
package.json
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{unit,integration,e2e}/**/*.test.js : Use sample task fixtures for consistent test data, mock file system operations, and test both success and error paths for task operations.
Applied to files:
package.jsonpackages/tm-core/src/auth/credential-store.test.ts
📚 Learning: 2025-07-18T17:07:39.336Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-07-18T17:07:39.336Z
Learning: Applies to tests/{unit,integration,e2e,fixtures}/**/*.js : Test files must be organized as follows: unit tests in tests/unit/, integration tests in tests/integration/, end-to-end tests in tests/e2e/, and test fixtures in tests/fixtures/.
Applied to files:
package.json
📚 Learning: 2025-07-18T17:12:57.903Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to scripts/modules/**/*.test.js : Test CLI and MCP interfaces with real task data, verify end-to-end workflows across tag contexts, and test error scenarios and recovery in integration tests.
Applied to files:
package.json
📚 Learning: 2025-07-18T17:10:31.810Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/git_workflow.mdc:0-0
Timestamp: 2025-07-18T17:10:31.810Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx} : Create a test file if needed and ensure all tests pass before completing a task
Applied to files:
package.json
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{integration,e2e}/**/*.test.js : Properly mock session objects when required by functions, and reset environment variables between tests if modified.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-08-03T12:13:33.875Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/test_workflow.mdc:0-0
Timestamp: 2025-08-03T12:13:33.875Z
Learning: Applies to **/*.test.ts : Use established mocking patterns for dependencies such as bcrypt and Prisma in test files
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Do not import real AI service clients in tests; create fully mocked versions that return predictable responses.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{unit,integration,e2e}/**/*.test.js : Do not try to use the real action implementation without proper mocking, and do not mock Commander partially—either mock it completely or test the action directly.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Be careful with how you mock or stub functions that depend on module state; use factory functions in mocks to ensure proper initialization order.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Use jest.spyOn() after imports to create spies on mock functions and reference these spies in test assertions.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{unit,integration,e2e}/**/*.test.js : When testing CLI commands built with Commander.js, test the command action handlers directly rather than trying to mock the entire Commander.js chain.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Follow the test file organization: mocks must be set up before importing modules under test, and spies on mocked modules should be set up after imports.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Troubleshoot mock functions not being called by ensuring mocks are defined before imports, referencing the correct mock instance, and verifying import paths.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to tests/{unit,integration,e2e}/**/*.test.js : Mock the action handlers for CLI commands and verify they're called with correct arguments.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Clear mocks between tests with jest.clearAllMocks() in beforeEach.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:12:57.903Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/new_features.mdc:0-0
Timestamp: 2025-07-18T17:12:57.903Z
Learning: Applies to scripts/modules/**/*.test.js : Follow the mock-first-then-import pattern and use jest.spyOn() for testing in Jest, clearing mocks between tests.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Isolate tests by properly mocking shared resources and resetting state in beforeEach and afterEach hooks.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Do not rely on environment variables for API keys in tests; set mock environment variables in test setup.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Verify file operations use the correct paths in expect statements.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:07:39.336Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/architecture.mdc:0-0
Timestamp: 2025-07-18T17:07:39.336Z
Learning: Module dependencies should be mocked before importing the test module, following Jest's hoisting behavior.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Do not define mock variables before jest.mock() calls (they won't be accessible due to hoisting).
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Always declare mocks before importing the modules being tested in Jest test files.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Follow the mock-first-then-import pattern for all Jest mocks.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-07-18T17:16:13.793Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/tests.mdc:0-0
Timestamp: 2025-07-18T17:16:13.793Z
Learning: Applies to **/*.test.js : Set mock environment variables in test setup and restore them after each test.
Applied to files:
packages/tm-core/src/auth/auth-manager.test.ts
📚 Learning: 2025-09-03T12:16:15.866Z
Learnt from: Crunchyman-ralph
PR: eyaltoledano/claude-task-master#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-core/src/auth/credential-store.ts
📚 Learning: 2025-07-18T17:08:48.695Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/commands.mdc:0-0
Timestamp: 2025-07-18T17:08:48.695Z
Learning: Applies to scripts/modules/commands.js : Use path.join() to construct file paths, follow established naming conventions (like task_001.txt), check file existence before deletion, and handle file deletion errors gracefully.
Applied to files:
packages/tm-core/src/auth/credential-store.ts
🧬 Code graph analysis (6)
packages/tm-core/src/config/config-manager.ts (1)
packages/tm-core/src/interfaces/configuration.interface.ts (1)
RuntimeStorageConfig(88-103)
packages/tm-core/src/storage/storage-factory.ts (2)
packages/tm-core/src/interfaces/configuration.interface.ts (3)
RuntimeStorageConfig(88-103)IConfiguration(196-238)StorageSettings(109-129)packages/tm-core/src/auth/auth-manager.ts (1)
AuthManager(19-136)
packages/tm-core/src/auth/auth-manager.test.ts (1)
packages/tm-core/src/auth/auth-manager.ts (1)
AuthManager(19-136)
packages/tm-core/src/auth/credential-store.test.ts (2)
packages/tm-core/src/auth/credential-store.ts (1)
CredentialStore(21-238)packages/tm-core/src/auth/types.ts (2)
AuthCredentials(5-13)AuthenticationError(75-87)
packages/tm-core/src/auth/auth-manager.ts (2)
packages/tm-core/src/logger/factory.ts (1)
getLogger(23-44)packages/tm-core/src/logger/logger.ts (1)
error(163-166)
packages/tm-core/src/auth/credential-store.ts (3)
packages/tm-core/src/logger/factory.ts (1)
getLogger(23-44)packages/tm-core/src/auth/types.ts (2)
AuthConfig(30-34)AuthenticationError(75-87)packages/tm-core/src/auth/config.ts (1)
getAuthConfig(32-37)
🔇 Additional comments (15)
packages/tm-core/src/interfaces/configuration.interface.ts (1)
77-83: StorageType addition looks good.Union and docs align with downstream factory logic. No action.
packages/tm-core/src/config/config-manager.spec.ts (1)
312-314: LGTM: new explicit API check semantics.The test correctly uses the renamed
isApiExplicitlyConfigured()and expectsfalseby default.packages/tm-core/src/auth/auth-manager.test.ts (2)
88-106: LGTM: config propagation is asserted via constructor spy.Good verification that the initial config reaches dependencies.
15-17: No changes needed: mock path matches SUT import.
The mock path'../logger/index.js'exactly matches the import inauth-manager.ts.packages/tm-core/src/config/config-manager.ts (3)
140-161: Good: runtime storage config now returns deterministic basePath and api flags.
basePath ?? this.projectRootavoids undefined;apiConfiguredreflects endpoint/token presence.
224-230: Re-init after update: explicitinitialized = falseis correct.Ensures precedence is reapplied on the next merge; no further changes needed.
195-197: No remaining callers ofisUsingApiStorageVerified via project-wide search; safe to proceed with the rename.packages/tm-core/src/auth/credential-store.ts (3)
45-76: Token expiry normalization and skew handling look solid.
Good finite checks, normalization, and 30s skew to avoid edge flakiness.
86-99: Corrupt-file quarantine and cleanup are robust.
Unique quarantine filenames and non-regex matching mitigate ReDoS and collisions.Also applies to: 193-237
115-141: Safe persistence without mutating input; ISO serialization validated.
Good security on dir/file modes and atomic rename.packages/tm-core/src/auth/credential-store.test.ts (5)
13-26: Good use of fs and logger mocks; aligns with team learnings.Automocking fs and injecting a mock logger matches our testing guidelines and the retrieved learning about mocking filesystem and exercising success/error paths.
46-117: Happy path and invalid timestamp cases are well covered.Covers ISO, number, invalid string/NaN/Infinity, and missing expiresAt with correct warning assertions.
378-394: Corrupt file quarantine path is well tested.Validates rename and warning without overfitting the filename suffix.
454-528: hasValidCredentials coverage is thorough.Valid, expired, missing, invalid JSON, and default allowExpired=false are all exercised with appropriate log assertions.
5-12: Style note: bare core-module imports match team preference.Using 'fs', 'path', 'os' without the node: prefix aligns with Crunchyman-ralph’s stated preference.
⛔ Skipped due to learnings
Learnt from: Crunchyman-ralph PR: eyaltoledano/claude-task-master#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: CR PR: eyaltoledano/claude-task-master#0 File: .cursor/rules/utilities.mdc:0-0 Timestamp: 2025-07-18T17:18:17.759Z Learning: Applies to {scripts/modules/utils.js,mcp-server/src/core/utils/path-utils.js,mcp-server/src/tools/utils.js} : Keep utilities relevant to their location, export all utility functions in a single statement per file, group related exports together, export configuration constants, do not use default exports, and do not create circular dependencies.Learnt from: CR PR: eyaltoledano/claude-task-master#0 File: .cursor/rules/test_workflow.mdc:0-0 Timestamp: 2025-08-03T12:13:33.875Z Learning: Applies to **/*.test.ts : Use established mocking patterns for dependencies such as bcrypt and Prisma in test filesLearnt from: CR PR: eyaltoledano/claude-task-master#0 File: .cursor/rules/tests.mdc:0-0 Timestamp: 2025-07-18T17:16:13.793Z Learning: Applies to **/*.test.js : Verify file operations use the correct paths in expect statements.Learnt from: CR PR: eyaltoledano/claude-task-master#0 File: .cursor/rules/tests.mdc:0-0 Timestamp: 2025-07-18T17:16:13.793Z Learning: Applies to tests/{unit,integration,e2e}/**/*.test.js : Use sample task fixtures for consistent test data, mock file system operations, and test both success and error paths for task operations.Learnt from: CR PR: eyaltoledano/claude-task-master#0 File: .cursor/rules/test_workflow.mdc:0-0 Timestamp: 2025-08-03T12:13:33.875Z Learning: Applies to {tests/integration/**/*.test.ts,src/**/*.integration.test.ts} : Use supertest for API endpoint testing in integration test files
| vi.mock('./credential-store.js', () => { | ||
| return { | ||
| CredentialStore: class { | ||
| constructor(config: any) { | ||
| CredentialStoreSpy(config); | ||
| this.getCredentials = vi.fn(() => null); | ||
| } | ||
| getCredentials() { | ||
| return null; | ||
| } | ||
| saveCredentials() {} | ||
| clearCredentials() {} | ||
| hasValidCredentials() { | ||
| return false; | ||
| } | ||
| } | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove duplicate getCredentials definitions in the mock class.
The constructor assignment shadows the prototype method. Keep one definition to avoid confusion.
vi.mock('./credential-store.js', () => {
return {
CredentialStore: class {
constructor(config: any) {
CredentialStoreSpy(config);
- this.getCredentials = vi.fn(() => null);
}
- getCredentials() {
- return null;
- }
+ getCredentials = vi.fn(() => null);
saveCredentials() {}
clearCredentials() {}
hasValidCredentials() {
return false;
}
}
};
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vi.mock('./credential-store.js', () => { | |
| return { | |
| CredentialStore: class { | |
| constructor(config: any) { | |
| CredentialStoreSpy(config); | |
| this.getCredentials = vi.fn(() => null); | |
| } | |
| getCredentials() { | |
| return null; | |
| } | |
| saveCredentials() {} | |
| clearCredentials() {} | |
| hasValidCredentials() { | |
| return false; | |
| } | |
| } | |
| }; | |
| }); | |
| vi.mock('./credential-store.js', () => { | |
| return { | |
| CredentialStore: class { | |
| constructor(config: any) { | |
| CredentialStoreSpy(config); | |
| } | |
| getCredentials = vi.fn(() => null); | |
| saveCredentials() {} | |
| clearCredentials() {} | |
| hasValidCredentials() { | |
| return false; | |
| } | |
| } | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In packages/tm-core/src/auth/auth-manager.test.ts around lines 21 to 38, the
mock class defines getCredentials twice (once assigned inside the constructor
and once as a prototype method) which is redundant and confusing; remove the
duplicate by deleting the constructor assignment (this.getCredentials = vi.fn(()
=> null)) and keep the single prototype getCredentials() { return null; } (or
alternatively keep the constructor assignment and remove the prototype method)
so the mock exposes only one clear implementation, and ensure the constructor
still calls CredentialStoreSpy(config).
| vi.mock('../clients/supabase-client.js', () => { | ||
| return { | ||
| SupabaseAuthClient: class { | ||
| constructor() {} | ||
| refreshSession() { | ||
| return Promise.resolve({}); | ||
| } | ||
| signOut() { | ||
| return Promise.resolve(); | ||
| } | ||
| } | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Stub realistic shapes for auth client responses (future-proof refresh/logout tests).
Return fields used by refreshToken() to prevent accidental breakage when coverage expands.
SupabaseAuthClient: class {
constructor() {}
- refreshSession() {
- return Promise.resolve({});
+ refreshSession() {
+ return Promise.resolve({
+ token: 'newAccess',
+ refreshToken: 'newRefresh',
+ expiresAt: Date.now() + 3600_000
+ });
}
signOut() {
return Promise.resolve();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vi.mock('../clients/supabase-client.js', () => { | |
| return { | |
| SupabaseAuthClient: class { | |
| constructor() {} | |
| refreshSession() { | |
| return Promise.resolve({}); | |
| } | |
| signOut() { | |
| return Promise.resolve(); | |
| } | |
| } | |
| }; | |
| }); | |
| vi.mock('../clients/supabase-client.js', () => { | |
| return { | |
| SupabaseAuthClient: class { | |
| constructor() {} | |
| refreshSession() { | |
| return Promise.resolve({ | |
| token: 'newAccess', | |
| refreshToken: 'newRefresh', | |
| expiresAt: Date.now() + 3600_000 | |
| }); | |
| } | |
| signOut() { | |
| return Promise.resolve(); | |
| } | |
| } | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In packages/tm-core/src/auth/auth-manager.test.ts around lines 56 to 68, the
SupabaseAuthClient mock returns empty objects which don't match the real client
shape and can break refresh/logout tests; update the mock so refreshSession()
resolves with the realistic shape used by refreshToken (e.g., an object with
data: { session: { access_token, refresh_token, expires_at }, user: { id, email
} }, error: null) and signOut() resolves with { error: null } (or the equivalent
fields your code expects) so tests remain accurate as coverage expands.
| it('should warn when config is provided after initialization', () => { | ||
| // Clear previous calls | ||
| mockLogger.warn.mockClear(); | ||
|
|
||
| // First call with config | ||
| AuthManager.getInstance({ baseUrl: 'https://first.auth.com' }); | ||
|
|
||
| // Second call with different config | ||
| AuthManager.getInstance({ baseUrl: 'https://second.auth.com' }); | ||
|
|
||
| // Verify warning was logged | ||
| expect(mockLogger.warn).toHaveBeenCalledWith( | ||
| expect.stringMatching(/config.*after initialization.*ignored/i) | ||
| ); | ||
| }); | ||
|
|
||
| it('should not warn when no config is provided after initialization', () => { | ||
| // Clear previous calls | ||
| mockLogger.warn.mockClear(); | ||
|
|
||
| // First call with config | ||
| AuthManager.getInstance({ configDir: '/test/config' }); | ||
|
|
||
| // Second call without config | ||
| AuthManager.getInstance(); | ||
|
|
||
| // Verify no warning was logged | ||
| expect(mockLogger.warn).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add coverage for refresh and logout paths.
Cover error when no refresh token, success path with save, and logout clearing credentials + remote sign-out call.
I can draft these tests quickly. Example additions:
it('refreshToken throws when no refresh token', async () => {
const mgr = AuthManager.getInstance();
await expect(mgr.refreshToken()).rejects.toThrow(/No refresh token/i);
});
it('logout clears credentials and calls remote sign-out', async () => {
const mgr = AuthManager.getInstance();
const store = (mgr as any).credentialStore;
const client = (mgr as any).supabaseClient;
vi.spyOn(store, 'clearCredentials');
vi.spyOn(client, 'signOut');
await mgr.logout();
expect(client.signOut).toHaveBeenCalled();
expect(store.clearCredentials).toHaveBeenCalled();
});Also applies to: 138-149
| it('should check token expiration correctly', () => { | ||
| const expiredTimestamp = Date.now() - 3600000; // 1 hour ago | ||
| const mockCredentials = { | ||
| token: 'expired-token', | ||
| userId: 'user-expired', | ||
| expiresAt: expiredTimestamp, | ||
| tokenType: 'standard', | ||
| savedAt: new Date().toISOString() | ||
| }; | ||
|
|
||
| vi.mocked(fs.existsSync).mockReturnValue(true); | ||
| vi.mocked(fs.readFileSync).mockReturnValue( | ||
| JSON.stringify(mockCredentials) | ||
| ); | ||
|
|
||
| const result = store.getCredentials(); | ||
|
|
||
| expect(result).toBeNull(); | ||
| expect(mockLogger.warn).toHaveBeenCalledWith( | ||
| expect.stringContaining('Authentication token has expired'), | ||
| expect.any(Object) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add boundary test for clock skew (tokens expiring within 30s).
Cover the “about to expire” window to prevent regressions around CLOCK_SKEW_MS.
Apply this diff near the existing expiry test:
+ it('should treat tokens expiring within the skew window as expired', () => {
+ const soon = Date.now() + 29_000; // within 30s skew
+ const mockCredentials = {
+ token: 'soon-expiring',
+ userId: 'user-soon',
+ expiresAt: soon,
+ tokenType: 'standard',
+ savedAt: new Date().toISOString()
+ };
+ vi.mocked(fs.existsSync).mockReturnValue(true);
+ vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(mockCredentials));
+ const result = store.getCredentials();
+ expect(result).toBeNull();
+ expect(mockLogger.warn).toHaveBeenCalledWith(
+ expect.stringContaining('Authentication token has expired'),
+ expect.any(Object)
+ );
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should check token expiration correctly', () => { | |
| const expiredTimestamp = Date.now() - 3600000; // 1 hour ago | |
| const mockCredentials = { | |
| token: 'expired-token', | |
| userId: 'user-expired', | |
| expiresAt: expiredTimestamp, | |
| tokenType: 'standard', | |
| savedAt: new Date().toISOString() | |
| }; | |
| vi.mocked(fs.existsSync).mockReturnValue(true); | |
| vi.mocked(fs.readFileSync).mockReturnValue( | |
| JSON.stringify(mockCredentials) | |
| ); | |
| const result = store.getCredentials(); | |
| expect(result).toBeNull(); | |
| expect(mockLogger.warn).toHaveBeenCalledWith( | |
| expect.stringContaining('Authentication token has expired'), | |
| expect.any(Object) | |
| ); | |
| }); | |
| it('should check token expiration correctly', () => { | |
| const expiredTimestamp = Date.now() - 3600000; // 1 hour ago | |
| const mockCredentials = { | |
| token: 'expired-token', | |
| userId: 'user-expired', | |
| expiresAt: expiredTimestamp, | |
| tokenType: 'standard', | |
| savedAt: new Date().toISOString() | |
| }; | |
| vi.mocked(fs.existsSync).mockReturnValue(true); | |
| vi.mocked(fs.readFileSync).mockReturnValue( | |
| JSON.stringify(mockCredentials) | |
| ); | |
| const result = store.getCredentials(); | |
| expect(result).toBeNull(); | |
| expect(mockLogger.warn).toHaveBeenCalledWith( | |
| expect.stringContaining('Authentication token has expired'), | |
| expect.any(Object) | |
| ); | |
| }); | |
| it('should treat tokens expiring within the skew window as expired', () => { | |
| const soon = Date.now() + 29_000; // within 30s skew | |
| const mockCredentials = { | |
| token: 'soon-expiring', | |
| userId: 'user-soon', | |
| expiresAt: soon, | |
| tokenType: 'standard', | |
| savedAt: new Date().toISOString() | |
| }; | |
| vi.mocked(fs.existsSync).mockReturnValue(true); | |
| vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(mockCredentials)); | |
| const result = store.getCredentials(); | |
| expect(result).toBeNull(); | |
| expect(mockLogger.warn).toHaveBeenCalledWith( | |
| expect.stringContaining('Authentication token has expired'), | |
| expect.any(Object) | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In packages/tm-core/src/auth/credential-store.test.ts around lines 185 to 207,
add a boundary unit test that verifies tokens expiring within the CLOCK_SKEW_MS
window (e.g., within 30s) are treated as expired: create mockCredentials with
expiresAt = Date.now() + 25000 (or CLOCK_SKEW_MS - 5000), mock
fs.existsSync/readFileSync as in the existing test, call store.getCredentials(),
assert it returns null, and assert mockLogger.warn was called with a message
containing 'Authentication token has expired' (or similar) and an object; follow
the same test structure and expectations as the existing expiry test to ensure
consistent behavior around clock skew.
| it('should handle missing expiresAt when saving', () => { | ||
| const credentials: AuthCredentials = { | ||
| token: 'test-token', | ||
| userId: 'user-no-expiry', | ||
| tokenType: 'standard', | ||
| savedAt: new Date().toISOString() | ||
| // No expiresAt | ||
| }; | ||
|
|
||
| store.saveCredentials(credentials); | ||
|
|
||
| const writtenData = vi.mocked(fs.writeFileSync).mock | ||
| .calls[0][1] as string; | ||
| const parsed = JSON.parse(writtenData); | ||
| expect(parsed.expiresAt).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider asserting savedAt normalization too.
Confirm persisted savedAt is an ISO string (and that input object wasn’t mutated, which you already test).
Example:
const parsed = JSON.parse(writtenData);
expect(parsed.expiresAt).toBeUndefined();
+expect(typeof parsed.savedAt).toBe('string');
+expect(new Date(parsed.savedAt).toISOString()).toBe(parsed.savedAt);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should handle missing expiresAt when saving', () => { | |
| const credentials: AuthCredentials = { | |
| token: 'test-token', | |
| userId: 'user-no-expiry', | |
| tokenType: 'standard', | |
| savedAt: new Date().toISOString() | |
| // No expiresAt | |
| }; | |
| store.saveCredentials(credentials); | |
| const writtenData = vi.mocked(fs.writeFileSync).mock | |
| .calls[0][1] as string; | |
| const parsed = JSON.parse(writtenData); | |
| expect(parsed.expiresAt).toBeUndefined(); | |
| }); | |
| it('should handle missing expiresAt when saving', () => { | |
| const credentials: AuthCredentials = { | |
| token: 'test-token', | |
| userId: 'user-no-expiry', | |
| tokenType: 'standard', | |
| savedAt: new Date().toISOString() | |
| // No expiresAt | |
| }; | |
| store.saveCredentials(credentials); | |
| const writtenData = vi.mocked(fs.writeFileSync).mock | |
| .calls[0][1] as string; | |
| const parsed = JSON.parse(writtenData); | |
| expect(parsed.expiresAt).toBeUndefined(); | |
| expect(typeof parsed.savedAt).toBe('string'); | |
| expect(new Date(parsed.savedAt).toISOString()).toBe(parsed.savedAt); | |
| }); |
🤖 Prompt for AI Agents
In packages/tm-core/src/auth/credential-store.test.ts around lines 341 to 356,
add assertions to verify savedAt is persisted as an ISO string and that the
input object wasn’t mutated: after parsing writtenData assert parsed.savedAt is
a string and that new Date(parsed.savedAt).toISOString() === parsed.savedAt (or
simply equals the original credentials.savedAt if that was an ISO string), and
keep/ensure the existing assertion that the original credentials.savedAt remains
unchanged.
| describe('cleanupCorruptFiles', () => { | ||
| it('should remove old corrupt files', () => { | ||
| const now = Date.now(); | ||
| const oldFile = 'auth.json.corrupt-' + (now - 8 * 24 * 60 * 60 * 1000); // 8 days old | ||
| const newFile = 'auth.json.corrupt-' + (now - 1000); // 1 second old | ||
|
|
||
| vi.mocked(fs.existsSync).mockReturnValue(true); | ||
| vi.mocked(fs.readdirSync).mockReturnValue([ | ||
| { name: oldFile, isFile: () => true }, | ||
| { name: newFile, isFile: () => true }, | ||
| { name: 'auth.json', isFile: () => true } | ||
| ] as any); | ||
| vi.mocked(fs.statSync).mockImplementation((filePath) => { | ||
| if (filePath.includes(oldFile)) { | ||
| return { mtimeMs: now - 8 * 24 * 60 * 60 * 1000 } as any; | ||
| } | ||
| return { mtimeMs: now - 1000 } as any; | ||
| }); | ||
| vi.mocked(fs.unlinkSync).mockImplementation(() => undefined); | ||
|
|
||
| store.cleanupCorruptFiles(); | ||
|
|
||
| expect(fs.unlinkSync).toHaveBeenCalledWith( | ||
| expect.stringContaining(oldFile) | ||
| ); | ||
| expect(fs.unlinkSync).not.toHaveBeenCalledWith( | ||
| expect.stringContaining(newFile) | ||
| ); | ||
| }); | ||
|
|
||
| it('should handle cleanup errors gracefully', () => { | ||
| vi.mocked(fs.existsSync).mockReturnValue(true); | ||
| vi.mocked(fs.readdirSync).mockImplementation(() => { | ||
| throw new Error('Permission denied'); | ||
| }); | ||
|
|
||
| // Should not throw | ||
| expect(() => store.cleanupCorruptFiles()).not.toThrow(); | ||
| expect(mockLogger.debug).toHaveBeenCalledWith( | ||
| expect.stringContaining('Error during corrupt file cleanup') | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add negative case: ignore non-numeric corrupt suffixes.
Ensure files like auth.json.corrupt-latest or auth.json.corrupt-123abc are skipped per /^\d+$/ rule.
Append this test:
+ it('should ignore corrupt files with non-numeric suffix', () => {
+ vi.mocked(fs.existsSync).mockReturnValue(true);
+ vi.mocked(fs.readdirSync).mockReturnValue([
+ { name: 'auth.json.corrupt-latest', isFile: () => true },
+ { name: 'auth.json.corrupt-123abc', isFile: () => true }
+ ] as any);
+ store.cleanupCorruptFiles();
+ expect(fs.unlinkSync).not.toHaveBeenCalled();
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('cleanupCorruptFiles', () => { | |
| it('should remove old corrupt files', () => { | |
| const now = Date.now(); | |
| const oldFile = 'auth.json.corrupt-' + (now - 8 * 24 * 60 * 60 * 1000); // 8 days old | |
| const newFile = 'auth.json.corrupt-' + (now - 1000); // 1 second old | |
| vi.mocked(fs.existsSync).mockReturnValue(true); | |
| vi.mocked(fs.readdirSync).mockReturnValue([ | |
| { name: oldFile, isFile: () => true }, | |
| { name: newFile, isFile: () => true }, | |
| { name: 'auth.json', isFile: () => true } | |
| ] as any); | |
| vi.mocked(fs.statSync).mockImplementation((filePath) => { | |
| if (filePath.includes(oldFile)) { | |
| return { mtimeMs: now - 8 * 24 * 60 * 60 * 1000 } as any; | |
| } | |
| return { mtimeMs: now - 1000 } as any; | |
| }); | |
| vi.mocked(fs.unlinkSync).mockImplementation(() => undefined); | |
| store.cleanupCorruptFiles(); | |
| expect(fs.unlinkSync).toHaveBeenCalledWith( | |
| expect.stringContaining(oldFile) | |
| ); | |
| expect(fs.unlinkSync).not.toHaveBeenCalledWith( | |
| expect.stringContaining(newFile) | |
| ); | |
| }); | |
| it('should handle cleanup errors gracefully', () => { | |
| vi.mocked(fs.existsSync).mockReturnValue(true); | |
| vi.mocked(fs.readdirSync).mockImplementation(() => { | |
| throw new Error('Permission denied'); | |
| }); | |
| // Should not throw | |
| expect(() => store.cleanupCorruptFiles()).not.toThrow(); | |
| expect(mockLogger.debug).toHaveBeenCalledWith( | |
| expect.stringContaining('Error during corrupt file cleanup') | |
| ); | |
| }); | |
| }); | |
| describe('cleanupCorruptFiles', () => { | |
| it('should remove old corrupt files', () => { | |
| const now = Date.now(); | |
| const oldFile = 'auth.json.corrupt-' + (now - 8 * 24 * 60 * 60 * 1000); // 8 days old | |
| const newFile = 'auth.json.corrupt-' + (now - 1000); // 1 second old | |
| vi.mocked(fs.existsSync).mockReturnValue(true); | |
| vi.mocked(fs.readdirSync).mockReturnValue([ | |
| { name: oldFile, isFile: () => true }, | |
| { name: newFile, isFile: () => true }, | |
| { name: 'auth.json', isFile: () => true } | |
| ] as any); | |
| vi.mocked(fs.statSync).mockImplementation((filePath) => { | |
| if (filePath.includes(oldFile)) { | |
| return { mtimeMs: now - 8 * 24 * 60 * 60 * 1000 } as any; | |
| } | |
| return { mtimeMs: now - 1000 } as any; | |
| }); | |
| vi.mocked(fs.unlinkSync).mockImplementation(() => undefined); | |
| store.cleanupCorruptFiles(); | |
| expect(fs.unlinkSync).toHaveBeenCalledWith( | |
| expect.stringContaining(oldFile) | |
| ); | |
| expect(fs.unlinkSync).not.toHaveBeenCalledWith( | |
| expect.stringContaining(newFile) | |
| ); | |
| }); | |
| it('should handle cleanup errors gracefully', () => { | |
| vi.mocked(fs.existsSync).mockReturnValue(true); | |
| vi.mocked(fs.readdirSync).mockImplementation(() => { | |
| throw new Error('Permission denied'); | |
| }); | |
| // Should not throw | |
| expect(() => store.cleanupCorruptFiles()).not.toThrow(); | |
| expect(mockLogger.debug).toHaveBeenCalledWith( | |
| expect.stringContaining('Error during corrupt file cleanup') | |
| ); | |
| }); | |
| it('should ignore corrupt files with non-numeric suffix', () => { | |
| vi.mocked(fs.existsSync).mockReturnValue(true); | |
| vi.mocked(fs.readdirSync).mockReturnValue([ | |
| { name: 'auth.json.corrupt-latest', isFile: () => true }, | |
| { name: 'auth.json.corrupt-123abc', isFile: () => true } | |
| ] as any); | |
| store.cleanupCorruptFiles(); | |
| expect(fs.unlinkSync).not.toHaveBeenCalled(); | |
| }); | |
| }); |
| export interface StorageSettings | ||
| extends Omit<RuntimeStorageConfig, 'apiConfigured'> { | ||
| /** Base path for file storage */ | ||
| basePath?: string; | ||
| /** | ||
| * Indicates whether API is configured | ||
| * @computed Derived automatically from presence of apiEndpoint or apiAccessToken | ||
| * @internal Should not be set manually in user config - computed by ConfigManager | ||
| */ | ||
| readonly apiConfigured?: boolean; | ||
| /** Enable automatic backups */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid redundant basePath declaration; keep computed flag optional/readonly.
- basePath is already inherited from Omit<RuntimeStorageConfig, 'apiConfigured'>; redeclaring it is noise.
export interface StorageSettings
extends Omit<RuntimeStorageConfig, 'apiConfigured'> {
- /** Base path for file storage */
- basePath?: string;
/**
* Indicates whether API is configured
* @computed Derived automatically from presence of apiEndpoint or apiAccessToken
* @internal Should not be set manually in user config - computed by ConfigManager
*/
readonly apiConfigured?: boolean;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface StorageSettings | |
| extends Omit<RuntimeStorageConfig, 'apiConfigured'> { | |
| /** Base path for file storage */ | |
| basePath?: string; | |
| /** | |
| * Indicates whether API is configured | |
| * @computed Derived automatically from presence of apiEndpoint or apiAccessToken | |
| * @internal Should not be set manually in user config - computed by ConfigManager | |
| */ | |
| readonly apiConfigured?: boolean; | |
| /** Enable automatic backups */ | |
| export interface StorageSettings | |
| extends Omit<RuntimeStorageConfig, 'apiConfigured'> { | |
| /** | |
| * Indicates whether API is configured | |
| * @computed Derived automatically from presence of apiEndpoint or apiAccessToken | |
| * @internal Should not be set manually in user config - computed by ConfigManager | |
| */ | |
| readonly apiConfigured?: boolean; | |
| /** Enable automatic backups */ | |
| // …rest of properties… | |
| } |
🤖 Prompt for AI Agents
In packages/tm-core/src/interfaces/configuration.interface.ts around lines 109
to 119, the StorageSettings interface redundantly redeclares basePath which is
already inherited from Omit<RuntimeStorageConfig, 'apiConfigured'>; remove the
explicit basePath declaration and its comment to avoid duplication, leaving only
the readonly optional apiConfigured property and any other unique properties;
ensure JSDoc for apiConfigured remains intact to indicate it is computed and
internal.
| const nextStorage: StorageSettings = { | ||
| ...(config.storage as StorageSettings), | ||
| type: 'api', | ||
| apiAccessToken: credentials.token, | ||
| apiEndpoint: | ||
| config.storage?.apiEndpoint || | ||
| process.env.HAMSTER_API_URL || | ||
| 'https://tryhamster.com/api' | ||
| } as any; // Cast to any to bypass strict type checking for partial config | ||
| }; | ||
| config.storage = nextStorage; | ||
| } |
There was a problem hiding this comment.
Fix runtime crash: spreading possibly undefined config.storage.
If config.storage is undefined, object spread throws. Use a safe base. Also assert token presence.
Apply:
- const nextStorage: StorageSettings = {
- ...(config.storage as StorageSettings),
- type: 'api',
- apiAccessToken: credentials.token,
- apiEndpoint:
- config.storage?.apiEndpoint ||
- process.env.HAMSTER_API_URL ||
- 'https://tryhamster.com/api'
- };
- config.storage = nextStorage;
+ if (!credentials.token) {
+ throw new TaskMasterError('Authenticated but missing access token', ERROR_CODES.MISSING_CONFIGURATION, { storageType: 'api' });
+ }
+ const base = (config.storage ?? {}) as Partial<StorageSettings>;
+ const nextStorage: StorageSettings = {
+ ...base,
+ type: 'api',
+ apiAccessToken: credentials.token!,
+ apiEndpoint:
+ base.apiEndpoint ??
+ process.env.HAMSTER_API_URL ??
+ 'https://tryhamster.com/api'
+ } as StorageSettings;
+ config.storage = nextStorage;Repeat similarly for the block below:
- const nextStorage: StorageSettings = {
- ...(config.storage as StorageSettings),
- type: 'api',
- apiAccessToken: credentials.token,
- apiEndpoint:
- config.storage?.apiEndpoint ||
- process.env.HAMSTER_API_URL ||
- 'https://tryhamster.com/api'
- };
- config.storage = nextStorage;
+ if (!credentials.token) {
+ throw new TaskMasterError('Authenticated but missing access token', ERROR_CODES.MISSING_CONFIGURATION, { storageType: 'api' });
+ }
+ const base = (config.storage ?? {}) as Partial<StorageSettings>;
+ const nextStorage: StorageSettings = {
+ ...base,
+ type: 'api',
+ apiAccessToken: credentials.token!,
+ apiEndpoint:
+ base.apiEndpoint ??
+ process.env.HAMSTER_API_URL ??
+ 'https://tryhamster.com/api'
+ } as StorageSettings;
+ config.storage = nextStorage;Also applies to: 108-118
🤖 Prompt for AI Agents
In packages/tm-core/src/storage/storage-factory.ts around lines 78-88 (and
likewise around 108-118), the code spreads config.storage which may be undefined
and reads credentials.token without asserting it; change the spread to use a
safe base object (e.g. {...(config.storage || {})}) and validate that
credentials.token exists before assigning apiAccessToken (throw or return an
explicit error if missing), then set apiEndpoint the same way; apply the same
safe-spread and token-assertion fix to the later block at 108-118.
| "skipLibCheck": true, | ||
| "forceConsistentCasingInFileNames": true, | ||
| "moduleResolution": "node", | ||
| "moduleResolution": "bundler", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Switch to bundler resolution: good, but tighten ESM settings and verify path aliases at runtime.
- OK to switch; matches explicit .js imports and Vitest/Vite pipelines.
- Add these for safer ESM interop and DX:
- "verbatimModuleSyntax": true
- "moduleDetection": "force"
- If Node types are used in tm-core (e.g., BufferEncoding in configuration.interface.ts), add "types": ["node"] here.
"forceConsistentCasingInFileNames": true,
"moduleResolution": "bundler",
+ "verbatimModuleSyntax": true,
+ "moduleDetection": "force",
+ "types": ["node"],
"resolveJsonModule": true,Run to ensure no extensionless relative imports remain (required under bundler for runtime ESM):
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -g 'packages/tm-core/src/**' \
-e 'from\s+[\"\'](\.{1,2}\/[^\"\'']+)[\"\'']' | \
grep -vE '\.(js|mjs|cjs|json)[\"\'']$' || echo "OK: all relative imports have explicit extensions"Length of output: 234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all TS relative imports and filter out those with explicit .js/.mjs/.cjs/.json extensions
rg -nP --type=ts -g 'packages/tm-core/src/**' "from\s+['\"]\.\.?/[^'\"]+['\"]" \
| grep -vP "\.(js|mjs|cjs|json)['\"]$" \
|| echo "OK: all relative imports have explicit extensions"Length of output: 15832
Tighten ESM settings and add explicit extensions to all relative imports
- In packages/tm-core/tsconfig.json under compilerOptions, add:
"forceConsistentCasingInFileNames": true, "moduleResolution": "bundler", + "verbatimModuleSyntax": true, + "moduleDetection": "force", + "types": ["node"], "resolveJsonModule": true,
- Runtime ESM (with bundler resolution) requires file extensions on all relative imports. Update every import in packages/tm-core/src that lacks an explicit extension (e.g. change
import … from './id-generator';
to
import … from './id-generator.js';).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "moduleResolution": "bundler", | |
| "forceConsistentCasingInFileNames": true, | |
| "moduleResolution": "bundler", | |
| "verbatimModuleSyntax": true, | |
| "moduleDetection": "force", | |
| "types": ["node"], | |
| "resolveJsonModule": true, |
🤖 Prompt for AI Agents
In packages/tm-core/tsconfig.json around line 26, you set "moduleResolution":
"bundler" which enforces runtime ESM semantics; update every relative import in
packages/tm-core/src to include explicit file extensions (e.g. change import
from './id-generator' to './id-generator.js'), ensuring both import and dynamic
import paths use .js, and confirm any index imports use './folder/index.js' or
'./folder/index.js' as appropriate; run a full compile and adjust any path
aliases or build tooling (rollup/tsup) to preserve .js extensions in emitted
output so runtime resolves correctly.
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
Breaking Changes
Bug Fixes
Tests