Skip to content

feat: handle new command errors better#1318

Merged
Crunchyman-ralph merged 4 commits intonextfrom
ralph/fix/improve.display.errors.in.cli
Oct 16, 2025
Merged

feat: handle new command errors better#1318
Crunchyman-ralph merged 4 commits intonextfrom
ralph/fix/improve.display.errors.in.cli

Conversation

@Crunchyman-ralph
Copy link
Collaborator

@Crunchyman-ralph Crunchyman-ralph commented Oct 16, 2025

  • 🐛 Bug fix
  • ✨ Feature
  • 🔌 Integration
  • 📝 Docs
  • 🧹 Refactor
  • Other:

Description

  • show brief and brief url when listing tasks
  • handle errors better

Related Issues

How to Test This

# Example commands or steps

Expected result:

Contributor Checklist

  • Created changeset: npm run changeset
  • Tests pass: npm test
  • Format check passes: npm run format-check (or npm run format to fix)
  • Addressed CodeRabbit comments (if any)
  • Linked related issues (if any)
  • Manually tested the changes

Changelog Entry


For Maintainers

  • PR title follows conventional commits
  • Target branch correct
  • Labels added
  • Milestone assigned (if applicable)

Summary by CodeRabbit

  • New Features

    • Command headers now show storage type and brief details (including web links when available).
    • Organization slug is preserved in context so names and URLs are friendlier.
    • CLI exposes utilities to control error display and debug stack traces.
  • Bug Fixes

    • More consistent, user-friendly error messages; JSON outputs include sanitized per-task error details and final failure status.
  • Refactor

    • Command error handling centralized to a shared display pathway, reducing abrupt exits in many flows.

@changeset-bot
Copy link

changeset-bot bot commented Oct 16, 2025

⚠️ No Changeset found

Latest commit: 5128e70

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 16, 2025

Walkthrough

Centralizes 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

Cohort / File(s) Summary
Centralized CLI error handling
apps/cli/src/utils/error-handler.ts, apps/cli/src/index.ts, scripts/modules/commands.js
Add displayError and isDebugMode; re-export them; replace ad-hoc per-command handleError/console.error usages with displayError (supports skipExit/forceStack).
Commands — error handling & header usage
apps/cli/src/commands/auth.command.ts, apps/cli/src/commands/start.command.ts, apps/cli/src/commands/export.command.ts, apps/cli/src/commands/context.command.ts, apps/cli/src/commands/list.command.ts, apps/cli/src/commands/next.command.ts, apps/cli/src/commands/set-status.command.ts, apps/cli/src/commands/show.command.ts
Remove per-command private handleError helpers; replace catch paths with displayError (some with skipExit); propagate storageType and use displayCommandHeader for unified headers; set-status adds per-task JSON error output and deferred final exit; export.getLastResult() made public.
Display/header utilities
apps/cli/src/utils/display-helpers.ts, apps/cli/src/ui/components/header.component.ts
Add displayCommandHeader and getWebAppUrl; extend header component with BriefInfo and storageType to branch rendering for API (brief/url/orgSlug) vs file (tag/path) storage.
Core — auth/context type
packages/tm-core/src/auth/types.ts
Add optional orgSlug?: string to UserContext.
Core — error codes
packages/tm-core/src/errors/task-master-error.ts
Add NO_BRIEF_SELECTED to exported ERROR_CODES (and thus ErrorCode union).
Core — storage & services error flow
packages/tm-core/src/storage/api-storage.ts, packages/tm-core/src/services/task-service.ts
Switch to synchronous auth context access in API storage; add ensureBriefSelected and wrapError; explicitly propagate NO_BRIEF_SELECTED (user-facing) and wrap other errors as TaskMasterError with op/context metadata; ensure tags cache includes current brief.
Core — TaskMasterCore API additions
packages/tm-core/src/task-master-core.ts
Add getStorageConfig() and getStorageDisplayInfo() to expose storage config and brief/org display info for API storage.

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
Loading
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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • eyaltoledano

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "feat: handle new command errors better" directly addresses a significant portion of the changeset. The pull request introduces a centralized error handling system via a new displayError utility function in error-handler.ts, replaces scattered error handling patterns across multiple command files (auth, context, export, list, next, set-status, show, start commands), and removes redundant private error handling methods from individual commands. This represents a real and substantial improvement to error handling throughout the CLI. While the changeset also includes UI/display enhancements (brief info display, storage type propagation, header improvements), the error handling improvements are clearly a major focus of the changes and are accurately captured by the title.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ralph/fix/improve.display.errors.in.cli

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 92ac3d5 and 5128e70.

📒 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)
⏰ 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 (2)
packages/tm-core/src/storage/api-storage.ts (2)

168-187: Centralized brief validation looks solid.

