feat: handle new command errors better#1318
Conversation
|
WalkthroughCentralizes CLI error handling via displayError, adds storage-aware command headers (displayCommandHeader), exposes storage/brief/org info in TaskMasterCore, introduces NO_BRIEF_SELECTED error code, updates API storage and task-service error propagation, and surfaces orgSlug in user context. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant TM as TaskMasterCore
participant EH as displayError
note right of CLI: Header & storage info flow
CLI->>TM: getStorageDisplayInfo()
TM-->>CLI: { briefId, briefName, orgSlug } or null
CLI->>CLI: displayCommandHeader(...)
alt Error occurs in command
CLI->>EH: displayError(error, options?)
EH-->>CLI: prints message (+stack if debug/forced)
opt skipExit not set
EH-->>System: process.exit(1)
end
end
sequenceDiagram
participant API_Method as api-storage method
participant AuthMgr as AuthManager
participant Remote as Remote API
participant TMErr as TaskMasterError
API_Method->>AuthMgr: getContext() (sync)
alt no brief selected
AuthMgr-->>API_Method: context without briefId
API_Method->>API_Method: throw TaskMasterError(NO_BRIEF_SELECTED)
else brief present
AuthMgr-->>API_Method: context with briefId
API_Method->>Remote: perform API call
alt API error (not NO_BRIEF_SELECTED)
Remote-->>API_Method: error
API_Method->>API_Method: wrap and throw TaskMasterError(with op & context)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)packages/tm-core/src/storage/api-storage.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
Comment |
87ac012 to
90c0211
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- show brief and brief url when listing tasks
90c0211 to
0bbe7f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/tm-core/src/storage/api-storage.ts (2)
126-151: Improve error handling specificity inloadTagsIntoCache.The catch block at lines 147-150 logs "No brief selected, starting with empty cache" for any error, but errors could originate from sources other than missing brief selection (e.g.,
AuthManagerfailures, unexpected exceptions).Apply this diff to make error handling more specific:
private async loadTagsIntoCache(): Promise<void> { try { const authManager = AuthManager.getInstance(); const context = authManager.getContext(); // If we have a selected brief, create a virtual "tag" for it if (context?.briefId) { // Create a virtual tag representing the current brief const briefTag: TaskTag = { name: context.briefId, tasks: [], // Will be populated when tasks are loaded metadata: { briefId: context.briefId, briefName: context.briefName, organizationId: context.orgId } }; this.tagsCache.clear(); this.tagsCache.set(context.briefId, briefTag); + } else { + console.debug('No brief selected, starting with empty cache'); } } catch (error) { - // If no brief is selected, that's okay - user needs to select one first - console.debug('No brief selected, starting with empty cache'); + console.error('Error loading tags into cache:', error); + // Continue with empty cache } }
361-385: Consider optimizing redundant context retrieval.The method retrieves context at line 366, then calls
loadTagsIntoCache()at line 371, which retrieves context again. While not a functional issue, this is slightly inefficient.Consider passing the context to
loadTagsIntoCacheto avoid redundant retrieval, or inline the cache update logic here since it's simple.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
apps/cli/src/commands/auth.command.ts(6 hunks)apps/cli/src/commands/context.command.ts(10 hunks)apps/cli/src/commands/export.command.ts(2 hunks)apps/cli/src/commands/list.command.ts(3 hunks)apps/cli/src/commands/next.command.ts(3 hunks)apps/cli/src/commands/set-status.command.ts(4 hunks)apps/cli/src/commands/show.command.ts(4 hunks)apps/cli/src/commands/start.command.ts(2 hunks)apps/cli/src/index.ts(1 hunks)apps/cli/src/ui/components/header.component.ts(1 hunks)apps/cli/src/utils/display-helpers.ts(1 hunks)apps/cli/src/utils/error-handler.ts(1 hunks)packages/tm-core/src/auth/types.ts(1 hunks)packages/tm-core/src/errors/task-master-error.ts(1 hunks)packages/tm-core/src/services/task-service.ts(3 hunks)packages/tm-core/src/storage/api-storage.ts(7 hunks)packages/tm-core/src/task-master-core.ts(1 hunks)scripts/modules/commands.js(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-31T22:07:49.716Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/commands.mdc:0-0
Timestamp: 2025-07-31T22:07:49.716Z
Learning: Applies to scripts/modules/commands.js : Provide specific error handling for common issues, include troubleshooting hints for each error type, and use consistent error formatting across all commands.
Applied to files:
apps/cli/src/utils/error-handler.tsscripts/modules/commands.js
📚 Learning: 2025-07-18T17:14:54.131Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/telemetry.mdc:0-0
Timestamp: 2025-07-18T17:14:54.131Z
Learning: Applies to scripts/modules/task-manager/**/*.js : If the core logic function handles CLI output (outputFormat === 'text' or 'cli'), and aiServiceResponse.telemetryData is available, it must call displayAiUsageSummary(aiServiceResponse.telemetryData, 'cli') from scripts/modules/ui.js.
Applied to files:
apps/cli/src/commands/list.command.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/commands.js : Provide clear error messages for common failures, handle tagged system migration errors gracefully, include suggestions for resolution, and exit with appropriate codes for scripting in CLI commands.
Applied to files:
scripts/modules/commands.js
📚 Learning: 2025-07-18T17:14:54.131Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/telemetry.mdc:0-0
Timestamp: 2025-07-18T17:14:54.131Z
Learning: Applies to scripts/modules/commands.js : CLI command definitions in scripts/modules/commands.js must call the appropriate core logic function with outputFormat: 'text', and must not call displayAiUsageSummary directly if the core logic function already handles it.
Applied to files:
scripts/modules/commands.js
📚 Learning: 2025-07-31T22:07:49.716Z
Learnt from: CR
PR: eyaltoledano/claude-task-master#0
File: .cursor/rules/commands.mdc:0-0
Timestamp: 2025-07-31T22:07:49.716Z
Learning: Applies to scripts/modules/commands.js : Integrate version checking in the CLI run function, starting the update check in the background and displaying notifications after command execution.
Applied to files:
scripts/modules/commands.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (22)
packages/tm-core/src/auth/types.ts (1)
19-19: LGTM! Clean type extension for organization slug.The addition of the optional
orgSlugfield toUserContextis a non-breaking change that enables propagating organization scope through storage/display logic as described in the PR objectives.apps/cli/src/utils/error-handler.ts (1)
14-16: LGTM! Clean debug mode detection.The
isDebugMode()function correctly checks for explicit debug flags ('true' or '1') in the DEBUG environment variable.scripts/modules/commands.js (2)
22-24: LGTM! Centralized error handling import.Adding
displayErrorto the public imports from@tm/clienables consistent error display across the CLI.
5160-5160: LGTM! Consistent error routing.Delegating to
displayError(error)for non-ConfigurationError failures maintains consistent error presentation throughout the CLI.apps/cli/src/index.ts (1)
27-29: LGTM! Clean public API extension.Re-exporting the centralized error handling utilities (
displayError,isDebugMode) makes them available to other CLI components and external consumers.apps/cli/src/commands/start.command.ts (2)
19-19: LGTM! Centralized error handling import.Importing
displayErrorfrom the error-handler module enables consistent error display in the start command.
160-165: LGTM! Clean error handling migration.Replacing the local
handleErrorwith centralizeddisplayErrormaintains consistency across CLI commands. The spinner is properly stopped before error display.packages/tm-core/src/services/task-service.ts (4)
164-172: LGTM! Proper error propagation for user-facing errors.The catch block correctly identifies
NO_BRIEF_SELECTEDerrors as user-facing and re-throws them without logging or wrapping, preventing double-wrapping and preserving the original error context for CLI display.
199-206: LGTM! Consistent error handling pattern.The getTask method applies the same error propagation pattern as getTaskList, correctly short-circuiting user-facing errors.
543-550: LGTM! Consistent error handling pattern.The updateTaskStatus method applies the same error propagation pattern, maintaining consistency across the service.
164-172: NO_BRIEF_SELECTED error code is defined and used consistently. Definition found intask-master-error.tsand usage verified across service and storage layers.apps/cli/src/commands/list.command.ts (4)
20-21: LGTM! Proper imports for centralized utilities.Adding imports for
displayErroranddisplayCommandHeaderenables consistent error handling and header rendering across CLI commands.
54-54: LGTM! Type-safe storage type field.Adding
storageType: Exclude<StorageType, 'auto'>to the result interface correctly excludes the 'auto' sentinel value that's only valid in configuration.
110-110: LGTM! Clean error handling migration.Replacing local error handling with centralized
displayErrormaintains consistency across CLI commands.
254-260: LGTM! Centralized header rendering.Using
displayCommandHeaderwith storage type provides consistent header formatting across commands and enables storage-type-aware display logic.apps/cli/src/commands/export.command.ts (4)
10-10: LGTM! Clean import consolidation.Consolidating the auth imports into a single line improves readability without changing functionality.
13-13: LGTM! Centralized error handling import.Importing
displayErrorenables consistent error display in the export command.
197-197: LGTM! Clean error handling migration.Replacing local error handling with centralized
displayErrormaintains consistency. The spinner state is properly checked before failing.
336-336: Confirm necessity of public visibility forgetLastResult()
No usages found in application or tests; if exposing this method to external consumers or for testing is intended, add appropriate documentation or tests—otherwise revert its visibility toprivate.apps/cli/src/utils/display-helpers.ts (1)
13-32: LGTM!The URL construction logic correctly handles protocol detection and applies appropriate defaults (http for localhost, https otherwise).
packages/tm-core/src/storage/api-storage.ts (2)
210-245: Verify inconsistent brief selection checks.Read operations (
loadTasks,loadTask,updateTaskStatus) now explicitly check forcontext?.briefIdand throwNO_BRIEF_SELECTEDerrors, but write operations likesaveTasks(line 210),saveTask(line 294),deleteTask(line 332), andupdateTask(line 519) do not perform the same check.This inconsistency could lead to confusing error messages when users attempt write operations without a selected brief. The underlying repository calls may fail with less helpful error messages.
Should write operations also check for a selected brief, or is there a different error-handling strategy for them?
129-129: Confirm synchronous context retrieval: AuthManager.getContext() returnsUserContext | nullsynchronously, so removingawaitin all call sites is valid.
| const filePath = | ||
| storageType === 'file' && tmCore | ||
| ? `.taskmaster/tasks/tasks.json` | ||
| : undefined; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid hardcoding the file path.
The file path .taskmaster/tasks/tasks.json is hardcoded. If tmCore is provided, query it for the actual file path to ensure accuracy and maintainability, especially if storage configurations vary.
Consider retrieving the path from tmCore configuration:
// Get file path for display (only for file storage)
const filePath =
storageType === 'file' && tmCore
- ? `.taskmaster/tasks/tasks.json`
+ ? tmCore.getStorageConfig()?.filePath || '.taskmaster/tasks/tasks.json'
: undefined;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/cli/src/utils/display-helpers.ts around lines 61-64, the code hardcodes
`.taskmaster/tasks/tasks.json`; instead, when tmCore is present query tmCore for
the configured storage file path and use that value (falling back to undefined
if no path is exposed). Update the logic to: check tmCore for a public API or
config property (e.g., a method like getStoragePath() or a config field such as
config.storageFilePath/config.tasksPath), call or read it safely (handle promise
if async), validate the returned path, and assign that to filePath only when
storageType === 'file' and a path is available; otherwise keep filePath
undefined. Ensure null/undefined checks and no hardcoded literal remain.
| export function displayError( | ||
| error: any, | ||
| options: { | ||
| /** Skip exit, useful when caller wants to handle exit */ | ||
| skipExit?: boolean; | ||
| /** Force show stack trace regardless of debug mode */ | ||
| forceStack?: boolean; | ||
| } = {} | ||
| ): void { | ||
| // Check if it's a TaskMasterError with sanitized details | ||
| if (error?.getSanitizedDetails) { | ||
| const sanitized = error.getSanitizedDetails(); | ||
| console.error(chalk.red(`\n${sanitized.message}`)); | ||
|
|
||
| // Show stack trace in debug mode or if forced | ||
| if ((isDebugMode() || options.forceStack) && error.stack) { | ||
| console.error(chalk.gray('\nStack trace:')); | ||
| console.error(chalk.gray(error.stack)); | ||
| } | ||
| } else { | ||
| // For other errors, show the message | ||
| const message = error?.message ?? String(error); | ||
| console.error(chalk.red(`\nError: ${message}`)); | ||
|
|
||
| // Show stack trace in debug mode or if forced | ||
| if ((isDebugMode() || options.forceStack) && error?.stack) { | ||
| console.error(chalk.gray('\nStack trace:')); | ||
| console.error(chalk.gray(error.stack)); | ||
| } | ||
| } | ||
|
|
||
| // Exit if not skipped | ||
| if (!options.skipExit) { | ||
| process.exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify consistent error exit behavior across commands.
The displayError function provides centralized error handling with optional stack traces and process exit. However, ensure that all calling commands are aware of the default process.exit(1) behavior and use skipExit: true when they need to handle cleanup or additional logic after displaying errors.
Run the following script to verify how commands call displayError:
🏁 Script executed:
#!/bin/bash
# Description: Find all displayError calls and check if any need skipExit
# Expect: Commands that need cleanup should use skipExit option
rg -n "displayError\(" --type ts -A 2 -B 2Length of output: 12189
🏁 Script executed:
#!/bin/bash
rg -n "finally" --type ts apps/cli/src/commandsLength of output: 116
Add skipExit to displayError in commands with finally blocks
- apps/cli/src/commands/next.command.ts (line 80): change to
displayError(error, { skipExit: true }) - apps/cli/src/commands/set-status.command.ts (line 185): change to
displayError(error, { skipExit: true })
🤖 Prompt for AI Agents
In apps/cli/src/utils/error-handler.ts lines 25 to 60: update the call sites in
the commands so they pass skipExit: true to displayError; specifically, in
apps/cli/src/commands/next.command.ts around line 80 replace displayError(error)
with displayError(error, { skipExit: true }) and in
apps/cli/src/commands/set-status.command.ts around line 185 replace
displayError(error) with displayError(error, { skipExit: true }) so finally
blocks can run without the process exiting.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cli/src/commands/set-status.command.ts (1)
167-179: Do not emit success output when a task update failsLine 140 sets
hasError = true, but Lines 167-178 still mark the command result assuccess: trueand calldisplayResults. As written, a failing task will print a “success” payload (and, in JSON mode, a second contradictory JSON object) before exiting with code 1. That’s confusing for users and breaks consumers expecting a single, truthful JSON response. Please gate the success path on!hasErrorso we only persist/displaylastResultwhen every task update succeeds.- // Store result for potential reuse - this.lastResult = { - success: true, - updatedTasks, - storageType: this.tmCore.getStorageType() as Exclude< - StorageType, - 'auto' - > - }; - - // Display results - this.displayResults(this.lastResult, options); + if (!hasError) { + // Store result for potential reuse + this.lastResult = { + success: true, + updatedTasks, + storageType: this.tmCore.getStorageType() as Exclude< + StorageType, + 'auto' + > + }; + + // Display results + this.displayResults(this.lastResult, options); + }
♻️ Duplicate comments (1)
apps/cli/src/utils/display-helpers.ts (1)
63-66: Address the hardcoded file path.The file path
.taskmaster/tasks/tasks.jsonis hardcoded despitetmCore.getStorageConfig()being available (as mentioned in related changes). While the comment notes this is display-only, querying the actual configured path improves accuracy and maintainability if storage configuration ever changes.Apply this diff to use the configured path:
// Get file path for display (only for file storage) - // Note: The file structure is fixed for file storage and won't change. - // This is a display-only relative path, not used for actual file operations. const filePath = storageType === 'file' && tmCore - ? `.taskmaster/tasks/tasks.json` + ? tmCore.getStorageConfig()?.filePath || '.taskmaster/tasks/tasks.json' : undefined;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apps/cli/src/commands/next.command.ts(4 hunks)apps/cli/src/commands/set-status.command.ts(5 hunks)apps/cli/src/utils/display-helpers.ts(1 hunks)packages/tm-core/src/storage/api-storage.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/tm-core/src/storage/api-storage.ts (2)
packages/tm-core/src/auth/auth-manager.ts (1)
AuthManager(26-286)packages/tm-core/src/errors/task-master-error.ts (2)
TaskMasterError(133-332)ERROR_CODES(9-59)
apps/cli/src/commands/set-status.command.ts (2)
apps/cli/src/utils/error-handler.ts (1)
displayError(25-60)apps/cli/src/utils/ui.ts (1)
displayError(192-208)
apps/cli/src/commands/next.command.ts (2)
apps/cli/src/utils/error-handler.ts (1)
displayError(25-60)apps/cli/src/utils/display-helpers.ts (1)
displayCommandHeader(38-75)
apps/cli/src/utils/display-helpers.ts (2)
packages/tm-core/src/types/index.ts (1)
StorageType(11-11)apps/cli/src/ui/components/header.component.ts (2)
BriefInfo(11-16)displayHeader(32-81)
🪛 GitHub Actions: CI
packages/tm-core/src/storage/api-storage.ts
[error] 169-169: TypeScript compile error: Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
🔇 Additional comments (4)
apps/cli/src/commands/next.command.ts (2)
61-92: LGTM! Clean error handling with proper cleanup.The error handling pattern (catch → display without exit → cleanup → exit) ensures resources are released even on error, which is correct.
177-180: LGTM! Centralized header rendering.Using
displayCommandHeaderaligns with the broader refactoring to consolidate display logic and correctly propagatesstorageTypefor API vs. file storage contexts.packages/tm-core/src/storage/api-storage.ts (2)
129-129: LGTM! Correct removal ofawait.
AuthManager.getContext()is synchronous (returnsUserContext | nulldirectly), so removingawaitis correct.Also applies to: 320-320
766-787: LGTM! Excellent refactoring to reduce duplication.The
ensureBriefSelected()andwrapError()helper methods successfully address the past review comments by eliminating duplicated context checks and error-wrapping logic across multiple methods.Also applies to: 808-830
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/tm-core/src/storage/api-storage.ts (1)
770-787: Add explicit return type to fix TypeScript error.The missing return type annotation causes TypeScript to infer that
briefIdmight be optional in the returned context, which requires non-null assertions at call sites (e.g., line 169). The past review flagged this as the root cause of a pipeline failure and remains unaddressed.Apply this diff to add a type alias and explicit return type:
+type ContextWithBrief = NonNullable<ReturnType<typeof AuthManager.prototype.getContext>> & { briefId: string }; + /** * Ensure a brief is selected in the current context * @returns The current auth context with a valid briefId */ - private ensureBriefSelected(operation: string) { + private ensureBriefSelected(operation: string): ContextWithBrief { const authManager = AuthManager.getInstance(); const context = authManager.getContext(); if (!context?.briefId) { throw new TaskMasterError( 'No brief selected', ERROR_CODES.NO_BRIEF_SELECTED, { operation, userMessage: 'No brief selected. Please select a brief first using: tm context brief <brief-id> or tm context brief <brief-url>' } ); } - return context; + return context as ContextWithBrief; }Based on past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/tm-core/src/storage/api-storage.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tm-core/src/storage/api-storage.ts (1)
packages/tm-core/src/errors/task-master-error.ts (2)
TaskMasterError(133-332)ERROR_CODES(9-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (3)
packages/tm-core/src/storage/api-storage.ts (3)
231-241: LGTM!The validation pattern is correct:
ensureBriefSelectedis called for its side effect (throwing if no brief is selected), andwrapErrorproperly preservesNO_BRIEF_SELECTEDerrors while wrapping other exceptions.
505-548: LGTM!The validation and error handling pattern is consistent and correct throughout the method.
808-830: LGTM!The
wrapErrorhelper is correctly implemented and addresses the duplicate error-wrapping logic flagged in previous reviews. The logic properly preservesNO_BRIEF_SELECTEDerrors while wrapping other exceptions asSTORAGE_ERROR.
5ba7079 to
92ac3d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
packages/tm-core/src/storage/api-storage.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tm-core/src/storage/api-storage.ts (2)
packages/tm-core/src/auth/auth-manager.ts (1)
AuthManager(26-286)packages/tm-core/src/errors/task-master-error.ts (2)
TaskMasterError(133-332)ERROR_CODES(9-59)
🪛 GitHub Actions: CI
packages/tm-core/src/storage/api-storage.ts
[error] 41-45: Formatter would have printed the following content: type ContextWithBrief = NonNullable<ReturnType> & { briefId: string }; The formatter detected a formatting issue in this region. Run 'biome format .' to fix formatting.
92ac3d5 to
5128e70
Compare
This PR was automatically generated to update documentation based on recent changes. Original commit: feat: handle new command errors better (#1318)\n\n\n Co-authored-by: Claude <claude-assistant@anthropic.com>
Description
Related Issues
How to Test This
# Example commands or stepsExpected result:
Contributor Checklist
npm run changesetnpm testnpm run format-check(ornpm run formatto fix)Changelog Entry
For Maintainers
Summary by CodeRabbit
New Features
Bug Fixes
Refactor