Switching to ensureBriefSelected removes the non-null assertions and guarantees downstream code always has a real brief context before touching the repository. This tightens the happy-path while keeping the user-facing error message consistent.


818-836: wrapError helper keeps NO_BRIEF_SELECTED intact.

Reusing this helper across the catch blocks trims duplication and ensures we never lose the original NO_BRIEF_SELECTED signal while consistently wrapping other failures with STORAGE_ERROR metadata. Nicely done.


Comment @coderabbitai help to get the list of available commands and usage tips.

@Crunchyman-ralph Crunchyman-ralph force-pushed the ralph/fix/improve.display.errors.in.cli branch from 87ac012 to 90c0211 Compare October 16, 2025 17:27
@Crunchyman-ralph
Copy link
Collaborator Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 16, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

- show brief and brief url when listing tasks
@Crunchyman-ralph Crunchyman-ralph force-pushed the ralph/fix/improve.display.errors.in.cli branch from 90c0211 to 0bbe7f7 Compare October 16, 2025 17:30
@Crunchyman-ralph Crunchyman-ralph changed the title feat: implement runtime and build time env variables for remote access feat: handle new command errors better Oct 16, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in loadTagsIntoCache.

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., AuthManager failures, 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 loadTagsIntoCache to 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8649c8a and 0bbe7f7.

📒 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.ts
  • 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/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 orgSlug field to UserContext is 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 displayError to the public imports from @tm/cli enables 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 displayError from the error-handler module enables consistent error display in the start command.


160-165: LGTM! Clean error handling migration.

Replacing the local handleError with centralized displayError maintains 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_SELECTED errors 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 in task-master-error.ts and 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 displayError and displayCommandHeader enables 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 displayError maintains consistency across CLI commands.


254-260: LGTM! Centralized header rendering.

Using displayCommandHeader with 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 displayError enables consistent error display in the export command.


197-197: LGTM! Clean error handling migration.

Replacing local error handling with centralized displayError maintains consistency. The spinner state is properly checked before failing.


336-336: Confirm necessity of public visibility for getLastResult()
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 to private.

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 for context?.briefId and throw NO_BRIEF_SELECTED errors, but write operations like saveTasks (line 210), saveTask (line 294), deleteTask (line 332), and updateTask (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() returns UserContext | null synchronously, so removing await in all call sites is valid.

Comment on lines +61 to +64
const filePath =
storageType === 'file' && tmCore
? `.taskmaster/tasks/tasks.json`
: undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Comment on lines +25 to +60
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);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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 2

Length of output: 12189


🏁 Script executed:

#!/bin/bash
rg -n "finally" --type ts apps/cli/src/commands

Length 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 fails

Line 140 sets hasError = true, but Lines 167-178 still mark the command result as success: true and call displayResults. 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 !hasError so we only persist/display lastResult when 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.json is hardcoded despite tmCore.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

📥 Commits

Reviewing files that changed from the base of the PR and between 0bbe7f7 and 1698ee1.

📒 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 displayCommandHeader aligns with the broader refactoring to consolidate display logic and correctly propagates storageType for API vs. file storage contexts.

packages/tm-core/src/storage/api-storage.ts (2)

129-129: LGTM! Correct removal of await.

AuthManager.getContext() is synchronous (returns UserContext | null directly), so removing await is correct.

Also applies to: 320-320


766-787: LGTM! Excellent refactoring to reduce duplication.

The ensureBriefSelected() and wrapError() 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 briefId might 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1698ee1 and 5ba7079.

📒 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: ensureBriefSelected is called for its side effect (throwing if no brief is selected), and wrapError properly preserves NO_BRIEF_SELECTED errors while wrapping other exceptions.


505-548: LGTM!

The validation and error handling pattern is consistent and correct throughout the method.


808-830: LGTM!

The wrapError helper is correctly implemented and addresses the duplicate error-wrapping logic flagged in previous reviews. The logic properly preserves NO_BRIEF_SELECTED errors while wrapping other exceptions as STORAGE_ERROR.

@Crunchyman-ralph Crunchyman-ralph force-pushed the ralph/fix/improve.display.errors.in.cli branch from 5ba7079 to 92ac3d5 Compare October 16, 2025 20:18
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ba7079 and 92ac3d5.

📒 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.

@Crunchyman-ralph Crunchyman-ralph force-pushed the ralph/fix/improve.display.errors.in.cli branch from 92ac3d5 to 5128e70 Compare October 16, 2025 20:24
@Crunchyman-ralph Crunchyman-ralph merged commit 662e386 into next Oct 16, 2025
6 checks passed
github-actions bot added a commit that referenced this pull request Oct 16, 2025
  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>
sfc-gh-dflippo pushed a commit to sfc-gh-dflippo/task-master-ai that referenced this pull request Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant