ralph/feat.add.workflow

[Crunchyman-ralph]

• feat: create tm-core and apps/cli (chore: add tm-core package with tag and tasks.json #1093)
• feat: add oauth with remote server (feat: add oauth with remote server #1178)
• chore: address oauth PR concerns (chore: address oauth PR concerns #1184)
• wip

What type of PR is this?

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

Description

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

  • New @tm/cli with commands: tasks list, auth (login/logout/status/refresh), and workflow (start/list/status/stop).
  • OAuth-based authentication with token refresh and status.
  • Workflow engine integration for running tasks in isolated worktrees.
  • Core library (@tm/core) with config management and file/API storage.

• Improvements

  • Dist-based executables and streamlined startup (no sync FS reads).
  • Consistent CLI UI with colored output and multiple formats (text/json/compact).

• Documentation

  • Migration roadmap, CLI commander pattern, tm-core architecture, and package READMEs.

• Chores

  • Monorepo expansion, build scripts, TS/tsup configs, and VS Code formatter settings.

[changeset-bot]

⚠️ No Changeset found

Latest commit: d4826e0

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]

Walkthrough

Introduces a monorepo architecture with new @tm/core and @tm/workflow-engine packages and a new @tm/cli app. Adds core domain/types, config/auth/storage subsystems, TaskMasterCore facade, workflow orchestration, and multiple CLI commands (list, auth, workflow). Refactors scripts to consume new packages, updates packaging/build (tsup), and adds docs/reports.

Changes

Cohort / File(s)	Summary
Monorepo setup & packaging package.json, .vscode/settings.json, packages/*/tsconfig.json, packages/*/tsup.config.ts, packages/*/vitest.config.ts, apps/cli/*	Adds workspace entries, dist-based bins, build/dev scripts, uses tsup; configures TypeScript/formatters; creates @tm/cli package with build/test/lint setup and entrypoints.
Documentation & planning artifacts .taskmaster/docs/MIGRATION-ROADMAP.md, docs/CLI-COMMANDER-PATTERN.md, .taskmaster/reports/*, packages/tm-core/{README.md,CHANGELOG.md,POC-STATUS.md}, packages/workflow-engine/README.md	Adds migration roadmap, CLI pattern doc, complexity reports, and package READMEs/CHANGELOG; no runtime code.
Project state & tasks metadata .taskmaster/state.json, .taskmaster/tasks/tasks.json	Updates timestamp; adds tm-core phase-1 tasks block with nested tasks/subtasks.
CLI commands & utilities apps/cli/src/index.ts, apps/cli/src/commands/*, apps/cli/src/commands/workflow/*, apps/cli/src/utils/ui.ts	Implements Commander-based commands: list tasks, auth (login/logout/status/refresh), workflow (start/list/status/stop); central registrar; UI helpers for tables, banners, colors.
Core: public API & facade packages/tm-core/src/index.ts, packages/tm-core/src/task-master-core.ts, packages/tm-core/src/constants/index.ts, packages/tm-core/src/entities/task.entity.ts	Adds TaskMasterCore facade, consolidated exports, task domain entity, and constants (statuses, priorities, formats).
Core: types & interfaces packages/tm-core/src/types/*, packages/tm-core/src/interfaces/*	Introduces task models, filters, guards; configuration contracts and defaults; AI provider interfaces/base class; storage contracts/base class.
Core: config subsystem packages/tm-core/src/config/*, packages/tm-core/src/config/services/*, tests *.spec.ts	Adds ConfigManager and services (loader, merger, persistence, runtime state, env provider) with unit/integration tests.
Core: auth subsystem packages/tm-core/src/auth/*, tests auth-*.test.ts	Adds AuthManager singleton, CredentialStore (file-backed), OAuthService (local callback), types/config, Supabase client wrapper; tests for manager/store.
Core: storage subsystem packages/tm-core/src/storage/*, packages/tm-core/src/storage/file-storage/*	Implements FileStorage (single tasks.json with format handler), ApiStorage (REST with retries/timeouts), StorageFactory (auto/file/api selection), utilities (atomic ops, paths).
Core: services & providers packages/tm-core/src/services/*, packages/tm-core/src/providers/*, tests tests/unit/*.test.ts, tests/integration/list-tasks.test.ts	Adds TaskService (business logic for lists/stats/filters), AI base provider and test mock; smoke and base-provider tests; list-tasks integration test.
Core: logger & errors & parser & utils packages/tm-core/src/logger/*, packages/tm-core/src/errors/*, packages/tm-core/src/parser/index.ts, packages/tm-core/src/utils/*, packages/tm-core/src/clients/*	Adds structured error system, logger with env-driven levels, placeholder parser, ID generators, Supabase client, and public barrels.
Workflow engine package packages/workflow-engine/src/**/*, packages/workflow-engine/package.json	Adds TaskExecutionManager orchestrating WorktreeManager, ProcessSandbox, WorkflowStateManager; types, errors, barrels; package config/build.
Core–workflow bridge packages/tm-core/src/workflow/*	Exposes WorkflowService that wraps the workflow engine for core consumers.
MCP server adjustments mcp-server/src/index.js, mcp-server/src/tools/utils.js	Switches package.json reads to JSON module imports; removes sync FS reads for version info.
Scripts refactor & assets scripts/modules/commands.js, scripts/modules/config-manager.js, scripts/modules/prompt-manager.js, scripts/modules/update-config-tokens.js, scripts/init.js, src/utils/asset-resolver.js, src/utils/rule-transformer.js, tests/integration/profiles/*	Moves command registration to @tm/cli; replaces FS JSON reads with JSON imports; reworks prompt loading/validation to in-memory; introduces asset resolver; adapts rule transformer and tests to new assets/dist layout.
Extension formatting-only apps/extension/package.json	Reformats arrays to single-line; no behavior changes.
tm-core package scaffolding packages/tm-core/.gitignore, packages/tm-core/tsup.config.ts	Adds ignore rules; build config with env injection of TM_PUBLIC_* variables.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant CLI as @tm/cli (ListTasksCommand)
  participant Core as TaskMasterCore
  participant Service as TaskService
  participant Factory as StorageFactory
  participant Storage as FileStorage/ApiStorage

  User->>CLI: tm list [--status --tag ...]
  CLI->>Core: getTaskList(options)
  Core->>Service: getTaskList(options)
  Service->>Factory: create(config, projectPath)
  Factory-->>Storage: IStorage instance
  Service->>Storage: loadTasks(tag)
  Storage-->>Service: Task[]
  Service-->>Core: {tasks,total,filtered,tag,storageType}
  Core-->>CLI: ListTasksResult
  CLI->>User: Render table/json

Loading

sequenceDiagram
  autonumber
  actor User
  participant CLI as @tm/cli (AuthCommand)
  participant Auth as AuthManager
  participant OAuth as OAuthService
  participant Supa as SupabaseAuthClient
  participant Store as CredentialStore

  User->>CLI: tm auth login
  CLI->>Auth: authenticateWithOAuth({openBrowser})
  Auth->>OAuth: authenticate(options)
  OAuth->>OAuth: start local callback server
  OAuth-->>CLI: onAuthUrl(url)
  CLI->>User: Opens browser to URL
  Supa-->>OAuth: Callback with token
  OAuth->>Supa: getUser(token)
  OAuth->>Store: saveCredentials({token,refreshToken,expiresAt,...})
  OAuth-->>Auth: AuthCredentials
  Auth-->>CLI: AuthCredentials
  CLI->>User: Success + status

Loading

sequenceDiagram
  autonumber
  actor User
  participant CLI as @tm/cli (WorkflowStartCommand)
  participant Core as TaskMasterCore
  participant Wf as WorkflowService
  participant Eng as TaskExecutionManager
  participant WT as WorktreeManager
  participant PS as ProcessSandbox
  participant WS as WorkflowStateManager

  User->>CLI: tm workflow start <taskId>
  CLI->>Core: workflow.start(taskId, opts)
  Core->>Wf: start(taskId, opts)
  Wf->>Eng: startTaskExecution(Task, opts)
  Eng->>WT: createWorktree(taskId, branch)
  WT-->>Eng: WorktreeInfo
  Eng->>WS: registerWorkflow(ctx)
  Eng->>PS: startProcess(workflowId, taskId, prompt, opts)
  PS-->>Eng: pid
  Eng-->>Wf: workflowId
  Wf-->>Core: workflowId
  Core-->>CLI: workflowId
  CLI->>User: Show run info

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120–180 minutes

Possibly related PRs

• Feat: add typescript and start refactoring #1190 — Implements the same monorepo refactor (tm-core, workflow-engine, CLI), overlapping files and APIs.
• feat: add oauth with remote server #1178 — Adds/modifies the same auth stack (AuthManager, CredentialStore, OAuthService) and CLI auth command.
• feat: improve config-manager max tokens for openrouter and kimi-k2 model #1035 — Changes the same config-model loading path (scripts/modules/config-manager.js and supported-models.json) now switched to JSON imports here.

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "ralph/feat.add.workflow" is related to the changeset (the PR adds workflow-related packages and CLI workflow commands) but is a branch-style identifier rather than a concise, human-readable single-sentence summary; it therefore only partially meets the guideline for a clear PR title.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch ralph/feat.add.workflow

---

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

[Crunchyman-ralph]

duplicate of #1198

[coderabbitai]

Actionable comments posted: 281

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (21)

scripts/init.js (1)

620-637: Inconsistent asset loading for .gitignore; use resolver to avoid path breakage.

This still relies on __dirname/../assets/gitignore. In bundled/npm installs this path may not exist; use readAsset('gitignore') for consistency.

-  try {
-    const gitignoreTemplatePath = path.join(
-      __dirname,
-      '..',
-      'assets',
-      'gitignore'
-    );
-    const templateContent = fs.readFileSync(gitignoreTemplatePath, 'utf8');
+  try {
+    const templateContent = readAsset('gitignore', 'utf8');
     manageGitignoreFile(
       path.join(targetDir, GITIGNORE_FILE),
       templateContent,
       storeTasksInGit,
       log
     );

src/utils/asset-resolver.js (1)

1-93: Add tests to satisfy utils coverage policy.

Per guidelines, utils need ≥90% coverage. Add unit tests for: candidate resolution order, find-up fallback, traversal guard, and Buffer vs utf8 reads.

I can add a Jest/Vitest suite hitting these branches and a fixture assets tree; want me to open a follow-up PR?

scripts/modules/prompt-manager.js (4)

82-123: Ensure loadPrompt is made async if implementing lazy loading.

If you implement the suggested lazy loading approach, this method needs to become async to handle dynamic imports.

If implementing lazy loading:

-loadPrompt(promptId, variables = {}, variantKey = null) {
+async loadPrompt(promptId, variables = {}, variantKey = null) {
	try {
		// ... cache check ...
		
		// Load template
-		const template = this.loadTemplate(promptId);
+		const template = await this.loadTemplate(promptId);

---

273-286: ⚠️ Security concern: Using new Function for condition evaluation is risky.

The use of new Function to evaluate conditions poses security risks as it could execute arbitrary code if the condition strings are not properly sanitized. This is especially concerning if conditions could come from user input or external sources.

Replace the unsafe evaluation with a safer approach:

evaluateCondition(condition, variables) {
	try {
-		// Create a safe evaluation context
-		const context = { ...variables };
-		
-		// Simple condition evaluation (can be enhanced)
-		// For now, supports basic comparisons
-		const func = new Function(...Object.keys(context), `return ${condition}`);
-		return func(...Object.values(context));
+		// Parse and evaluate conditions safely
+		// Support common patterns like: varName === "value", varName > 5, etc.
+		const comparisonRegex = /^(\w+(?:\.\w+)*)\s*(===?|!==?|>=?|<=?)\s*(.+)$/;
+		const match = condition.trim().match(comparisonRegex);
+		
+		if (match) {
+			const [, path, operator, rawValue] = match;
+			const leftValue = this.getNestedValue(variables, path);
+			
+			// Parse the right value (handle strings, numbers, booleans)
+			let rightValue = rawValue.trim();
+			if (rightValue.startsWith('"') && rightValue.endsWith('"')) {
+				rightValue = rightValue.slice(1, -1);
+			} else if (rightValue === 'true') {
+				rightValue = true;
+			} else if (rightValue === 'false') {
+				rightValue = false;
+			} else if (!isNaN(rightValue)) {
+				rightValue = parseFloat(rightValue);
+			}
+			
+			// Evaluate based on operator
+			switch (operator) {
+				case '===':
+				case '==':
+					return leftValue === rightValue;
+				case '!==':
+				case '!=':
+					return leftValue !== rightValue;
+				case '>':
+					return leftValue > rightValue;
+				case '>=':
+					return leftValue >= rightValue;
+				case '<':
+					return leftValue < rightValue;
+				case '<=':
+					return leftValue <= rightValue;
+				default:
+					return false;
+			}
+		}
+		
+		// Handle simple boolean checks
+		const value = this.getNestedValue(variables, condition);
+		return !!value;
	} catch (error) {
		log('warn', `Failed to evaluate condition: ${condition}`);
		return false;
	}
}

---

409-434: Consider making validateAllPrompts async for lazy loading compatibility.

If implementing lazy loading, this method needs to handle async template loading.

-validateAllPrompts() {
+async validateAllPrompts() {
	const results = { total: 0, errors: [], valid: [] };
	
-	// Iterate through all imported prompts
-	for (const [promptId, template] of this.prompts.entries()) {
+	// Validate all available prompts (load them if using lazy loading)
+	const promptIds = this.promptPaths ? Object.keys(this.promptPaths) : Array.from(this.prompts.keys());
+	
+	for (const promptId of promptIds) {
		results.total++;
		
		try {
+			// Load template if using lazy loading
+			const template = this.promptPaths 
+				? await this.loadTemplate(promptId)
+				: this.prompts.get(promptId);
+			
			// Validate the template
			if (this.validatePrompt && this.validatePrompt !== true) {

---

529-534: Consider async initialization for the singleton if implementing lazy loading.

If adopting the lazy loading approach, the singleton getter might need to handle async initialization.

-export function getPromptManager() {
+export async function getPromptManager() {
	if (!promptManager) {
		promptManager = new PromptManager();
+		// Optionally preload critical prompts
+		// await promptManager.preloadCriticalPrompts();
	}
	return promptManager;
}

scripts/modules/config-manager.js (6)

686-706: Normalize provider name and unify “no API key” handling in one place

Avoid case-sensitivity pitfalls and the split special-case for claude-code. Also reuse a single early-return path.

Apply:

 function isApiKeySet(providerName, session = null, projectRoot = null) {
-  // Define the expected environment variable name for each provider
-
-  // Providers that don't require API keys for authentication
-  const providersWithoutApiKeys = [
-    CUSTOM_PROVIDERS.OLLAMA,
-    CUSTOM_PROVIDERS.BEDROCK,
-    CUSTOM_PROVIDERS.MCP,
-    CUSTOM_PROVIDERS.GEMINI_CLI
-  ];
-
-  if (providersWithoutApiKeys.includes(providerName?.toLowerCase())) {
-    return true; // Indicate key status is effectively "OK"
-  }
-
-  // Claude Code doesn't require an API key
-  if (providerName?.toLowerCase() === 'claude-code') {
-    return true; // No API key needed
-  }
+  const normalized = providerName?.toLowerCase();
+  // Providers that don't require API keys for authentication
+  const providersWithoutApiKeys = [
+    CUSTOM_PROVIDERS.OLLAMA,
+    CUSTOM_PROVIDERS.BEDROCK,
+    CUSTOM_PROVIDERS.MCP,
+    CUSTOM_PROVIDERS.GEMINI_CLI,
+    'claude-code'
+  ];
+  if (providersWithoutApiKeys.includes(normalized)) {
+    return true; // Indicate key status is effectively "OK"
+  }

   const keyMap = {
@@
-  const providerKey = providerName?.toLowerCase();
+  const providerKey = normalized;
   if (!providerKey || !keyMap[providerKey]) {
     log('warn', `Unknown provider name: ${providerName} in isApiKeySet check.`);
     return false;
   }

Also applies to: 723-739

---

748-758: Early-return for MCP providers without keys + case normalization

Mirror isApiKeySet semantics and make the switch-case robust to casing.

Apply:

-function getMcpApiKeyStatus(providerName, projectRoot = null) {
+function getMcpApiKeyStatus(providerName, projectRoot = null) {
+  const provider = providerName?.toLowerCase();
+  // Providers that effectively don't need API keys checked in MCP config
+  if (
+    provider === 'claude-code' ||
+    provider === CUSTOM_PROVIDERS.OLLAMA ||
+    provider === CUSTOM_PROVIDERS.GEMINI_CLI ||
+    provider === CUSTOM_PROVIDERS.MCP ||
+    provider === CUSTOM_PROVIDERS.BEDROCK
+  ) {
+    return true;
+  }
@@
-    switch (providerName) {
+    switch (provider) {
       case 'anthropic':
         apiKeyToCheck = mcpEnv.ANTHROPIC_API_KEY;
         placeholderValue = 'YOUR_ANTHROPIC_API_KEY_HERE';
         break;

Also applies to: 777-829

---

148-177: Also validate model IDs against MODEL_MAP (non-strict, warn only)

You validate providers but not provider/model pairs. Add a warning to help users catch typos and deprecated IDs earlier.

Apply:

       if (
         config.models.fallback?.provider &&
         !validateProvider(config.models.fallback.provider)
       ) {
@@
         config.models.fallback.modelId = undefined;
       }
+      // Warn on unknown model IDs for known providers (non-strict)
+      const rolesToCheck = ['main', 'research', 'fallback'];
+      for (const roleName of rolesToCheck) {
+        const roleCfg = config.models[roleName];
+        if (
+          roleCfg?.provider &&
+          roleCfg?.modelId &&
+          !validateProviderModelCombination(roleCfg.provider, roleCfg.modelId)
+        ) {
+          console.warn(
+            chalk.yellow(
+              `Warning: Unknown modelId "${roleCfg.modelId}" for provider "${roleCfg.provider}" in ${roleName} config.`
+            )
+          );
+        }
+      }

---

41-46: Fix misleading comment about fallback defaults

The code sets a concrete fallback by default; the comment says otherwise.

Apply:

-      // No default fallback provider/model initially
+      // Default fallback provider/model

---

233-255: Cache default-loaded config to avoid repeated disk reads

Currently, calling getConfig() without explicitRoot reloads every time. Safe to cache when first loaded.

Apply:

   if (needsLoad) {
     const newConfig = _loadAndValidateConfig(explicitRoot); // _load handles null explicitRoot
@@
-    if (forceReload || explicitRoot) {
+    if (forceReload || explicitRoot || !loadedConfig) {
       loadedConfig = newConfig;
       loadedConfigRoot = explicitRoot; // Store the root used for this loaded config
     }
     return newConfig; // Return the newly loaded/default config
   }

---

967-968: Use consistent log level token

Elsewhere you use 'warn'; here it's 'warning'.

Apply:

-      log(
-        'warning',
+      log(
+        'warn',
         'Failed to write updated configuration with new userId. Please let the developers know.'
       );

scripts/modules/update-config-tokens.js (1)

27-35: Type-safe check and cap maxTokens to model limit (don’t raise it)

Match getParametersForRole semantics: use the model limit as an upper bound, not an unconditional overwrite.

Apply:

-          if (modelData && modelData.max_tokens) {
-            config.models[role].maxTokens = modelData.max_tokens;
-          }
+          const maxTokens = Number(modelData?.max_tokens);
+          if (Number.isFinite(maxTokens) && maxTokens > 0) {
+            const current = Number(config.models[role].maxTokens);
+            config.models[role].maxTokens = Number.isFinite(current)
+              ? Math.min(current, maxTokens)
+              : maxTokens;
+          }

tests/integration/profiles/roo-files-inclusion.test.js (1)

72-77: Reduce brittleness of string-based source assertions.

Asserting on literal log strings in source is fragile. Prefer importing the module and validating exported functions/behavior.

tests/integration/profiles/rules-files-inclusion.test.js (1)

77-83: Import path assertion is acceptable but brittle.

Consider validating behavior (e.g., exported ROO_MODES usage) instead of source string matching.

package.json (1)

103-105: Bump engines.node to >=22.0.0 and align CI to Node 22+

Repo uses "import ... with { type: 'json' }" (requires Node ≥22); engines remain >=18 and GitHub Actions use Node 20 — update to avoid CI/runtime failures.

• Replace in package.json files: package.json (root), apps/cli/package.json, packages/tm-core/package.json — change "node": ">=18.0.0" → "node": ">=22.0.0".
• Update all .github/workflows/* entries that set node-version: 20 to node-version: 22 (examples: .github/workflows/ci.yml, extension-ci.yml, release.yml, auto-close-duplicates.yml, backfill-duplicate-comments.yml, update-models-md.yml).
• No .nvmrc or .tool-versions found — verify local/dev manager configs if used.
• Evidence (JSON import usage): packages/tm-core/src/auth/oauth-service.ts, scripts/modules/update-config-tokens.js, scripts/modules/config-manager.js, mcp-server/src/index.js, mcp-server/src/tools/utils.js.

Diff (apply to each package.json engines block):

-  "engines": {
-    "node": ">=18.0.0"
-  },
+  "engines": {
+    "node": ">=22.0.0"
+  },

mcp-server/src/tools/utils.js (2)

287-325: Include telemetry in standard MCP responses.

Guidelines require telemetryData to reach clients from tools. handleApiResult currently drops it. Add passthrough when present.

 async function handleApiResult(
   result,
   log,
   errorPrefix = 'API error',
   processFunction = processMCPResponseData,
   projectRoot = null
 ) {
   // Get version info for every response
   const versionInfo = getVersionInfo();

   // Get tag info if project root is provided
   const tagInfo = projectRoot ? getTagInfo(projectRoot, log) : null;

   if (!result.success) {
     const errorMsg = result.error?.message || `Unknown ${errorPrefix}`;
     log.error(`${errorPrefix}: ${errorMsg}`);
     return createErrorResponse(errorMsg, versionInfo, tagInfo);
   }

   // Process the result data if needed
   const processedData = processFunction
     ? processFunction(result.data)
     : result.data;

   log.info('Successfully completed operation');

   // Create the response payload including version info and tag info
   const responsePayload = {
     data: processedData,
     version: versionInfo
   };
+  // Preserve telemetry data when provided by direct functions/tools
+  if (result.telemetryData) {
+    responsePayload.telemetryData = result.telemetryData;
+  }

   // Add tag information if available
   if (tagInfo) {
     responsePayload.tag = tagInfo;
   }

   return createContentResponse(responsePayload);
 }

---

178-192: Overly verbose session logging risks leaking sensitive paths; downgrade/sanitize.

Log only structure/flags at debug level, avoid raw URIs.

-  log.info(
-    `Session object: ${JSON.stringify({
+  log.debug(
+    `Session meta: ${JSON.stringify({
       hasSession: !!session,
       hasRoots: !!session?.roots,
       rootsType: typeof session?.roots,
       isRootsArray: Array.isArray(session?.roots),
       rootsLength: session?.roots?.length,
-      firstRoot: session?.roots?.[0],
       hasRootsRoots: !!session?.roots?.roots,
       rootsRootsType: typeof session?.roots?.roots,
       isRootsRootsArray: Array.isArray(session?.roots?.roots),
       rootsRootsLength: session?.roots?.roots?.length,
-      firstRootsRoot: session?.roots?.roots?.[0]
+      // Do not log raw URIs; redact potential PII
+      hasFirstRootUri: !!session?.roots?.[0]?.uri,
+      hasAltFirstRootUri: !!session?.roots?.roots?.[0]?.uri
     })}`
   );

mcp-server/src/index.js (1)

118-126: Bug: dereferencing session in the else branch when session is falsy.

This will throw when no session exists.

-    } else {
-      session.server.sendLoggingMessage({
-        data: {
-          context: session.context,
-          message: `No MCP sessions available, providers not registered`
-        },
-        level: 'warn'
-      });
-    }
+    } else {
+      this.logger?.warn('No MCP sessions available, providers not registered');
+    }

packages/tm-core/src/logger/factory.ts (1)

1-60: Remove "./logger" subpath export — restrict logger API to the package root

• Delete the "./logger" entry from packages/tm-core/package.json exports (it currently exposes src/logger/index.ts / dist/logger/index.js).
• Keep the re-export in packages/tm-core/src/index.ts (export { getLogger, createLogger, setGlobalLogger } from './logger/index.js';) so consumers access logger only via '@tm/core'.

packages/tm-core/src/interfaces/configuration.interface.ts (1)

404-445: Update default model IDs and MAX_PROMPT_LENGTH

File: packages/tm-core/src/interfaces/configuration.interface.ts (lines 404–445)

• Anthropic: replace/deprecate "claude-3-5-sonnet-20241022" — Anthropic announced retirement (Aug 13, 2025); migrate to "claude-sonnet-4-20250514" or make the model configurable.
• OpenAI: "gpt-4o-mini" is superseded by the GPT‑4.1 family (released Apr 14, 2025). gpt-4o-mini context ≈128K tokens; GPT‑4.1 variants support up to 1,000,000 tokens — update default to a current recommended OpenAI model.
• MAX_PROMPT_LENGTH (100000) may not match provider token caps or units (tokens vs characters) — store and enforce provider-specific token limits and validate inputs at runtime.
• Add inline links to provider docs and a "last-verified" date (e.g., 2025-09-11).

[coderabbitai]

🧹 Nitpick (assertive)

Generated report — don’t hand-edit

Same note: treat this as generated; keep changes flowing from task-master tooling. If this replaces/duplicates tm-core-complexity.json, consider retaining only one canonical artifact to reduce drift.

🤖 Prompt for AI Agents

.taskmaster/reports/task-complexity-report_tm-core-phase-1.json lines 1-77: This
is a generated artifact that should not be hand-edited and appears to
duplicate/replace tm-core-complexity.json; to fix, remove this generated file
from the PR (undo manual edits), ensure only one canonical complexity report is
committed (either tm-core-complexity.json or this file), and update the
build/tooling or .gitignore to prevent generated reports from being committed in
future so the canonical artifact is produced and tracked by the tooling instead
of being checked in manually.

[coderabbitai]

🧹 Nitpick (assertive)

⚠️ Potential issue

Do not manually edit Taskmaster-generated files — revert this hunk and regenerate via Taskmaster commands.

Per repository guidelines, .taskmaster/tasks/tasks.json must be modified only through Taskmaster CLI/automation. Hand-editing risks schema drift and tooling breakage. Please revert these changes and reapply them using Taskmaster commands (e.g., add-tag/add-task/update-subtask/import flows).

I can provide an exact sequence of Taskmaster CLI commands to recreate “tm-core-phase-1” with its tasks/subtasks if you confirm the desired fields.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Potential ID type inconsistency with core types (Task.id likely string).

Core type definitions indicate Task.id should be a string (e.g., "TASK-..."), while these entries use numeric IDs. If the codebase has migrated to string IDs (see id-generator utilities), these numeric IDs will be inconsistent.

Run to assess current repo usage and decide whether to migrate these to string IDs:

If the project standard is string IDs, regenerate this tag’s tasks via Taskmaster so IDs follow the expected format (don’t hand-convert).

Also applies to: 7373-7375, 7441-7444, 7508-7511, 7575-7577, 7642-7645, 7709-7712, 7777-7780, 7847-7850, 7915-7918, 7988-7991

---

🏁 Script executed:

#!/bin/bash
# Inspect current IDs in tasks.json across tags
jq -r '
  to_entries[] | select(.key!="metadata")
  | .key as $tag
  | .value.tasks[] | [$tag, ( .id|type ), .id] | @tsv
' .taskmaster/tasks/tasks.json | column -t

Length of output: 264

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE=".taskmaster/tasks/tasks.json"
if [[ ! -f "$FILE" ]]; then
  echo "ERROR: $FILE not found"
  exit 0
fi

python3 - <<'PY'
import json,os,re,sys

file = ".taskmaster/tasks/tasks.json"
with open(file, "r", encoding="utf-8") as f:
    data = json.load(f)

total_tasks = 0
total_counts = {"number":0,"string":0,"other":0}
per_tag = {}
numeric_samples = []

for tag, content in data.items():
    if tag == "metadata":
        continue
    tasks = content.get("tasks", []) or []
    counts = {"number":0,"string":0,"other":0}
    for t in tasks:
        total_tasks += 1
        tid = t.get("id", None)
        if isinstance(tid, int):
            typ = "number"
        elif isinstance(tid, str):
            typ = "string"
        else:
            typ = "other"
        counts[typ] += 1
        total_counts[typ] += 1
        if typ == "number" and len(numeric_samples) < 50:
            numeric_samples.append((tag, tid, t.get("title","")[:80]))
    per_tag[tag] = counts

print("PER-TAG ID COUNTS (tag, string, number, other):")
for tag, c in per_tag.items():
    print(f"{tag:36} {c['string']:6} {c['number']:6} {c['other']:6}")

print("\nTOTAL TASKS:", total_tasks)
print("TOTAL by id type: string={string} number={number} other={other}".format(**total_counts))

if numeric_samples:
    print("\nEXAMPLES of numeric IDs (up to 50):")
    for tag, tid, title in numeric_samples:
        print(f"{tag:36} {tid:<12} {title}")

# Search repository for Task type defs and id utilities
patterns = [
    ("interface Task", re.compile(r'\binterface\s+Task\b')),
    ("type Task", re.compile(r'\btype\s+Task\b')),
    ("class Task", re.compile(r'\bclass\s+Task\b')),
    ("id-generator/idGenerator/generateId", re.compile(r'id-generator|idGenerator|generateId')),
    ("TASK-", re.compile(r'TASK-')),
]

print("\nSEARCHING repo for Task type defs and id utilities (file:line:preview) -- limited to 200 hits")
hits=0
max_hits=200
exclude_dirs = {".git","node_modules","dist","build","venv",".venv","__pycache__"}
for root, dirs, files in os.walk("."):
    dirs[:] = [d for d in dirs if d not in exclude_dirs]
    for fname in files:
        if hits>=max_hits:
            break
        path = os.path.join(root, fname)
        # skip binary-like by extension
        if any(path.startswith(p) for p in ["./.git", "./node_modules", "./dist", "./build"]):
            continue
        try:
            with open(path, "r", encoding="utf-8", errors="ignore") as fh:
                for i,line in enumerate(fh,1):
                    for label,pat in patterns:
                        if pat.search(line):
                            preview = line.strip()
                            print(f"{path}:{i}:{label}: {preview[:200]}")
                            hits += 1
                            break
                    if hits>=max_hits:
                        break
        except Exception:
            continue
    if hits>=max_hits:
        break

if hits==0:
    print("No matches found for Task type defs or id utilities.")
PY

Length of output: 59

---

🏁 Script executed:

#!/bin/sh
# Safer, portable verification script for Task.id types and Task type/id utilities.
FILE=".taskmaster/tasks/tasks.json"
if [ ! -f "$FILE" ]; then
  echo "ERROR: $FILE not found"
  exit 0
fi

python3 - <<'PY'
import json,os,re,sys

file = ".taskmaster/tasks/tasks.json"
try:
    with open(file, "r", encoding="utf-8") as f:
        data = json.load(f)
except Exception as e:
    print("ERROR reading JSON:", e)
    sys.exit(0)

total_tasks = 0
total_counts = {"number":0,"string":0,"other":0}
per_tag = {}
numeric_samples = []

for tag, content in data.items():
    if tag == "metadata":
        continue
    tasks = content.get("tasks") or []
    counts = {"number":0,"string":0,"other":0}
    for t in tasks:
        total_tasks += 1
        tid = t.get("id", None)
        if isinstance(tid, bool) or tid is None:
            typ = "other"
        elif isinstance(tid, int):
            typ = "number"
        elif isinstance(tid, str):
            typ = "string"
        else:
            typ = "other"
        counts[typ] += 1
        total_counts[typ] += 1
        if typ == "number" and len(numeric_samples) < 50:
            numeric_samples.append((tag, tid, t.get("title","")[:120]))
    per_tag[tag] = counts

print("PER-TAG ID COUNTS (tag, string, number, other):")
for tag, c in sorted(per_tag.items()):
    print(f"{tag:60} {c['string']:6} {c['number']:6} {c['other']:6}")

print("\nTOTAL TASKS:", total_tasks)
print("TOTAL by id type: string={string} number={number} other={other}".format(**total_counts))

if numeric_samples:
    print("\nEXAMPLES of numeric IDs (up to 50):")
    for tag, tid, title in numeric_samples:
        print(f"{tag:60} {str(tid):<12} {title}")

# Search repo for Task type definitions and id utilities
patterns = [
    ("interface Task", re.compile(r'\binterface\s+Task\b')),
    ("export interface Task", re.compile(r'\bexport\s+interface\s+Task\b')),
    ("type Task", re.compile(r'\btype\s+Task\b')),
    ("class Task", re.compile(r'\bclass\s+Task\b')),
    ("Task.id", re.compile(r'\bTask\b.*\b(id)\b')),
    ("id: string (TS)", re.compile(r'\b(id)\s*:\s*string\b')),
    ("id-generator", re.compile(r'id[-_]?generator|generateId|generate-id|idGenerator')),
    ("TASK-", re.compile(r'TASK-')),
]

print("\nSEARCH HITS (file:line:label:preview) -- limited to 200:")
hits = 0
max_hits = 200
exclude_dirs = {".git","node_modules","dist","build","venv",".venv","__pycache__"}
for root, dirs, files in os.walk("."):
    dirs[:] = [d for d in dirs if d not in exclude_dirs]
    for fname in files:
        if hits >= max_hits:
            break
        path = os.path.join(root, fname)
        # skip some large/binary-looking files by extension
        if any(path.lower().endswith(ext) for ext in ('.png','.jpg','.jpeg','.gz','.zip','.exe','.dll','.so','.pdf')):
            continue
        try:
            with open(path, "r", encoding="utf-8", errors="ignore") as fh:
                for i,line in enumerate(fh,1):
                    for label,pat in patterns:
                        if pat.search(line):
                            preview = line.strip()
                            print(f"{path}:{i}:{label}: {preview[:300]}")
                            hits += 1
                            break
                    if hits >= max_hits:
                        break
        except Exception:
            continue
    if hits >= max_hits:
        break

if hits == 0:
    print("No matches found for Task type defs or id utilities.")
PY

Length of output: 29895

---

Replace numeric IDs in .taskmaster/tasks/tasks.json with canonical string IDs from the id-generator.

115 tasks currently use numeric IDs (master: 93, cc-kiro-hooks: 10, tm-core-phase-1: 11, test-tag: 1) while core types and utilities expect string IDs (see packages/tm-core/src/types/index.ts and packages/tm-core/src/utils/id-generator.ts). Regenerate the affected tag(s) via Taskmaster so IDs are produced by the id-generator (do not hand-convert). Inspect: .taskmaster/tasks/tasks.json; packages/tm-core/src/utils/id-generator.ts; packages/tm-core/src/types/index.ts; packages/tm-core/src/entities/task.entity.ts.

🤖 Prompt for AI Agents

In .taskmaster/tasks/tasks.json around lines 7304-7305 there are numeric task
IDs that must be replaced with canonical string IDs produced by the project's
id-generator; regenerate the affected task groups/tags via the Taskmaster
tooling (do not hand-convert numeric IDs), ensuring the new IDs match the format
in packages/tm-core/src/utils/id-generator.ts and conform to the types in
packages/tm-core/src/types/index.ts and
packages/tm-core/src/entities/task.entity.ts; run Taskmaster to produce and
commit the updated tasks.json so all previously numeric IDs (master,
cc-kiro-hooks, tm-core-phase-1, test-tag) are replaced with generated string
IDs.

[coderabbitai]

🧹 Nitpick (assertive)

Harden TS SDK path for multi-root/monorepo.

Use workspace variable to avoid breakage when opening a subfolder or using alternate root.

Apply this diff:

-  "typescript.tsdk": "node_modules/typescript/lib",
+  "typescript.tsdk": "${workspaceFolder}/node_modules/typescript/lib",

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

Suggested change

	"typescript.tsdk": "node_modules/typescript/lib",
	"typescript.tsdk": "${workspaceFolder}/node_modules/typescript/lib",

🤖 Prompt for AI Agents

In .vscode/settings.json around line 14, the TypeScript SDK path is hardcoded to
"node_modules/typescript/lib"; change it to use the workspace variable so it
works in multi-root/monorepo setups by replacing the value with
"${workspaceFolder}/node_modules/typescript/lib" (keep JSON quoting intact) so
the editor resolves the TS SDK relative to the opened workspace folder.

[coderabbitai]

🧹 Nitpick (assertive)

Biome as default formatter: good; add missing React/JSONC language scopes.

Add JavaScript React and JSONC so tsx/jsx and tsconfig-like files are consistently formatted by Biome.

Apply this diff:

   "[typescript]": {
     "editor.defaultFormatter": "biomejs.biome"
   },
   "[typescriptreact]": {
     "editor.defaultFormatter": "biomejs.biome"
   },
   "[javascript]": {
     "editor.defaultFormatter": "biomejs.biome"
   },
+  "[javascriptreact]": {
+    "editor.defaultFormatter": "biomejs.biome"
+  },
   "[json]": {
     "editor.defaultFormatter": "biomejs.biome"
-  }
+  },
+  "[jsonc]": {
+    "editor.defaultFormatter": "biomejs.biome"
+  }

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

Suggested change

	"[typescript]": {
	"editor.defaultFormatter": "biomejs.biome"
	},
	"[typescriptreact]": {
	"editor.defaultFormatter": "biomejs.biome"
	},
	"[javascript]": {
	"editor.defaultFormatter": "biomejs.biome"
	},
	"[json]": {
	"editor.defaultFormatter": "biomejs.biome"
	}
	"[typescript]": {
	"editor.defaultFormatter": "biomejs.biome"
	},
	"[typescriptreact]": {
	"editor.defaultFormatter": "biomejs.biome"
	},
	"[javascript]": {
	"editor.defaultFormatter": "biomejs.biome"
	},
	"[javascriptreact]": {
	"editor.defaultFormatter": "biomejs.biome"
	},
	"[json]": {
	"editor.defaultFormatter": "biomejs.biome"
	},
	"[jsonc]": {
	"editor.defaultFormatter": "biomejs.biome"
	}

🤖 Prompt for AI Agents

.vscode/settings.json around lines 15 to 26: the workspace sets Biome as the
default formatter for typescript, typescriptreact, javascript and json but is
missing the JavaScript React and JSONC scopes; add entries for
"[javascriptreact]" and "[jsonc]" with "editor.defaultFormatter":
"biomejs.biome" so .jsx/.tsx and tsconfig-like JSONC files are formatted
consistently by Biome.

[coderabbitai]

🛠️ Refactor suggestion

Prevent path traversal outside assets.

Use path.resolve and enforce that resolved paths stay within the assets dir.

-export function getAssetPath(relativePath) {
-  const assetsDir = getAssetsDir();
-  return path.join(assetsDir, relativePath);
-}
+export function getAssetPath(relativePath) {
+  const assetsDir = getAssetsDir();
+  const full = path.resolve(assetsDir, relativePath);
+  if (!full.startsWith(assetsDir + path.sep)) {
+    throw new Error(`Refusing to access outside assets: ${relativePath}`);
+  }
+  return full;
+}

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

Suggested change

	export function getAssetPath(relativePath) {
	const assetsDir = getAssetsDir();
	return path.join(assetsDir, relativePath);
	}
	export function getAssetPath(relativePath) {
	const assetsDir = getAssetsDir();
	const full = path.resolve(assetsDir, relativePath);
	if (!full.startsWith(assetsDir + path.sep)) {
	throw new Error(`Refusing to access outside assets: ${relativePath}`);
	}
	return full;
	}

🤖 Prompt for AI Agents

In src/utils/asset-resolver.js around lines 64 to 67, the function returns a
joined path allowing path traversal outside the assets directory; resolve the
assetsDir and the candidate with path.resolve, then verify the resolved
candidate is within the resolved assetsDir (e.g. compute
path.relative(resolvedAssetsDir, resolvedCandidate) and throw an error if it
starts with '..' or is absolute outside), and only return the resolvedCandidate
when the check passes to prevent escaping the assets directory.

[coderabbitai]

🧹 Nitpick (assertive)

Minor: support Buffer reads cleanly and surface clearer errors.

Return Buffer when encoding is falsy; wrap read errors for context.

-export function readAsset(relativePath, encoding = 'utf8') {
-  const assetPath = getAssetPath(relativePath);
-  return fs.readFileSync(assetPath, encoding);
-}
+export function readAsset(relativePath, encoding = 'utf8') {
+  const assetPath = getAssetPath(relativePath);
+  try {
+    return encoding ? fs.readFileSync(assetPath, encoding) : fs.readFileSync(assetPath);
+  } catch (e) {
+    throw new Error(`Failed to read asset "${relativePath}" at ${assetPath}: ${e.message}`);
+  }
+}

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

Suggested change

	export function readAsset(relativePath, encoding = 'utf8') {
	const assetPath = getAssetPath(relativePath);
	return fs.readFileSync(assetPath, encoding);
	}
	export function readAsset(relativePath, encoding = 'utf8') {
	const assetPath = getAssetPath(relativePath);
	try {
	return encoding ? fs.readFileSync(assetPath, encoding) : fs.readFileSync(assetPath);
	} catch (e) {
	throw new Error(`Failed to read asset "${relativePath}" at ${assetPath}: ${e.message}`);
	}
	}

🤖 Prompt for AI Agents

In src/utils/asset-resolver.js around lines 89 to 92, the readAsset function
always reads as UTF-8 and doesn't handle read errors with context; change it to
return a Buffer when encoding is falsy (pass null to fs.readFileSync) and wrap
the fs.readFileSync call in a try/catch that throws a new error including the
assetPath and original error (use the Error cause if available) so callers get
clear context on failures.

[coderabbitai]

🧹 Nitpick (assertive)

Avoid repeated FS scans for assetsDir (cache once).

getAssetsDir() likely hits the filesystem; cache it per invocation and pass to hooks to reduce overhead.

-  if (typeof profile.onAddRulesProfile === 'function') {
+  const assetsDir = (() => {
+    try { return getAssetsDir(); } catch { return null; }
+  })();
+  if (typeof profile.onAddRulesProfile === 'function' && assetsDir) {
-    const assetsDir = getAssetsDir();
     profile.onAddRulesProfile(projectRoot, assetsDir);
@@
-  if (typeof profile.onPostConvertRulesProfile === 'function') {
-    try {
-      const assetsDir = getAssetsDir();
+  if (typeof profile.onPostConvertRulesProfile === 'function' && assetsDir) {
+    try {
       profile.onPostConvertRulesProfile(projectRoot, assetsDir);

Also applies to: 307-309

🤖 Prompt for AI Agents

In src/utils/rule-transformer.js around lines 212-214 and 307-309,
getAssetsDir() is being called multiple times causing repeated filesystem scans;
cache the result once in a local variable (e.g., const assetsDir =
getAssetsDir()) in the surrounding function scope and replace the duplicate
getAssetsDir() calls with that variable when calling profile.onAddRulesProfile
and any other hook invocations to reuse the same path and avoid repeated FS
hits.

[coderabbitai]

🧹 Nitpick (assertive)

Guard against missing "files" in package.json to avoid TypeError.

Add an existence check before toContain for clearer failures.

-    // Check if dist/** is included in the files array (which contains bundled output including Roo files)
-    expect(packageJson.files).toContain('dist/**');
+    // Check if dist/** is included in the files array (which contains bundled output including Roo files)
+    expect(Array.isArray(packageJson.files)).toBe(true);
+    expect(packageJson.files).toContain('dist/**');

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

Suggested change

	test('package.json includes dist/** in the "files" array for bundled files', () => {
	// Read the package.json file
	const packageJsonPath = path.join(process.cwd(), 'package.json');
	const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8'));
	// Check if assets/** is included in the files array (which contains Roo files)
	expect(packageJson.files).toContain('assets/**');
	// Check if dist/** is included in the files array (which contains bundled output including Roo files)
	expect(packageJson.files).toContain('dist/**');
	});
	test('package.json includes dist/** in the "files" array for bundled files', () => {
	// Read the package.json file
	const packageJsonPath = path.join(process.cwd(), 'package.json');
	const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8'));
	// Check if dist/** is included in the files array (which contains bundled output including Roo files)
	expect(Array.isArray(packageJson.files)).toBe(true);
	expect(packageJson.files).toContain('dist/**');
	});

🤖 Prompt for AI Agents

In tests/integration/profiles/roo-files-inclusion.test.js around lines 10 to 17,
the test assumes packageJson.files exists and will throw a TypeError if it's
missing; update the test to first assert that packageJson.files is defined and
an array (e.g., using expect(Array.isArray(packageJson.files)).toBe(true) or
expect(packageJson).toHaveProperty('files')), then proceed to check that it
contains 'dist/**' so failures are clear and do not raise an exception.

[coderabbitai]

🧹 Nitpick (assertive)

Guard against missing "files" in package.json to avoid TypeError.

Mirror the defensive check here as well.

-    // Check if dist/** is included in the files array (which contains bundled output including assets)
-    expect(packageJson.files).toContain('dist/**');
+    // Check if dist/** is included in the files array (which contains bundled output including assets)
+    expect(Array.isArray(packageJson.files)).toBe(true);
+    expect(packageJson.files).toContain('dist/**');

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

Suggested change

	test('package.json includes dist/** in the "files" array for bundled files', () => {
	// Read the package.json file
	const packageJsonPath = path.join(process.cwd(), 'package.json');
	const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8'));
	// Check if assets/** is included in the files array (which contains rules files)
	expect(packageJson.files).toContain('assets/**');
	// Check if dist/** is included in the files array (which contains bundled output including assets)
	expect(packageJson.files).toContain('dist/**');
	});
	test('package.json includes dist/** in the "files" array for bundled files', () => {
	// Read the package.json file
	const packageJsonPath = path.join(process.cwd(), 'package.json');
	const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8'));
	// Check if dist/** is included in the files array (which contains bundled output including assets)
	expect(Array.isArray(packageJson.files)).toBe(true);
	expect(packageJson.files).toContain('dist/**');
	});

🤖 Prompt for AI Agents

In tests/integration/profiles/rules-files-inclusion.test.js around lines 10 to
17, the test assumes packageJson.files exists and will throw a TypeError when
it's missing; update the test to defensively check that packageJson.files is
present and is an array (e.g., assert Array.isArray(packageJson.files) with a
clear failure message) before asserting it contains 'dist/**', or fall back to
treating missing files as an empty array and assert accordingly so the test
fails cleanly instead of throwing.

[coderabbitai]

Review continued from previous batch...

[coderabbitai]

🧹 Nitpick (assertive)

⚠️ Potential issue

Do not commit .taskmaster/state.json; revert this change and exclude from PR.

Per project guidelines, this file is system-managed. Please drop it from the PR and ignore future changes (e.g., add to .gitignore or mark assume-unchanged).

Want a quick git snippet to remove it from the PR and prevent future diffs?

🤖 Prompt for AI Agents

.taskmaster/state.json around line 3: this system-managed file was accidentally
committed; revert the change and remove it from the PR, then prevent future
diffs by adding .taskmaster/state.json to the repository .gitignore (or marking
the file assume-unchanged locally) and amend the commit/branch so the file is no
longer part of the changeset before merging.

[coderabbitai]

🛠️ Refactor suggestion

Add required test scripts per repo guidelines

Root package.json should expose test:unit, test:integration, and test:ci in addition to existing scripts.

   "scripts": {
@@
-    "test:coverage": "node --experimental-vm-modules node_modules/.bin/jest --coverage",
+    "test:coverage": "node --experimental-vm-modules node_modules/.bin/jest --coverage",
+    "test:unit": "node --experimental-vm-modules node_modules/.bin/jest --coverage --selectProjects unit || node --experimental-vm-modules node_modules/.bin/jest --coverage --testPathPattern=tests/unit|src/.+\\.test\\.ts$",
+    "test:integration": "node --experimental-vm-modules node_modules/.bin/jest --coverage --selectProjects integration || node --experimental-vm-modules node_modules/.bin/jest --coverage --testPathPattern=tests/integration",
     "test:e2e": "./tests/e2e/run_e2e.sh",
@@
-    "format": "biome format . --write"
+    "format": "biome format . --write",
+    "test:ci": "npm run build:packages && npm run test:coverage"
   },

If you have Jest projects configured, the selectProjects branches will be used; otherwise the fallback patterns apply.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

package.json lines 13-37: add three new npm scripts — test:unit,
test:integration, and test:ci — alongside the existing test script; implement
each to invoke Jest the same way the current "test" script does (node
--experimental-vm-modules node_modules/.bin/jest) but scoped: test:unit should
run unit projects using --selectProjects <unit-project-names> or a fallback
pattern like "--testPathPattern=tests/unit|__tests__/unit", test:integration
should run integration projects using --selectProjects
<integration-project-names> or fallback pattern like
"--testPathPattern=tests/integration|__tests__/integration", and test:ci should
run the CI suite (combine unit+integration or use --selectProjects for CI
project names) and include any CI-specific flags (e.g., --runInBand or --ci);
add these entries to the "scripts" object so callers can run npm run test:unit,
npm run test:integration, and npm run test:ci.

[coderabbitai]

🧹 Nitpick (assertive)

postpack chmod is fragile on Windows

chmod will fail on Windows shells. Consider doing this at build time (tsup banner/shebang) or guarding by OS.

-    "postpack": "chmod +x dist/task-master.js dist/mcp-server.js",
+    "postpack": "node -e \"const os=require('os');if(os.platform()!=='win32'){require('child_process').execSync('chmod +x dist/task-master.js dist/mcp-server.js')}\"",

Or add shebang via tsup config and skip chmod entirely.

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

Suggested change

	"postpack": "chmod +x dist/task-master.js dist/mcp-server.js",
	"changeset": "changeset",
	"release": "changeset publish",
	"inspector": "npx @modelcontextprotocol/inspector node mcp-server/server.js",
	"mcp-server": "node mcp-server/server.js",
	"inspector": "npx @modelcontextprotocol/inspector node dist/mcp-server.js",
	"mcp-server": "node dist/mcp-server.js",
	"format-check": "biome format .",
	"postpack": "node -e \"const os=require('os');if(os.platform()!=='win32'){require('child_process').execSync('chmod +x dist/task-master.js dist/mcp-server.js')}\"",
	"changeset": "changeset",
	"release": "changeset publish",
	"inspector": "npx @modelcontextprotocol/inspector node dist/mcp-server.js",
	"mcp-server": "node dist/mcp-server.js",
	"format-check": "biome format .",

🤖 Prompt for AI Agents

In package.json around lines 30-35 the "postpack" script uses chmod which fails
on Windows; remove or replace this fragile postpack step and instead add a
shebang via the tsup build (set banner/shebang so dist/*.js are executable) or
conditionally perform chmod only on POSIX platforms (e.g., move the chmod into a
small Node script invoked from postpack that checks process.platform and runs
chmod only on non-windows), then update package.json to remove the unconditional
chmod call.

[coderabbitai]

🛠️ Refactor suggestion

Mark internal package as private to prevent accidental publish.

Given this is an internal monorepo package (not an SDK), safeguard against accidental npm publish.

 {
   "name": "@tm/core",
   "version": "1.0.0",
+  "private": true,
   "description": "Core library for Task Master - TypeScript task management system",
   "type": "module",
-  "types": "./dist/index.d.ts",
+  "types": "./src/index.ts",

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

Suggested change

	{
	"name": "@tm/core",
	"version": "1.0.0",
	"description": "Core library for Task Master - TypeScript task management system",
	"type": "module",
	"types": "./dist/index.d.ts",
	"main": "./dist/index.js",
	{
	"name": "@tm/core",
	"version": "1.0.0",
	"private": true,
	"description": "Core library for Task Master - TypeScript task management system",
	"type": "module",
	"types": "./src/index.ts",
	"main": "./dist/index.js",

🤖 Prompt for AI Agents

In packages/tm-core/package.json around lines 1 to 7 the package is not marked
private; add a top-level "private": true property (boolean true) to the
package.json to prevent accidental npm publish, keep it at the root of the JSON
object alongside name/version/type, and update any CI/lint expectations if they
validate package privacy.

[coderabbitai]

🛠️ Refactor suggestion

Align root "types" with src to match team DX preference.

Subpath exports already point "types" to ./src/*. For consistency and DX (per team preference), point root "types" to ./src/index.ts.

-"types": "./dist/index.d.ts",
+"types": "./src/index.ts",

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

Suggested change

	"types": "./dist/index.d.ts",
	"types": "./src/index.ts",

🤖 Prompt for AI Agents

In packages/tm-core/package.json around line 6, the root "types" field points to
"./dist/index.d.ts" but the project's subpath exports already reference
"./src/*"; update the root "types" to point to "./src/index.ts" to align with
team DX. Edit the package.json to replace the current types value with
"./src/index.ts", save, and run a quick build/typecheck locally to ensure no
path mismatches.

[coderabbitai]

🧹 Nitpick (assertive)

Barrel export ok; verify './task-execution' is exported from package.json with types.

Optional: switch to explicit named exports to keep API tight.

🤖 Prompt for AI Agents

In packages/workflow-engine/src/task-execution/index.ts around line 6, the
barrel export re-exports everything from './task-execution-manager.js' but the
package's public API may not expose './task-execution' and its types in
package.json; update package.json to include an export entry for
"./task-execution" (pointing to the built JS and the .d.ts types) or add it
under the "types" field so consumers get correct typings, and optionally replace
the star export with explicit named exports to tighten the public API (export
only the symbols you intend to expose).

[coderabbitai]

🛠️ Refactor suggestion

Make time fields and metadata safe for JSON boundaries.

These types likely cross IPC/JSON. Use Date | string for timestamps and avoid any.

-  startedAt: Date;
+  startedAt: Date | string;
   /** Workflow status */
   status: WorkflowStatus;
   /** Last activity timestamp */
-  lastActivity: Date;
+  lastActivity: Date | string;
   /** Execution metadata */
-  metadata?: Record<string, any>;
+  metadata?: Record<string, unknown>;

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

Suggested change

	startedAt: Date;
	/** Workflow status */
	status: WorkflowStatus;
	/** Last activity timestamp */
	lastActivity: Date;
	/** Execution metadata */
	metadata?: Record<string, any>;
	}
	startedAt: Date | string;
	/** Workflow status */
	status: WorkflowStatus;
	/** Last activity timestamp */
	lastActivity: Date | string;
	/** Execution metadata */
	metadata?: Record<string, unknown>;
	}

🤖 Prompt for AI Agents

In packages/workflow-engine/src/types/workflow.types.ts around lines 37 to 44,
the time fields and metadata currently use Date and Record<string, any> which
can break when crossing JSON/IPC boundaries; change startedAt and lastActivity
to use Date | string (so callers can pass ISO strings or Date objects) and
tighten metadata to metadata?: Record<string, unknown> (avoid any) to make the
shape JSON-safe and explicit.

[coderabbitai]

🧹 Nitpick (assertive)

Optional: strengthen event typing with a discriminated union.

Typing WorkflowEvent.data per WorkflowEventType will catch many bugs at compile time (e.g., process.output has { line: string }).

I can draft a discriminated union map (EventMap) and derive WorkflowEvent generically if useful.

🤖 Prompt for AI Agents

In packages/workflow-engine/src/types/workflow.types.ts around lines 46 to 54,
the WorkflowEvent currently uses an untyped data field which allows mismatched
payloads (e.g., process.output should be { line: string }); define an EventMap
discriminated union mapping each WorkflowEventType string to its specific
payload shape, then change WorkflowEvent to be generic/derived from EventMap so
the data field is strongly typed per event (e.g., type EventMap = {
"process.output": { line: string }, ... }; type WorkflowEvent<T extends keyof
EventMap = keyof EventMap> = { type: T; data: EventMap[T]; ... }). Update usages
to leverage the new generic so TypeScript will enforce correct payloads for each
event type.

[coderabbitai]

🛠️ Refactor suggestion

Use a serializable error shape in events instead of Error.

Error objects don’t serialize well across process boundaries. Define and use a minimal serializable shape to avoid runtime surprises.

+// Minimal serializable error representation for events
+export interface SerializedError {
+  name: string;
+  message: string;
+  stack?: string;
+  code?: string;
+}
+
 export interface WorkflowEvent {
   type: WorkflowEventType;
   workflowId: string;
   taskId: string;
   timestamp: Date;
   data?: any;
-  error?: Error;
+  error?: SerializedError;
 }

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

Suggested change

	export interface WorkflowEvent {
	type: WorkflowEventType;
	workflowId: string;
	taskId: string;
	timestamp: Date;
	data?: any;
	error?: Error;
	}
	// Minimal serializable error representation for events
	export interface SerializedError {
	name: string;
	message: string;
	stack?: string;
	code?: string;
	}
	export interface WorkflowEvent {
	type: WorkflowEventType;
	workflowId: string;
	taskId: string;
	timestamp: Date;
	data?: any;
	error?: SerializedError;
	}

🤖 Prompt for AI Agents

In packages/workflow-engine/src/types/workflow.types.ts around lines 56 to 63,
the WorkflowEvent currently uses Error for the error field which isn’t reliably
serializable; replace it with a minimal SerializableError interface (e.g. {
message: string; name?: string; stack?: string; code?: string|number; [key:
string]: any }) and change error?: Error to error?: SerializableError, then
update any imports/usages where WorkflowEvent is constructed or consumed to map
Error objects to this plain shape before sending across process boundaries
(populate message/name/stack/code as available).

[coderabbitai]

🧹 Nitpick (assertive)

Barrel export ok; ensure subpath export exists and types are published.

Check package exports for './worktree' and include 'types' mapping; consider explicit named exports when API stabilizes.

🤖 Prompt for AI Agents

In packages/workflow-engine/src/worktree/index.ts at line 6, the barrel export
exists but the package's public exports likely don't expose the './worktree'
subpath or its types; update package.json "exports" to include a './worktree'
entry that points to the built JS file and a "types" entry that points to the
corresponding .d.ts, ensure the build outputs the .d.ts into the package (or add
an index.d.ts alongside the built JS), and confirm tsconfig and the publish
artifacts include those type files; optionally, when the API stabilizes replace
the star export with explicit named exports to make future type publishing and
export surface clearer.

[coderabbitai]

Review continued from previous batch...

[coderabbitai]

⚠️ Potential issue

Fix exports: types path mismatch and invalid "require" target

"exports['.'].types" points to src while "files" excludes src; "require" points to an ESM file. Align to dist types and drop "require" unless you ship CJS.

   "exports": {
     ".": {
-      "types": "./src/index.ts",
-      "import": "./dist/index.js",
-      "require": "./dist/index.js"
+      "types": "./dist/index.d.ts",
+      "import": "./dist/index.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.

Suggested change

	"exports": {
	".": {
	"types": "./src/index.ts",
	"import": "./dist/index.js",
	"require": "./dist/index.js"
	}
	},
	"files": ["dist", "README.md"],
	"exports": {
	".": {
	"types": "./dist/index.d.ts",
	"import": "./dist/index.js"
	}
	},
	"files": ["dist", "README.md"],

🤖 Prompt for AI Agents

In apps/cli/package.json around lines 8 to 15, the exports block points "types"
at ./src/index.ts (which is not shipped) and sets "require" to an ESM-built
file; change "exports['.'].types" to point to the built declaration file
(./dist/index.d.ts) and remove the "require" entry unless you are shipping a CJS
build (if you do ship CJS, point "require" to the CJS bundle in dist instead);
ensure the "files" array contains dist so the .d.ts will be published.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Add CLI bin entries so the tool is runnable (tm and task-master aliases)

Without "bin", the package won’t install an executable.

   },
+  "bin": {
+    "tm": "./dist/index.js",
+    "task-master": "./dist/index.js"
+  },

Follow-up: ensure the built entry has a shebang and tsup preserves it (e.g., shebang in src/index.ts and tsup preserveShebang).

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "=== apps/cli/package.json (first 400 lines) ==="
if [ -f apps/cli/package.json ]; then
  sed -n '1,400p' apps/cli/package.json || true
else
  echo "MISSING: apps/cli/package.json"
fi

echo
echo "=== Search for existing 'bin' in repo/package.json files ==="
rg -n --hidden --no-messages '"bin"\s*:' || true

echo
echo "=== Check for built entry apps/cli/dist/index.js ==="
if [ -f apps/cli/dist/index.js ]; then
  echo "FOUND: apps/cli/dist/index.js (first 5 lines):"
  sed -n '1,5p' apps/cli/dist/index.js || true
else
  echo "NOT FOUND: apps/cli/dist/index.js"
  if [ -d apps/cli/dist ]; then
    echo "apps/cli/dist directory listing:"
    ls -la apps/cli/dist || true
  fi
fi

echo
echo "=== Find possible source entry files under apps/cli ==="
rg -n --hidden --no-messages '(?:^|/)(index|cli)\.(ts|js|mjs|cjs)$' apps/cli || true

echo
echo "=== Print first 8 lines of candidate source files ==="
for f in $(rg -n --hidden --no-messages '(?:^|/)(index|cli)\.(ts|js|mjs|cjs)$ apps/cli --files); do
  echo "---- $f ----"
  sed -n '1,8p' "$f" || true
done

echo
echo "=== Search for shebangs in apps/cli source files ==="
rg -n --hidden '^\s*#!' apps/cli || true

echo
echo "=== Search for tsup usage/config in repo ==="
rg -n --hidden --no-messages 'tsup' || true
rg -n --hidden --no-messages 'preserveShebang' || true
rg -n --hidden --no-messages 'tsup.config' || true

echo
echo "=== Search for build scripts mentioning tsup in package.json files ==="
rg -n --hidden --no-messages '"build"\s*:\s*".*tsup' || true

Length of output: 5677

---

Add CLI bin entries so the tool is runnable (tm and task-master aliases)

File: apps/cli/package.json — package currently lacks "bin" (npm won't install executables). Add:

   },
+  "bin": {
+    "tm": "./dist/index.js",
+    "task-master": "./dist/index.js"
+  },

Ensure the CLI entry source has a shebang and tsup is configured to preserve it so ./dist/index.js is executable.

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

Suggested change

	},
	"files": ["dist", "README.md"],
	},
	"bin": {
	"tm": "./dist/index.js",
	"task-master": "./dist/index.js"
	},
	"files": ["dist", "README.md"],

🤖 Prompt for AI Agents

In apps/cli/package.json around lines 14-15, add a "bin" section that maps the
executable names to the built entry (e.g., "task-master" and "tm" ->
"./dist/index.js") so npm will install CLI executables; ensure the CLI source
entry file starts with a node shebang (#!/usr/bin/env node) and update
tsup/build config to preserve that shebang in the output (use tsup's
shebang/banner option or enable shebang preservation) so ./dist/index.js is
executable after build.

[coderabbitai]

⚠️ Potential issue

Fix import: WorkflowCommand is a named export, not default.

Importing it as default will be undefined at runtime.

-import WorkflowCommand from './workflow.command.js';
+import { WorkflowCommand } from './workflow.command.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.

Suggested change

	import WorkflowCommand from './workflow.command.js';
	import { WorkflowCommand } from './workflow.command.js';

🤖 Prompt for AI Agents

In apps/cli/src/commands/index.ts around lines 8-9, the file imports
WorkflowCommand as a default but the module exports it as a named export;
replace the default import with a named import (e.g., import { WorkflowCommand }
from './workflow.command.js';), verify the exported identifier in
workflow.command.js matches exactly, and update any references if the import
name differs so the runtime no longer receives undefined.

[coderabbitai]

🧹 Nitpick (assertive)

Optional: return created command instances for testing/inspection.

Returning instances helps e2e tests assert wiring without relying on Commander’s internal state.

-export function registerAllCommands(program: Command): void {
+export function registerAllCommands(program: Command) {
+	const registered: any[] = [];
 	// Get all exports from this module
 	const commands = [
 		ListTasksCommand,
 		AuthCommand,
 		WorkflowCommand
 		// Add new commands here as they're imported above
 	];
 
 	commands.forEach((CommandClass) => {
 		if (
 			'register' in CommandClass &&
 			typeof (CommandClass as any).register === 'function'
 		) {
-			CommandClass.register(program);
+			const instance = (CommandClass as any).register(program);
+			if (instance) registered.push(instance);
 		}
 	});
+	return registered;
 }

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

Suggested change

	export function registerAllCommands(program: Command): void {
	// Get all exports from this module
	const commands = [
	ListTasksCommand,
	AuthCommand,
	WorkflowCommand
	// Add new commands here as they're imported above
	];
	commands.forEach((CommandClass) => {
	if (
	'register' in CommandClass &&
	typeof CommandClass.register === 'function'
	) {
	CommandClass.register(program);
	}
	});
	}
	export function registerAllCommands(program: Command) {
	const registered: any[] = [];
	// Get all exports from this module
	const commands = [
	ListTasksCommand,
	AuthCommand,
	WorkflowCommand
	// Add new commands here as they're imported above
	];
	commands.forEach((CommandClass) => {
	if (
	'register' in CommandClass &&
	typeof (CommandClass as any).register === 'function'
	) {
	const instance = (CommandClass as any).register(program);
	if (instance) registered.push(instance);
	}
	});
	return registered;
	}

🤖 Prompt for AI Agents

In apps/cli/src/commands/index.ts around lines 21 to 38, the registerAllCommands
function currently registers command classes but does not return the
instantiated command objects; change it to instantiate each command class (e.g.
new CommandClass()), call its register(program) method, collect those instances
into an array, and return that array so tests can inspect wiring; update the
function signature to return the array type and ensure any callers are updated
to accept/ignore the returned instances.

[coderabbitai]

🧹 Nitpick (assertive)

Use static register for aliases to keep wiring consistent

Prefer the same registration path for alias commands to ensure shared options/hooks remain identical.

-    this.addCommand(new WorkflowStartCommand('run'));  // tm workflow run <task-id>
-    this.addCommand(new WorkflowStopCommand('kill'));   // tm workflow kill <workflow-id>
-    this.addCommand(new WorkflowStatusCommand('info')); // tm workflow info <workflow-id>
+    WorkflowStartCommand.register(this, 'run');   // tm workflow run <task-id>
+    WorkflowStopCommand.register(this, 'kill');   // tm workflow kill <workflow-id>
+    WorkflowStatusCommand.register(this, 'info'); // tm workflow info <workflow-id>

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

Suggested change

	this.addCommand(new WorkflowStartCommand('run')); // tm workflow run <task-id>
	this.addCommand(new WorkflowStopCommand('kill')); // tm workflow kill <workflow-id>
	this.addCommand(new WorkflowStatusCommand('info')); // tm workflow info <workflow-id>
	}
	WorkflowStartCommand.register(this, 'run'); // tm workflow run <task-id>
	WorkflowStopCommand.register(this, 'kill'); // tm workflow kill <workflow-id>
	WorkflowStatusCommand.register(this, 'info'); // tm workflow info <workflow-id>
	}

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow.command.ts around lines 42 to 45, the alias
commands are being added via this.addCommand(new WorkflowStartCommand('run')) /
new WorkflowStopCommand('kill') / new WorkflowStatusCommand('info') which
bypasses the class-level wiring; replace these calls to use each command's
static register method (e.g. WorkflowStartCommand.register(this, 'run'),
WorkflowStopCommand.register(this, 'kill'), WorkflowStatusCommand.register(this,
'info')) so aliases are registered through the same path and inherit shared
options/hooks.

[coderabbitai]

🧹 Nitpick (assertive)

Add language to ASCII diagram fence (MD040)

Specify text to satisfy markdownlint.

-```
+```text
 TaskExecutionManager
 ...

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.17.2)</summary>

17-17: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</details>

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

In packages/workflow-engine/README.md around lines 17 to 22, the Markdown code
fence for the ASCII diagram lacks a language specifier triggering MD040; update
the opening triple-backtick to include a language token such as text (e.g.,

the diagram content unchanged and keeping the closing triple-backticks as-is.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Double-check API names and sync/async semantics in Quick Start

Ensure getWorkflowStatus signature and return type match implementation (sync vs async) and that startTaskExecution returns the documented ID.

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
rg -nP --glob 'packages/workflow-engine/src/**' '\bclass\s+TaskExecutionManager\b.*|getWorkflowStatus\s*\(|startTaskExecution\s*\(' -C2

Length of output: 1586

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "=== startTaskExecution (lines 90-200) ==="
sed -n '90,200p' packages/workflow-engine/src/task-execution/task-execution-manager.ts || true
echo
echo "=== getWorkflowStatus (lines 286-305) ==="
sed -n '286,305p' packages/workflow-engine/src/task-execution/task-execution-manager.ts || true
echo
echo "=== 'return' occurrences in the file (context) ==="
rg -n --glob 'packages/workflow-engine/src/**' '\breturn\b' -n packages/workflow-engine/src/task-execution/task-execution-manager.ts || true
echo
echo "=== occurrences of 'workflowId' in the file ==="
rg -n --glob 'packages/workflow-engine/src/**' '\bworkflowId\b' packages/workflow-engine/src || true

Length of output: 18148

---

Double-check API names and sync/async semantics in Quick Start

Verified: startTaskExecution is async and returns Promise (it returns a workflowId). getWorkflowStatus is synchronous and returns WorkflowExecutionContext | undefined — guard against undefined before accessing workflow.status.

Location: packages/workflow-engine/README.md (lines 40–55)

🤖 Prompt for AI Agents

In packages/workflow-engine/README.md around lines 40 to 55, the quick-start
uses startTaskExecution correctly as async but reads getWorkflowStatus without
guarding for its possibly undefined return; update the README example to call
manager.getWorkflowStatus(workflowId) then check for undefined (e.g., if
(!workflow) { handle/log and return/continue } else access workflow.status) so
you never dereference undefined; keep startTaskExecution and stopTaskExecution
as awaited async calls.

[coderabbitai]

🧹 Nitpick (assertive)

Surround shell code fences with blank lines (MD031)

Add blank lines before and after the fenced blocks in Troubleshooting section.

-   ```bash
+   
+   ```bash
    # Check git version (requires 2.5+)
    git --version
    ...
-   ```
+   ```
+
-   ```bash
+   
+   ```bash
    # Check Claude installation
    which claude
    ...
-   ```
+   ```
+
-   ```bash
+   
+   ```bash
    # Check worktree directory permissions
    chmod -R 755 ./worktrees
-   ```
+   ```

🧰 Tools

🪛 markdownlint-cli2 (0.17.2)

319-319: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

328-328: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

---

337-337: Fenced code blocks should be surrounded by blank lines

(MD031, blanks-around-fences)

🤖 Prompt for AI Agents

packages/workflow-engine/README.md around lines 319 to 341: the fenced bash code
blocks in the Troubleshooting section are not surrounded by blank lines (MD031);
add a blank line before each opening ```bash fence and a blank line after each
closing ``` fence so every fenced block is separated from surrounding text,
ensuring consistent Markdown formatting.

[coderabbitai]

🧹 Nitpick (assertive)

Terminate file with a single trailing newline (MD047)

-MIT WITH Commons-Clause
+MIT WITH Commons-Clause
+

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

Suggested change

	MIT WITH Commons-Clause
	MIT WITH Commons-Clause

🧰 Tools

🪛 markdownlint-cli2 (0.17.2)

371-371: Files should end with a single newline character

(MD047, single-trailing-newline)

🤖 Prompt for AI Agents

packages/workflow-engine/README.md around line 371: the file currently ends
without a trailing newline which violates MD047; add a single newline character
at the end of the file so the final line ("MIT WITH Commons-Clause") is followed
by one trailing newline and save the file.

[coderabbitai]

🧹 Nitpick (assertive)

Registry import: ensure package resolution and align tag-resolution guideline.

• Verify @tm/cli is resolvable at runtime from this script (Node path/workspace alias). If this runs outside TS build, ensure it points to built JS.
• Guidelines state command files must import and use getCurrentTag. You’re using taskMaster.getCurrentTag(). Either (a) import getCurrentTag and apply the documented pattern options.tag || getCurrentTag(projectRoot) || 'master', or (b) update the guideline to accept the TaskMaster facade pattern and ensure it follows the same precedence internally.

🤖 Prompt for AI Agents

In scripts/modules/commands.js around lines 17 to 20, the import from '@tm/cli'
may not resolve at runtime and the file violates the tag-resolution guideline by
calling taskMaster.getCurrentTag(); ensure the module path points to the built
JS entry (or adjust Node resolution/aliases) so @tm/cli is resolvable when
running outside the TS build, and change the tag handling to follow the
documented pattern: import getCurrentTag from @tm/cli and use options.tag ||
getCurrentTag(projectRoot) || 'master' (or, if you prefer to keep the TaskMaster
facade, update the facade to implement that same precedence internally and
document it).

[coderabbitai]

Review continued from previous batch...

[coderabbitai]

🧹 Nitpick (assertive)

Add “Breaking Changes & Deprecations” and “Rollback” sections for clarity.

Documenting (even “None yet”) aligns with our guidelines and eases adoption.

Apply this diff to append sections:

+ 
+ ## Breaking Changes & Deprecations
+ 
+ As of 2025-09-11: None. This section will enumerate any CLI flags, config keys, or APIs removed/changed, with migration steps.
+ 
+ ## Rollback & Recovery
+ 
+ - Adapters/bridges allow immediate fallback to legacy implementations.
+ - If a migrated command regresses, disable the new path and track via this roadmap.
+ - Always keep a restorable backup of user config/state during migrations.

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

Suggested change

	This is a living document - update as the migration progresses!
	This is a living document - update as the migration progresses!
	## Breaking Changes & Deprecations
	As of 2025-09-11: None. This section will enumerate any CLI flags, config keys, or APIs removed/changed, with migration steps.
	## Rollback & Recovery
	- Adapters/bridges allow immediate fallback to legacy implementations.
	- If a migrated command regresses, disable the new path and track via this roadmap.
	- Always keep a restorable backup of user config/state during migrations.

🤖 Prompt for AI Agents

.taskmaster/docs/MIGRATION-ROADMAP.md around line 188: the reviewer asks to
append two new headings "Breaking Changes & Deprecations" and "Rollback" (even
if empty) to improve clarity and align with documentation guidelines; update the
document by adding both section headers after the existing line with brief
placeholder content such as "None yet" or a short guidance sentence for each, so
future migration notes can be populated and readers know where to find
deprecation details and rollback procedures.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Reuse core type to avoid drift

If @tm/core already exports ListTasksResult, import it instead of redefining here.

Run to confirm export exists:

---

🏁 Script executed:

#!/bin/bash
rg -nP 'export\s+(type|interface)\s+ListTasksResult\b' packages/tm-core -C2

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "PWD: $(pwd)"
echo
echo "---- Searching for symbol 'ListTasksResult' across repo ----"
rg -n --hidden --no-ignore-vcs -S '\bListTasksResult\b' -C2 || true

echo
echo "---- Searching for explicit export declarations of ListTasksResult (type|interface) ----"
rg -nP --hidden --no-ignore-vcs 'export\s+(type|interface)\s+ListTasksResult\b' -C2 || true

echo
echo "---- Searching for re-exports / grouped exports mentioning ListTasksResult ----"
rg -nP --hidden --no-ignore-vcs 'export\s+\{[^}]*ListTasksResult[^}]*\}' -C2 || true
rg -nP --hidden --no-ignore-vcs 'export\s+type\s+\{[^}]*ListTasksResult[^}]*\}' -C2 || true

echo
echo "---- Locating any 'tm-core' package directories under packages/ ----"
find packages -maxdepth 3 -type d -name 'tm-core' -print -exec bash -c 'echo "Contents of {}:"; ls -la "{}" | sed -n "1,200p"' \; || true

echo
echo "---- If packages/tm-core exists, searching inside it explicitly ----"
if [ -d packages/tm-core ]; then
  rg -n --hidden --no-ignore-vcs -S '\bListTasksResult\b' packages/tm-core -C2 || true
  rg -nP --hidden --no-ignore-vcs 'export\s+(type|interface)\s+ListTasksResult\b' packages/tm-core -C2 || true
  rg -nP --hidden --no-ignore-vcs 'export\s+\{[^}]*ListTasksResult[^}]*\}' packages/tm-core -C2 || true
fi

echo
echo "---- Done ----"

Length of output: 9452

---

Import core ListTasksResult instead of redefining

tm-core already exports ListTasksResult — replace the local interface in apps/cli/src/commands/list.command.ts (lines ~35–41) with an import from the core package (e.g. import type { ListTasksResult } from '@tm/core';). Evidence: packages/tm-core/src/task-master-core.ts:28 (re-exports TaskListResult as ListTasksResult) and packages/tm-core/src/index.ts:11 (exports ListTasksResult). Remove the local interface to avoid type drift.

🤖 Prompt for AI Agents

In apps/cli/src/commands/list.command.ts around lines 35 to 41, the local
ListTasksResult interface duplicates the type exported by the tm-core package;
remove this local interface and add an import type { ListTasksResult } from
'@tm/core' at the top of the file, update any references to use the imported
type, and ensure the file's imports are sorted/organized accordingly to avoid
type drift.

[coderabbitai]

🛠️ Refactor suggestion

Avoid direct process.exit; use Commander error handling for testability

Replace hard exits with Command#error so CLI can opt into exitOverride in tests.

-      if (!this.validateOptions(options)) {
-        process.exit(1);
-      }
+      if (!this.validateOptions(options)) {
+        this.error('Invalid options', { exitCode: 1 });
+        return;
+      }

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

Suggested change

	if (!this.validateOptions(options)) {
	process.exit(1);
	}
	if (!this.validateOptions(options)) {
	this.error('Invalid options', { exitCode: 1 });
	return;
	}

🤖 Prompt for AI Agents

In apps/cli/src/commands/list.command.ts around lines 78 to 81, replace the
direct process.exit(1) call used when validateOptions fails with Commander-style
error handling so tests can opt into exitOverride; call the command's error
method (e.g., this.error('Invalid options', { exitCode: 1 }) or throw a
CommanderError) instead of process.exit so the CLI surface reports the failure
through Commander and remains testable.

[coderabbitai]

🛠️ Refactor suggestion

Always release resources (close tm-core) on success and error

Call cleanup on both paths to prevent handle leaks in tests/REPL usage.

       if (!options.silent) {
         this.displayResults(result, options);
       }
+      // Always cleanup on success
+      await this.cleanup();
     } catch (error: any) {
-      const msg = error?.getSanitizedDetails?.() ?? {
-        message: error?.message ?? String(error)
-      };
-      console.error(chalk.red(`Error: ${msg.message || 'Unexpected error'}`));
-      if (error.stack && process.env.DEBUG) {
-        console.error(chalk.gray(error.stack));
-      }
-      process.exit(1);
+      const msg = error?.getSanitizedDetails?.() ?? { message: error?.message ?? String(error) };
+      if (error.stack && process.env.DEBUG) console.error(chalk.gray(error.stack));
+      await this.cleanup();
+      this.error(`Error: ${msg.message || 'Unexpected error'}`, { exitCode: 1 });
     }

Also applies to: 100-104

🤖 Prompt for AI Agents

In apps/cli/src/commands/list.command.ts around lines 91 to 95 (and apply same
to 100-104), the code currently only calls displayResults on success and does
not release tm-core when returning or throwing; modify the function to always
release/cleanup tm-core by moving the cleanup call into a finally block (or
equivalent) that runs after success or error, await the close/cleanup method and
guard for tm-core being non-null to avoid runtime errors, and rethrow or
propagate the original error after cleanup so behavior is unchanged.

[coderabbitai]

🧹 Nitpick (assertive)

Align with team preference: avoid node: scheme for core modules

Per retrieved learnings, prefer bare imports for Node core modules.

-import path from 'node:path';
+import path from 'path';

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

Suggested change

	import path from 'node:path';
	import path from 'path';

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-start.command.ts around line 8, the
import uses the node: scheme ("import path from 'node:path';") which the team
prefers to avoid; change it to a bare core module import by replacing it with
"import path from 'path';" (or the equivalent require style if codebase uses
commonjs) and ensure any linter/tsconfig settings still accept bare imports.

[coderabbitai]

🧹 Nitpick (assertive)

Consider scoping the placeholder adapter to internal modules

Exporting both IStorage and a separate StorageAdapter can confuse consumers; keep the placeholder internal (or mark it @internal) until real adapters land.

Also applies to: 28-46

🤖 Prompt for AI Agents

In packages/tm-core/src/storage/index.ts around lines 17-23 (and similarly
28-46), the placeholder StorageAdapter interface is exported which can confuse
consumers; make the placeholder internal by either removing the export (make it
file-private/unexported) or annotating it with a JSDoc @internal tag and update
any references accordingly so only the intended public type (e.g., IStorage) is
exported; ensure public API exports remain unchanged and add a short comment
indicating the adapter is a temporary/internal placeholder.

[coderabbitai]

🛠️ Refactor suggestion

Fix falsey read values being coerced to null

|| null will treat empty string as null. Use nullish coalescing.

-  async read(path: string): Promise<string | null> {
-    return this.data.get(path) || null;
-  }
+  async read(path: string): Promise<string | null> {
+    return this.data.get(path) ?? null;
+  }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	async read(path: string): Promise<string | null> {
	return this.data.get(path) || null;
	}
	async read(path: string): Promise<string | null> {
	return this.data.get(path) ?? null;
	}

🤖 Prompt for AI Agents

In packages/tm-core/src/storage/index.ts around lines 31 to 33, the read()
method uses "|| null" which coerces falsy but valid values (like empty string)
to null; change the fallback to the nullish coalescing operator so that only
undefined becomes null (replace "return this.data.get(path) || null;" with a
nullish coalescing fallback).

[coderabbitai]

⚠️ Potential issue

Initialize core before use in this test

emptyCore is used without initialize(); this can cause nondeterminism depending on implementation.

       const emptyCore = createTaskMasterCore(emptyDir);
+      await emptyCore.initialize();
 
       try {
         const result = await emptyCore.listTasks();

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

Suggested change

	const emptyDir = await fs.mkdtemp(path.join(os.tmpdir(), 'tm-empty-'));
	const emptyCore = createTaskMasterCore(emptyDir);
	try {
	const result = await emptyCore.listTasks();
	expect(result.tasks).toHaveLength(0);
	expect(result.total).toBe(0);
	expect(result.filtered).toBe(0);
	const emptyDir = await fs.mkdtemp(path.join(os.tmpdir(), 'tm-empty-'));
	const emptyCore = createTaskMasterCore(emptyDir);
	await emptyCore.initialize();
	try {
	const result = await emptyCore.listTasks();
	expect(result.tasks).toHaveLength(0);
	expect(result.total).toBe(0);
	expect(result.filtered).toBe(0);

🤖 Prompt for AI Agents

In packages/tm-core/tests/integration/list-tasks.test.ts around lines 309 to
317, the test creates emptyCore but calls listTasks() before initializing it,
which can cause nondeterministic behavior; fix by awaiting
emptyCore.initialize() (or the appropriate init method) immediately after
createTaskMasterCore(emptyDir) and before calling emptyCore.listTasks(),
ensuring the core is fully initialized before use.

[coderabbitai]

🧹 Nitpick (assertive)

Tag-file behavior depends on storage strategy—confirm contract

This suite creates feature-branch.json directly. Ensure the storage layer officially supports per-tag files (vs single tasks.json) and document precedence/merge semantics.

🤖 Prompt for AI Agents

packages/tm-core/tests/integration/list-tasks.test.ts lines 371-405: the test
writes a per-tag file (feature-branch.json) directly but the storage contract
may only support a single tasks.json or have specific precedence/merge
semantics; verify the storage strategy and change the test to use the official
storage API (or a helper) to create tag-scoped tasks if the implementation
supports per-tag files, otherwise revert to creating tasks via the canonical
single-file format; update or add a short comment in the test describing the
expected precedence/merge behavior (which file wins) and, if needed, add an
assertion that validates the storage layer’s actual precedence/merge behavior so
the test aligns with the documented contract.

[coderabbitai]

🛠️ Refactor suggestion

Avoid order-dependent assertion

Don’t assume first item ordering; assert membership instead.

       it('should list default tasks when no tag specified', async () => {
         const result = await tmCore.listTasks();
 
-        expect(result.tasks).toHaveLength(5);
-        expect(result.tasks[0].id).toBe('1');
+        expect(result.tasks).toHaveLength(5);
+        expect(result.tasks.some((t) => t.id === '1')).toBe(true);
       });

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

Suggested change

	it('should list default tasks when no tag specified', async () => {
	const result = await tmCore.listTasks();
	expect(result.tasks).toHaveLength(5);
	expect(result.tasks[0].id).toBe('1');
	});
	it('should list default tasks when no tag specified', async () => {
	const result = await tmCore.listTasks();
	expect(result.tasks).toHaveLength(5);
	expect(result.tasks.some((t) => t.id === '1')).toBe(true);
	});

🤖 Prompt for AI Agents

In packages/tm-core/tests/integration/list-tasks.test.ts around lines 415 to
420, the test currently assumes a specific ordering (checking result.tasks[0].id
=== '1'); change it to assert membership instead: keep the length assertion,
then derive the array of task ids (e.g., map result.tasks to ids) and assert
that the ids include '1' (use toContain or expect.arrayContaining) so the test
no longer depends on item order.

[coderabbitai]

Review continued from previous batch...

[coderabbitai]

🧹 Nitpick (assertive)

Match import style preference (avoid node: scheme).

-import path from 'node:path';
+import path from 'path';

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

Suggested change

	import path from 'node:path';
	import path from 'path';

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-stop.command.ts around line 8, the
import uses the node: scheme ("node:path"); change it to the plain Node core
import ("path") to match project import style—replace import path from
'node:path' with import path from 'path' and run lint/typecheck to ensure no
other occurrences need updating.

[coderabbitai]

🧹 Nitpick (assertive)

Consider prompting for other active states too.

You confirm only for running/paused. Decide if pending/initializing should also prompt.

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-stop.command.ts around lines 135 to
145, the confirmation prompt only checks for 'running' and 'paused' states but
the reviewer suggests also prompting for other active states like 'pending' and
'initializing'; update the conditional to include those states (e.g.,
['running','paused','pending','initializing']) so the UI.confirm is triggered
for any active workflow state you consider sensitive, and keep the existing
force bypass and cancellation handling unchanged.

[coderabbitai]

🧹 Nitpick (assertive)

Ensure spinner lifecycle ends on both success and error paths.

Confirm ui.displaySpinner stops/clears; otherwise wrap with a try/finally and explicit .success()/.fail().

I can patch this once I see the spinner API; point me to apps/cli/src/utils/ui.ts.

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-stop.command.ts around lines 147 to
153, the spinner started with ui.displaySpinner('Stopping workflow and cleaning
up resources...') may not be stopped on error; wrap the await
this.workflowManager!.stopTaskExecution(...) in a try/finally so the spinner is
always cleared, calling spinner.success() in the try when the stop completes and
spinner.fail() in the catch (or call fail() in finally if an exception
occurred), or at minimum call spinner.clear()/stop() in finally; reference the
spinner methods from apps/cli/src/utils/ui.ts to use the correct API.

[coderabbitai]

🧹 Nitpick (assertive)

Optional: stop workflows concurrently to reduce wait time.

Safe since each stop is isolated; errors are aggregated.

-    let stopped = 0;
-    let failed = 0;
-
-    for (const workflow of activeWorkflows) {
-      try {
-        const workflowId = `workflow-${workflow.taskId}`;
-        await this.workflowManager!.stopTaskExecution(
-          workflowId,
-          options.force
-        );
-        stopped++;
-      } catch (error) {
-        console.error(
-          `${chalk.red('✗')} Failed to stop workflow ${workflow.taskId}: ${error}`
-        );
-        failed++;
-      }
-    }
+    const results = await Promise.allSettled(
+      activeWorkflows.map((w) =>
+        this.workflowManager!.stopTaskExecution(`workflow-${w.taskId}`, options.force)
+      )
+    );
+    const stopped = results.filter((r) => r.status === 'fulfilled').length;
+    const failed = results.length - stopped;
+    results.forEach((r, i) => {
+      if (r.status === 'rejected') {
+        const msg = (r.reason as any)?.message ?? String(r.reason);
+        console.error(`${chalk.red('✗')} Failed to stop workflow ${activeWorkflows[i].taskId}: ${msg}`);
+      }
+    });

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

Suggested change

	ui.displaySpinner('Stopping all workflows...');
	let stopped = 0;
	let failed = 0;
	for (const workflow of activeWorkflows) {
	try {
	const workflowId = `workflow-${workflow.taskId}`;
	await this.workflowManager!.stopTaskExecution(
	workflowId,
	options.force
	);
	stopped++;
	} catch (error) {
	console.error(
	`${chalk.red('✗')} Failed to stop workflow ${workflow.taskId}: ${error}`
	);
	failed++;
	}
	}
	ui.displaySpinner('Stopping all workflows...');
	const results = await Promise.allSettled(
	activeWorkflows.map((w) =>
	this.workflowManager!.stopTaskExecution(`workflow-${w.taskId}`, options.force)
	)
	);
	const stopped = results.filter((r) => r.status === 'fulfilled').length;
	const failed = results.length - stopped;
	results.forEach((r, i) => {
	if (r.status === 'rejected') {
	const msg = (r.reason as any)?.message ?? String(r.reason);
	console.error(`${chalk.red('✗')} Failed to stop workflow ${activeWorkflows[i].taskId}: ${msg}`);
	}
	});

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-stop.command.ts around lines 199 to
218, the loop stops workflows sequentially which increases wait time; convert
this to concurrent stops using Promise.allSettled: build an array of stop
promises (calling this.workflowManager!.stopTaskExecution for each workflow with
the computed workflowId and options.force), await Promise.allSettled on that
array, then iterate results to count stopped vs failed and log errors for
rejected results (including workflow.taskId context); ensure any UI spinner or
final summary logic runs after all promises settle.

[coderabbitai]

🧹 Nitpick (assertive)

Log error messages, not raw objects.

-        console.error(
-          `${chalk.red('✗')} Failed to stop workflow ${workflow.taskId}: ${error}`
-        );
+        const msg = (error as any)?.message ?? String(error);
+        console.error(`${chalk.red('✗')} Failed to stop workflow ${workflow.taskId}: ${msg}`);

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

Suggested change

	console.error(
	`${chalk.red('✗')} Failed to stop workflow ${workflow.taskId}: ${error}`
	);
	failed++;
	const msg = (error as any)?.message ?? String(error);
	console.error(`${chalk.red('✗')} Failed to stop workflow ${workflow.taskId}: ${msg}`);
	failed++;

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-stop.command.ts around lines 213 to
216, the code currently interpolates a raw error object into the log which can
produce "[object Object]" or include non-informative output; change the logging
to output a string representation of the error (for example use error instanceof
Error ? error.message : String(error)) or JSON.stringify/inspect the error to
include message and stack, and pass that string into console.error (preserve the
existing chalk formatting and increment of failed).

[coderabbitai]

🛠️ Refactor suggestion

Remove modulo bias in generateRandomString().

Modulo 36 over 256 introduces slight bias. Use rejection sampling.

- function generateRandomString(length: number): string {
-  const chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789';
-  const bytes = randomBytes(length);
-  let result = '';
-
-  for (let i = 0; i < length; i++) {
-    result += chars[bytes[i] % chars.length];
-  }
-
-  return result;
- }
+ function generateRandomString(length: number): string {
+  const chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789';
+  const base = chars.length; // 36
+  const maxMultiple = Math.floor(256 / base) * base; // 252
+  let result = '';
+  while (result.length < length) {
+    const buf = randomBytes(length); // oversample; small loop, short length
+    for (let i = 0; i < buf.length && result.length < length; i++) {
+      const v = buf[i];
+      if (v < maxMultiple) {
+        result += chars[v % base];
+      }
+    }
+  }
+  return result;
+ }

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

Suggested change

	function generateRandomString(length: number): string {
	const chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789';
	const bytes = randomBytes(length);
	let result = '';
	for (let i = 0; i < length; i++) {
	result += chars[bytes[i] % chars.length];
	}
	return result;
	function generateRandomString(length: number): string {
	const chars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789';
	const base = chars.length; // 36
	const maxMultiple = Math.floor(256 / base) * base; // 252
	let result = '';
	while (result.length < length) {
	const buf = randomBytes(length); // oversample; small loop, short length
	for (let i = 0; i < buf.length && result.length < length; i++) {
	const v = buf[i];
	if (v < maxMultiple) {
	result += chars[v % base];
	}
	}
	}
	return result;
	}

🤖 Prompt for AI Agents

In packages/tm-core/src/utils/id-generator.ts around lines 70 to 79, the current
generateRandomString() uses bytes[i] % chars.length which causes modulo bias;
replace it with rejection sampling: generate random bytes and for each needed
character, loop until you get a byte < 252 (floor(256 / 36) * 36) then use byte
% 36 as the index into chars; continue until the requested length is reached,
requesting more random bytes as necessary, and preserve the function signature
and charset.

[coderabbitai]

🧹 Nitpick (assertive)

Use bare imports instead of node: scheme for consistency.

Based on the retrieved learnings, the user Crunchyman-ralph prefers not to use node: scheme imports.

Apply this diff:

-import { spawn } from 'node:child_process';
-import { promises as fs } from 'node:fs';
-import path from 'node:path';
+import { spawn } from 'child_process';
+import { promises as fs } from 'fs';
+import path from 'path';

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

Suggested change

	import { spawn } from 'node:child_process';
	import { promises as fs } from 'node:fs';
	import path from 'node:path';
	import { spawn } from 'child_process';
	import { promises as fs } from 'fs';
	import path from 'path';

🤖 Prompt for AI Agents

In packages/workflow-engine/src/worktree/worktree-manager.ts around lines 6 to
8, the imports use the "node:" scheme (import { spawn } from
'node:child_process'; import { promises as fs } from 'node:fs'; import path from
'node:path';) but the project prefers bare imports; replace them with
bare-module imports: import { spawn } from 'child_process'; import { promises as
fs } from 'fs'; import path from 'path'; and update any other occurrences in the
file to match, then run lint/TS checks to ensure no import-type errors remain.

[coderabbitai]

⚠️ Potential issue

Missing taskId assignment causes removeWorktree to fail.

The cleanupAll method calls removeWorktree with worktree.taskId, but the taskId field is not properly populated when listing worktrees. The listWorktrees method filters for task worktrees but doesn't guarantee the taskId field is set correctly in the returned objects.

The issue is that completeWorktreeInfo at line 331 tries to extract taskId from the branch name, but if that fails, it falls back to partial.taskId || 'unknown'. Since partial comes from parsing git output which doesn't include taskId, this will always result in 'unknown' for listed worktrees.

Apply this diff to properly handle the taskId extraction:

 async cleanupAll(force = false): Promise<void> {
     const worktrees = await this.listWorktrees();
     
     for (const worktree of worktrees) {
-        if (worktree.taskId) {
+        // Extract taskId from the path if not available
+        let taskId = worktree.taskId;
+        if (!taskId || taskId === 'unknown') {
+            const match = worktree.path.match(/task-(.+?)$/);
+            taskId = match?.[1];
+        }
+        
+        if (taskId && taskId !== 'unknown') {
             try {
-                await this.removeWorktree(worktree.taskId, force);
+                // Check if this worktree is in our active map
+                if (this.activeWorktrees.has(taskId)) {
+                    await this.removeWorktree(taskId, force);
+                } else {
+                    // Directly remove via git command for orphaned worktrees
+                    const args = ['worktree', 'remove', worktree.path];
+                    if (force) args.push('--force');
+                    await this.executeGitCommand(args, { cwd: this.config.projectRoot });
+                }
             } catch (error) {
-                console.error(`Failed to cleanup worktree for task ${worktree.taskId}:`, error);
+                console.error(`Failed to cleanup worktree for task ${taskId}:`, error);
             }
         }
     }
 }

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

Suggested change

	if (worktree.taskId) {
	try {
	await this.removeWorktree(worktree.taskId, force);
	} catch (error) {
	console.error(`Failed to cleanup worktree for task ${worktree.taskId}:`, error);
	}
	}
	// Extract taskId from the path if not available
	let taskId = worktree.taskId;
	if (!taskId || taskId === 'unknown') {
	const match = worktree.path.match(/task-(.+?)$/);
	taskId = match?.[1];
	}
	if (taskId && taskId !== 'unknown') {
	try {
	// Check if this worktree is in our active map
	if (this.activeWorktrees.has(taskId)) {
	await this.removeWorktree(taskId, force);
	} else {
	// Directly remove via git command for orphaned worktrees
	const args = ['worktree', 'remove', worktree.path];
	if (force) args.push('--force');
	await this.executeGitCommand(args, { cwd: this.config.projectRoot });
	}
	} catch (error) {
	console.error(`Failed to cleanup worktree for task ${taskId}:`, error);
	}
	}

🤖 Prompt for AI Agents

packages/workflow-engine/src/worktree/worktree-manager.ts around lines 253 to
259: cleanupAll calls removeWorktree with worktree.taskId but listWorktrees
returns objects whose taskId is always 'unknown' because completeWorktreeInfo
falls back to 'unknown' when branch parsing fails; change completeWorktreeInfo
(around line 331) to derive and assign taskId from the branch name
deterministically (parse branch name and set partial.taskId) and stop forcing a
literal 'unknown' fallback — instead return undefined/null when parsing fails so
callers can skip or re-derive the id; update listWorktrees to populate taskId
from the parsed branch (or skip entries without a valid taskId) so
removeWorktree gets a real taskId in cleanupAll.

[coderabbitai]

🧹 Nitpick (assertive)

Complex taskId extraction logic could be simplified.

The completeWorktreeInfo method has complex logic for extracting taskId from branch names that could be made more robust.

Consider using a more explicit approach:

 private completeWorktreeInfo(partial: Partial<WorktreeInfo>): WorktreeInfo {
     const branch = partial.branch || 'unknown';
-    const taskIdMatch = branch.match(/^task\/(.+?)-/);
+    
+    // Try multiple patterns to extract taskId
+    let taskId: string | undefined;
+    
+    // First, check if it's already in activeWorktrees
+    for (const [id, info] of this.activeWorktrees) {
+        if (info.path === partial.path) {
+            taskId = id;
+            break;
+        }
+    }
+    
+    // If not found, extract from branch name
+    if (!taskId) {
+        const taskIdMatch = branch.match(/^task\/(.+?)-\d+$/);
+        taskId = taskIdMatch?.[1];
+    }
+    
+    // Finally, try to extract from path
+    if (!taskId && partial.path) {
+        const pathMatch = partial.path.match(/task-(.+?)$/);
+        taskId = pathMatch?.[1];
+    }
     
     return {
         path: partial.path || '',
         branch,
         createdAt: partial.createdAt || new Date(),
-        taskId: taskIdMatch?.[1] || partial.taskId || 'unknown',
+        taskId: taskId || 'unknown',
         commit: partial.commit,
         locked: partial.locked || false,
         lockReason: partial.lockReason
     };
 }

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

Suggested change

	private completeWorktreeInfo(partial: Partial<WorktreeInfo>): WorktreeInfo {
	const branch = partial.branch || 'unknown';
	const taskIdMatch = branch.match(/^task\/(.+?)-/);
	return {
	path: partial.path || '',
	branch,
	createdAt: partial.createdAt || new Date(),
	taskId: taskIdMatch?.[1] || partial.taskId || 'unknown',
	commit: partial.commit,
	locked: partial.locked || false,
	lockReason: partial.lockReason
	};
	}
	private completeWorktreeInfo(partial: Partial<WorktreeInfo>): WorktreeInfo {
	const branch = partial.branch || 'unknown';
	// Try multiple patterns to extract taskId
	let taskId: string | undefined;
	// First, check if it's already in activeWorktrees
	for (const [id, info] of this.activeWorktrees) {
	if (info.path === partial.path) {
	taskId = id;
	break;
	}
	}
	// If not found, extract from branch name
	if (!taskId) {
	const taskIdMatch = branch.match(/^task\/(.+?)-\d+$/);
	taskId = taskIdMatch?.[1];
	}
	// Finally, try to extract from path
	if (!taskId && partial.path) {
	const pathMatch = partial.path.match(/task-(.+?)$/);
	taskId = pathMatch?.[1];
	}
	return {
	path: partial.path || '',
	branch,
	createdAt: partial.createdAt || new Date(),
	taskId: taskId || 'unknown',
	commit: partial.commit,
	locked: partial.locked || false,
	lockReason: partial.lockReason
	};
	}

🤖 Prompt for AI Agents

In packages/workflow-engine/src/worktree/worktree-manager.ts around lines 323 to
336, the taskId extraction from the branch string is fragile and overly complex;
update the logic to first prefer partial.taskId when present, otherwise
normalize the branch (default to 'unknown') and use a clearer regex like
/^task\/([^\/-]+)/ to capture the task id (so it works for "task/ID",
"task/ID-..." and avoids empty matches), then fall back to 'unknown'; keep the
rest of the completed WorktreeInfo properties as-is and ensure locked defaults
to false.

[coderabbitai]

⚠️ Potential issue

Potential race condition in cleanup handlers.

Multiple cleanup handlers are registered for the same events, and the cleanup is called synchronously in the exit handler which could block the process from exiting properly.

The cleanup should be more defensive:

 private setupCleanupHandlers(): void {
+    let cleanupInProgress = false;
     const cleanup = () => {
+        if (cleanupInProgress) return;
+        cleanupInProgress = true;
         console.log('Cleaning up worktrees...');
-        this.cleanupAll(true).catch(console.error);
+        // Don't await in exit handler to avoid blocking
+        this.cleanupAll(true)
+            .catch(console.error)
+            .finally(() => { cleanupInProgress = false; });
     };
 
-    process.on('SIGINT', cleanup);
-    process.on('SIGTERM', cleanup);
-    process.on('exit', cleanup);
+    process.once('SIGINT', cleanup);
+    process.once('SIGTERM', cleanup);
+    process.once('exit', cleanup);
 }

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

Suggested change

	const cleanup = () => {
	console.log('Cleaning up worktrees...');
	this.cleanupAll(true).catch(console.error);
	};
	process.on('SIGINT', cleanup);
	process.on('SIGTERM', cleanup);
	process.on('exit', cleanup);
	}
	private setupCleanupHandlers(): void {
	let cleanupInProgress = false;
	const cleanup = () => {
	if (cleanupInProgress) return;
	cleanupInProgress = true;
	console.log('Cleaning up worktrees...');
	// Don't await in exit handler to avoid blocking
	this.cleanupAll(true)
	.catch(console.error)
	.finally(() => { cleanupInProgress = false; });
	};
	process.once('SIGINT', cleanup);
	process.once('SIGTERM', cleanup);
	process.once('exit', cleanup);
	}

🤖 Prompt for AI Agents

In packages/workflow-engine/src/worktree/worktree-manager.ts around lines 342 to
350, multiple handlers are adding non-idempotent cleanup calls and the exit
handler invokes async cleanup synchronously which can block exit; change
handlers to register once (process.once) not on, add a boolean "cleanupCalled"
guard so cleanup runs only once, make cleanup return a Promise and avoid
awaiting it inside the 'exit' handler (instead trigger it asynchronously via
setImmediate or process.nextTick and catch/log errors), and ensure any
long-running async work is awaited only in SIGINT/SIGTERM handlers while the
'exit' handler performs a minimal synchronous-safe teardown or just kicks off
the async cleanup without blocking process termination.

[coderabbitai]

Review continued from previous batch...

[coderabbitai]

⚠️ Potential issue

Expired/remaining time calculation is wrong near the hour boundary.

If <1h remains, hoursRemaining is 0 and you print “expired”. Compare timestamps instead and show a friendly delta.

-      if (hoursRemaining > 0) {
-        console.log(
-          chalk.gray(
-            `  Expires: ${expiresAt.toLocaleString()} (${hoursRemaining} hours remaining)`
-          )
-        );
-      } else {
-        console.log(
-          chalk.yellow(`  Token expired at: ${expiresAt.toLocaleString()}`)
-        );
-      }
+      const msRemaining = expiresAt.getTime() - now.getTime();
+      if (msRemaining > 0) {
+        const mins = Math.round(msRemaining / 60000);
+        const human = mins >= 60 ? `${Math.floor(mins / 60)}h ${mins % 60}m` : `${mins}m`;
+        console.log(chalk.gray(`  Expires: ${expiresAt.toLocaleString()} (${human} remaining)`));
+      } else {
+        console.log(chalk.yellow(`  Token expired at: ${expiresAt.toLocaleString()}`));
+      }

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

Suggested change

	if (credentials.expiresAt) {
	const expiresAt = new Date(credentials.expiresAt);
	const now = new Date();
	const hoursRemaining = Math.floor(
	(expiresAt.getTime() - now.getTime()) / (1000 * 60 * 60)
	);
	if (hoursRemaining > 0) {
	console.log(
	chalk.gray(
	` Expires: ${expiresAt.toLocaleString()} (${hoursRemaining} hours remaining)`
	)
	);
	} else {
	console.log(
	chalk.yellow(` Token expired at: ${expiresAt.toLocaleString()}`)
	);
	}
	if (credentials.expiresAt) {
	const expiresAt = new Date(credentials.expiresAt);
	const now = new Date();
	const msRemaining = expiresAt.getTime() - now.getTime();
	if (msRemaining > 0) {
	const mins = Math.round(msRemaining / 60000);
	const human = mins >= 60 ? `${Math.floor(mins / 60)}h ${mins % 60}m` : `${mins}m`;
	console.log(
	chalk.gray(
	` Expires: ${expiresAt.toLocaleString()} (${human} remaining)`
	)
	);
	} else {
	console.log(
	chalk.yellow(` Token expired at: ${expiresAt.toLocaleString()}`)
	);
	}

🤖 Prompt for AI Agents

In apps/cli/src/commands/auth.command.ts around lines 187-204, the code uses
Math.floor on hoursRemaining which yields 0 when under 1 hour and incorrectly
prints "expired"; change the check to compare timestamps (expiresAt.getTime() >
now.getTime()) and compute the millisecond delta, then format a friendly
remaining string (e.g., hours and minutes, or "< 1 hour" / "X minutes") to
display when delta > 0, otherwise print the expired message; ensure you avoid
using floor for the main existence check and derive human-readable remaining
time from the positive delta.

[coderabbitai]

🧹 Nitpick (assertive)

Optional: align with repo preference to avoid node: scheme for core modules.

Past preference indicates not using node: imports. Switch to bare 'path' if you want consistency.

-import path from 'node:path';
+import path from 'path';

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

Suggested change

	import path from 'node:path';
	import path from 'path';

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-list.command.ts around line 8, the
import uses the 'node:' scheme (import path from 'node:path') which the repo
prefers to avoid; change the import to the bare core module form by replacing
'node:path' with 'path' so the file imports path from 'path' to align with
repository style.

[coderabbitai]

⚠️ Potential issue

Fix invalid Chalk color usage (chalk.orange is undefined).

Chalk has no orange convenience method. Use keyword/hex or a supported color.

-      paused: { icon: '⏸️', color: chalk.orange },
+      paused: { icon: '⏸️', color: chalk.keyword('orange') },

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

Suggested change

	paused: { icon: '⏸️', color: chalk.orange },
	completed: { icon: '✅', color: chalk.green },
	failed: { icon: '❌', color: chalk.red },
	paused: { icon: '⏸️', color: chalk.keyword('orange') },
	completed: { icon: '✅', color: chalk.green },
	failed: { icon: '❌', color: chalk.red },

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-list.command.ts around lines 207 to
209, the code uses chalk.orange which is not a valid Chalk method; replace it
with a supported color call such as chalk.keyword('orange') or
chalk.hex('#FFA500') (e.g., paused: { icon: '⏸️', color: chalk.keyword('orange')
}) to fix the invalid color usage.

[coderabbitai]

🛠️ Refactor suggestion

Make duration formatting resilient to string dates.

Workflow timestamps may be de/serialized; guard against strings to avoid runtime errors.

-  private formatDuration(start: Date, end: Date): string {
-    const diff = end.getTime() - start.getTime();
+  private formatDuration(start: Date | string, end: Date | string): string {
+    const startMs = new Date(start as any).getTime();
+    const endMs = new Date(end as any).getTime();
+    const diff = endMs - startMs;

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

Suggested change

	private formatDuration(start: Date, end: Date): string {
	const diff = end.getTime() - start.getTime();
	const minutes = Math.floor(diff / (1000 * 60));
	const hours = Math.floor(minutes / 60);
	if (hours > 0) {
	return `${hours}h ${minutes % 60}m`;
	} else if (minutes > 0) {
	return `${minutes}m`;
	} else {
	return '<1m';
	}
	}
	private formatDuration(start: Date | string, end: Date | string): string {
	const startMs = new Date(start as any).getTime();
	const endMs = new Date(end as any).getTime();
	const diff = endMs - startMs;
	const minutes = Math.floor(diff / (1000 * 60));
	const hours = Math.floor(minutes / 60);
	if (hours > 0) {
	return `${hours}h ${minutes % 60}m`;
	} else if (minutes > 0) {
	return `${minutes}m`;
	} else {
	return '<1m';
	}
	}

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-list.command.ts around lines 228 to
240, the formatDuration function assumes start and end are Date objects and will
throw if passed strings; coerce inputs to Date instances (e.g., if typeof start
=== 'string' or typeof end === 'string', wrap with new Date(...)), validate with
isNaN(date.getTime()) and return a clear fallback like '<invalid date>' if
either is invalid, and ensure you compute the difference only after
coercion/validation so string timestamps won't cause runtime errors.

[coderabbitai]

🧹 Nitpick (assertive)

Status icons/text diverge from existing scripts and miss completed.

Use the checkmark ✓ (U+2713) instead of √ and include completed mapping for back-compat with scripts/modules/ui.js.

-    done: {
-      color: chalk.green,
-      icon: String.fromCharCode(8730),
-      tableIcon: String.fromCharCode(8730)
-    }, // √
+    done: { color: chalk.green, icon: '✓', tableIcon: '✓' },
+    completed: { color: chalk.green, icon: '✓', tableIcon: '✓' },
@@
-  const simpleIcons = {
-    done: String.fromCharCode(8730), // √
+  const simpleIcons = {
+    done: '✓',
+    completed: '✓',

Also applies to: 42-55

🤖 Prompt for AI Agents

In apps/cli/src/utils/ui.ts around lines 18-35 (and also apply same change to
42-55), the status mapping uses the √ character (U+221A) for "done" and lacks a
"completed" key required for backward compatibility; update the "done" entry to
use the checkmark ✓ (U+2713) for both icon and tableIcon, and add a new
"completed" mapping that mirrors the "done" mapping (same color, icon, and
tableIcon) so existing scripts/modules/ui.js continue to work.

[coderabbitai]

⚠️ Potential issue

Fix error handling pattern in catch block.

Same error type issue.

Apply this diff:

-		} catch (error: any) {
-			if (error.code === 'ENOENT') {
+		} catch (error) {
+			if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
 				throw new Error(`Tag ${tag} not found - file doesn't exist`);
 			}
 			throw error;

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

Suggested change

	} catch (error: any) {
	if (error.code === 'ENOENT') {
	throw new Error(`Tag ${tag} not found - file doesn't exist`);
	}
	throw error;
	}
	} catch (error) {
	if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
	throw new Error(`Tag ${tag} not found - file doesn't exist`);
	}
	throw error;
	}

🤖 Prompt for AI Agents

In packages/tm-core/src/storage/file-storage/file-storage.ts around lines 313 to
318, the catch block replaces the original ENOENT error with a plain Error which
loses the original error type and properties; instead preserve the original
error object (and its code) while updating its message and rethrowing it — e.g.,
for ENOENT assign the new descriptive message onto the caught error (or
Object.assign a copy) and then throw the original/augmented error so the
error.type/code remains intact.

[coderabbitai]

⚠️ Potential issue

Fix error handling pattern.

Same error type issue.

Apply this diff:

-		} catch (error: any) {
-			if (error.code === 'ENOENT') {
+		} catch (error) {
+			if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
 				throw new Error(`Tag ${oldTag} not found - file doesn't exist`);
 			}
 			throw error;

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

Suggested change

	} catch (error: any) {
	if (error.code === 'ENOENT') {
	throw new Error(`Tag ${oldTag} not found - file doesn't exist`);
	}
	throw error;
	}
	} catch (error) {
	if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
	throw new Error(`Tag ${oldTag} not found - file doesn't exist`);
	}
	throw error;
	}

🤖 Prompt for AI Agents

In packages/tm-core/src/storage/file-storage/file-storage.ts around lines
361-366, the catch currently throws a new Error for ENOENT which loses the
original error type and code; instead, update the handler to preserve the
original error: if error && (error as any).code === 'ENOENT', set (error as
Error).message = `Tag ${oldTag} not found - file doesn't exist` and rethrow the
original error; if the caught value isn't an Error object, create a new Error,
set its code property to 'ENOENT' and throw that.

[coderabbitai]

⚠️ Potential issue

Fix error handling: use proper type for file system errors.

Similar to other files, the error handling pattern uses error.code which doesn't exist on the base Error type.

Apply this diff:

-    } catch (error: any) {
-      if (error.code === 'ENOENT') {
+    } catch (error) {
+      if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
         // Workflows file doesn't exist, start with empty state
         console.debug('No workflows.json found, starting with empty state');
         return;
       }
 
-      console.warn('Failed to load workflow state:', error.message);
+      console.warn('Failed to load workflow state:', (error as Error).message);

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

Suggested change

	} catch (error: any) {
	if (error.code === 'ENOENT') {
	// Workflows file doesn't exist, start with empty state
	console.debug('No workflows.json found, starting with empty state');
	return;
	}
	console.warn('Failed to load workflow state:', error.message);
	}
	} catch (error) {
	if ((error as NodeJS.ErrnoException).code === 'ENOENT') {
	// Workflows file doesn't exist, start with empty state
	console.debug('No workflows.json found, starting with empty state');
	return;
	}
	console.warn('Failed to load workflow state:', (error as Error).message);
	}

[coderabbitai]

⚠️ Potential issue

Fix error handling pattern in clearState.

Same error type issue.

Apply this diff:

-    } catch (error: any) {
-      if (error.code !== 'ENOENT') {
+    } catch (error) {
+      if ((error as NodeJS.ErrnoException).code !== 'ENOENT') {
         throw error;
       }

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

Suggested change

	await fs.unlink(this.stateFilePath);
	} catch (error: any) {
	if (error.code !== 'ENOENT') {
	throw error;
	}
	}
	await fs.unlink(this.stateFilePath);
	} catch (error) {
	if ((error as NodeJS.ErrnoException).code !== 'ENOENT') {
	throw error;
	}
	}

🤖 Prompt for AI Agents

In packages/workflow-engine/src/state/workflow-state-manager.ts around lines 279
to 284, the catch block uses a loosely typed error and accesses error.code
directly; change the handling to narrow the error type and safely check the
errno code before swallowing it. Replace the current catch with one that casts
error to NodeJS.ErrnoException (or checks for a code property via (error as
NodeJS.ErrnoException)?.code) and only suppress when code === 'ENOENT';
otherwise rethrow the original error.

[coderabbitai]

🧹 Nitpick (assertive)

Use proper environment variable check.

Checking environment variables directly can be inconsistent. Consider checking for truthy values.

-    if (process.env.TASKMASTER_DEBUG) {
+    if (process.env.TASKMASTER_DEBUG === 'true' || process.env.TASKMASTER_DEBUG === '1') {

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

Suggested change

	if (process.env.TASKMASTER_DEBUG) {
	if (process.env.TASKMASTER_DEBUG === 'true' || process.env.TASKMASTER_DEBUG === '1') {

🤖 Prompt for AI Agents

In packages/workflow-engine/src/state/workflow-state-manager.ts around line 301,
the code checks process.env.TASKMASTER_DEBUG directly which can be inconsistent;
change the condition to reliably detect truthy values (e.g., normalize and
compare to 'true' or coerce with a boolean) such as using
!!process.env.TASKMASTER_DEBUG or process.env.TASKMASTER_DEBUG?.toLowerCase()
=== 'true' so the debug flag behaves consistently across environments.

[coderabbitai]

Review continued from previous batch...

[coderabbitai]

⚠️ Potential issue

Fix incorrect TypeScript typing.

Using any in the catch block bypasses TypeScript's type checking. Use unknown instead for better type safety.

Apply this diff:

-    } catch (error: any) {
+    } catch (error) {
       ui.displayError(error.message || 'Failed to get workflow status');

Or if you need to access the message property:

-    } catch (error: any) {
+    } catch (error) {
+      const message = error instanceof Error ? error.message : 'Failed to get workflow status';
+      ui.displayError(message);

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

Suggested change

	} catch (error: any) {
	} catch (error) {
	const message = error instanceof Error ? error.message : 'Failed to get workflow status';
	ui.displayError(message);
	}

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-status.command.ts around line 55, the
catch block types the error as `any`; change it to `unknown` to preserve type
safety and avoid bypassing the checker. Update the catch signature to `catch
(error: unknown)` and, if you need to read `error.message`, narrow the type
first (e.g., `if (error instanceof Error) { /* use error.message */ }` or
extract a safe message via `const msg = error && typeof (error as any).message
=== 'string' ? (error as any).message : String(error);`). Ensure subsequent
error handling uses the narrowed type or the safe message variable.

[coderabbitai]

⚠️ Potential issue

Process event handler may cause memory leak.

The SIGINT handler is added every time watchWorkflowStatus is called but never removed, which could lead to memory leaks if the method is called multiple times.

Store and clean up the event handler properly:

+  // Store handler reference for cleanup
+  const sigintHandler = () => {
+    clearInterval(interval);
+    console.log(chalk.yellow('\n👋 Stopped watching workflow status'));
+    process.exit(0);
+  };
+
   // Handle Ctrl+C
-  process.on('SIGINT', () => {
-    clearInterval(interval);
-    console.log(chalk.yellow('\n👋 Stopped watching workflow status'));
-    process.exit(0);
-  });
+  process.once('SIGINT', sigintHandler);
+
+  // Clean up handler when watch ends naturally
+  // (add this in the workflow end check logic)

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

Suggested change

	// Handle Ctrl+C
	process.on('SIGINT', () => {
	clearInterval(interval);
	console.log(chalk.yellow('\n👋 Stopped watching workflow status'));
	process.exit(0);
	});
	// Store handler reference for cleanup
	const sigintHandler = () => {
	clearInterval(interval);
	console.log(chalk.yellow('\n👋 Stopped watching workflow status'));
	process.exit(0);
	};
	// Handle Ctrl+C
	process.once('SIGINT', sigintHandler);
	// Clean up handler when watch ends naturally
	// (add this in the workflow end check logic)

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-status.command.ts around lines 149 to
154, the SIGINT handler is registered on every call which can leak memory; make
the handler a named function, register it once per invocation, and remove it in
the cleanup path: create a const onSigint = () => { clearInterval(interval);
console.log(chalk.yellow('\n👋 Stopped watching workflow status'));
process.exit(0); }, register with process.on('SIGINT', onSigint) and ensure you
call process.off('SIGINT', onSigint) (and clearInterval) when stopping the
watcher or before re-registering to avoid duplicate listeners; alternatively use
process.once('SIGINT', onSigint) if one-time handling is acceptable.

[coderabbitai]

🛠️ Refactor suggestion

Replace infinite promise with proper event handling.

Using an infinite promise to keep the process alive is an anti-pattern that makes the code harder to test and reason about.

Replace with a proper event-based approach:

-    // Keep the process alive
-    await new Promise(() => {});
+    // Return a promise that resolves when watching ends
+    return new Promise<void>((resolve) => {
+      // Resolve when workflow ends
+      const checkEnd = () => {
+        this.workflowManager!.getWorkflowStatus(workflowId);
+        // ... existing workflow status check logic
+        if (['completed', 'failed', 'cancelled', 'timeout'].includes(workflow.status)) {
+          clearInterval(interval);
+          resolve();
+        }
+      };
+      
+      // Also resolve on SIGINT
+      process.once('SIGINT', () => {
+        clearInterval(interval);
+        console.log(chalk.yellow('\n👋 Stopped watching workflow status'));
+        resolve();
+      });
+    });

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-status.command.ts around line 157,
remove the infinite await new Promise(() => {}) and replace it with proper
event-driven shutdown handling: create a promise that resolves when the process
should exit (e.g., on 'SIGINT' and 'SIGTERM' signals or an AbortSignal/event
from the workflow service) and await that instead, wiring up listeners that
cleanly close any open resources and remove listeners after resolution; ensure
the promise rejects/handles errors from the workflow client and that you call
process.exit only after cleanup if needed.

[coderabbitai]

⚠️ Potential issue

Typo in color name 'orange' - should use 'yellow'.

The chalk library doesn't have an 'orange' color. This will cause a runtime error.

Apply this diff to fix the color name:

-      paused: { icon: '⏸️', color: chalk.orange },
+      paused: { icon: '⏸️', color: chalk.yellow },

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

Suggested change

	paused: { icon: '⏸️', color: chalk.orange },
	paused: { icon: '⏸️', color: chalk.yellow },

🤖 Prompt for AI Agents

In apps/cli/src/commands/workflow/workflow-status.command.ts around line 298 the
code uses chalk.orange which is invalid and causes a runtime error; change the
color reference to chalk.yellow (replace chalk.orange with chalk.yellow) so the
paused state uses a supported chalk color.

[coderabbitai]

🧹 Nitpick (assertive)

Type safety concern with 'any' type

The setNestedProperty method uses any type which bypasses TypeScript's type checking. This could lead to runtime errors if the wrong types are set.

Consider using a more type-safe approach:

 private setNestedProperty(
-  obj: any,
+  obj: Record<string, any>,
   path: readonly string[],
-  value: any
+  value: unknown
 ): void {

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

Suggested change

	private setNestedProperty(
	obj: any,
	path: readonly string[],
	value: any
	): void {
	const lastKey = path[path.length - 1];
	const keys = path.slice(0, -1);
	let current = obj;
	for (const key of keys) {
	if (!current[key]) {
	current[key] = {};
	}
	current = current[key];
	}
	current[lastKey] = value;
	}
	private setNestedProperty(
	obj: Record<string, any>,
	path: readonly string[],
	value: unknown
	): void {
	const lastKey = path[path.length - 1];
	const keys = path.slice(0, -1);
	let current = obj;
	for (const key of keys) {
	if (!current[key]) {
	current[key] = {};
	}
	current = current[key];
	}
	current[lastKey] = value;
	}

🤖 Prompt for AI Agents

In packages/tm-core/src/config/services/environment-config-provider.service.ts
around lines 111 to 128, the setNestedProperty method currently uses the unsafe
any type for obj and value; change the signature to be generic and type-safe
(e.g. <T, V> setNestedProperty(obj: T, path: readonly string[], value: V):
asserts obj is T & Record<string, any> or return T) and replace uses of any with
Record<string, unknown> or unknown, add narrow type guards when creating nested
objects (cast to Record<string, unknown> only after checking typeof === "object"
&& obj !== null), and ensure the final assignment casts appropriately (or
returns a new typed object) so TypeScript can enforce structure while preserving
runtime behavior.

[coderabbitai]

🧹 Nitpick (assertive)

Potential memory leak in exit handler

The exit handler cleans up maps but doesn't remove the event listeners attached to stdout/stderr streams.

Consider removing all listeners when the process exits:

 // Handle process exit
 childProcess.on('exit', (code, signal) => {
   process.status = code === 0 ? 'stopped' : 'crashed';
   
   this.emitEvent('process.stopped', workflowId, taskId, {
     pid: process.pid,
     exitCode: code,
     signal
   });
   
+  // Remove all listeners to prevent memory leaks
+  childProcess.stdout?.removeAllListeners();
+  childProcess.stderr?.removeAllListeners();
+  childProcess.removeAllListeners();
+  
   // Cleanup
   this.activeProcesses.delete(workflowId);
   this.childProcesses.delete(workflowId);

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

Suggested change

	childProcess.on('exit', (code, signal) => {
	process.status = code === 0 ? 'stopped' : 'crashed';
	this.emitEvent('process.stopped', workflowId, taskId, {
	pid: process.pid,
	exitCode: code,
	signal
	});
	// Cleanup
	this.activeProcesses.delete(workflowId);
	this.childProcesses.delete(workflowId);
	const timeout = this.timeouts.get(workflowId);
	if (timeout) {
	clearTimeout(timeout);
	this.timeouts.delete(workflowId);
	}
	});
	childProcess.on('exit', (code, signal) => {
	process.status = code === 0 ? 'stopped' : 'crashed';
	this.emitEvent('process.stopped', workflowId, taskId, {
	pid: process.pid,
	exitCode: code,
	signal
	});
	// Remove all listeners to prevent memory leaks
	childProcess.stdout?.removeAllListeners();
	childProcess.stderr?.removeAllListeners();
	childProcess.removeAllListeners();
	// Cleanup
	this.activeProcesses.delete(workflowId);
	this.childProcesses.delete(workflowId);
	const timeout = this.timeouts.get(workflowId);
	if (timeout) {
	clearTimeout(timeout);
	this.timeouts.delete(workflowId);
	}
	});

🤖 Prompt for AI Agents

In packages/workflow-engine/src/process/process-sandbox.ts around lines 287 to
305, the exit handler cleans up maps but leaves stdout/stderr and other event
listeners attached to the child process, risking a memory leak; update the
handler to remove listeners by checking for childProcess.stdout and
childProcess.stderr and calling removeAllListeners() (or off() for specific
handlers) on them, and also call childProcess.removeAllListeners() to clear any
remaining process-level listeners before deleting maps/timeouts so all
references are released.

[coderabbitai]

🛠️ Refactor suggestion

Multiple cleanup handler registrations could cause issues

The cleanup handlers are registered in the constructor, which means if multiple ProcessSandbox instances are created, multiple handlers will be registered on the process object.

Use a singleton pattern or track registration state:

 export class ProcessSandbox extends EventEmitter {
   private config: ProcessSandboxConfig;
   private activeProcesses = new Map<string, WorkflowProcess>();
   private childProcesses = new Map<string, ChildProcess>();
   private timeouts = new Map<string, NodeJS.Timeout>();
+  private static cleanupHandlersRegistered = false;
   
   private setupCleanupHandlers(): void {
+    // Only register once globally
+    if (ProcessSandbox.cleanupHandlersRegistered) {
+      return;
+    }
+    ProcessSandbox.cleanupHandlersRegistered = true;
+    
     const cleanup = () => {
       console.log('Cleaning up processes...');
       this.cleanupAll(true).catch(console.error);
     };

🤖 Prompt for AI Agents

packages/workflow-engine/src/process/process-sandbox.ts around lines 368 to 377:
the current setupCleanupHandlers registers process-level SIGINT/SIGTERM/exit
handlers per instance which leads to duplicate handlers if multiple
ProcessSandbox instances are created; change to register handlers only once by
adding a static/class-level boolean flag (e.g., handlersRegistered) checked
before registering, set it to true after registration, and ensure the cleanup
callback can call instance cleanupAll for all sandboxes (either by delegating to
a static manager that tracks instances or making cleanup call a static
cleanupAll that iterates instances); alternatively, remove listeners when an
instance is disposed by calling process.off for the bound handlers to avoid
accumulation.

[coderabbitai]

🧹 Nitpick (assertive)

Remove unused imports; align with repo import style.

path and WorkflowTimeoutError are unused. Also, team prefers avoiding node: scheme imports.

-import path from 'node:path';
@@
-  MaxConcurrentWorkflowsError,
-  WorkflowTimeoutError
+  MaxConcurrentWorkflowsError

Optionally (style):

-import { EventEmitter } from 'node:events';
+import { EventEmitter } from 'events';

Also applies to: 31-32

🤖 Prompt for AI Agents

In packages/workflow-engine/src/task-execution/task-execution-manager.ts around
lines 7 and also lines 31-32, remove the unused imports: drop the `path` import
and the `WorkflowTimeoutError` import, and change any remaining `node:`-prefixed
imports to the repo preferred non-`node:` style (e.g., use 'path' instead of
'node:path' where a path import is actually needed elsewhere); ensure import
ordering and linting rules remain satisfied after removal.

[coderabbitai]

🛠️ Refactor suggestion

Guard against concurrent starts for the same task (race condition).

Two overlapping startTaskExecution calls for the same task can pass hasActiveWorkflow() and both proceed, creating duplicate workflows/worktrees. Add a per-task in-flight guard.

 export class TaskExecutionManager extends EventEmitter {
   private config: TaskExecutionManagerConfig;
   private worktreeManager: WorktreeManager;
   private processSandbox: ProcessSandbox;
   private stateManager: WorkflowStateManager;
   private initialized = false;
+  private startingTasks = new Set<string>();
@@
-    try {
+    if (this.startingTasks.has(task.id)) {
+      throw new WorkflowError(
+        `Task ${task.id} already has an active workflow`,
+        'TASK_ALREADY_EXECUTING',
+        undefined,
+        task.id
+      );
+    }
+    this.startingTasks.add(task.id);
+    try {
       // Create worktree
@@
-    } catch (error) {
+    } catch (error) {
       throw new WorkflowError(
         `Failed to start task execution for ${task.id}`,
         'TASK_EXECUTION_START_ERROR',
         undefined,
         task.id,
         error as Error
       );
-    }
+    } finally {
+      this.startingTasks.delete(task.id);
+    }

Also applies to: 117-199

🤖 Prompt for AI Agents

In packages/workflow-engine/src/task-execution/task-execution-manager.ts around
lines 44-49 (and similarly for the logic between lines 117-199), there is a race
where two concurrent startTaskExecution calls can both observe no active
workflow and create duplicate workflows/worktrees; add a per-task in-flight
guard (e.g., a Map<string, Promise|boolean>) keyed by taskId that is checked at
the very start of startTaskExecution and immediately set when beginning entry
into the start path; if an entry exists, short-circuit (throw or return a
controlled error/result); ensure the guard is cleared in all completion and
error paths (finally) so subsequent starts can proceed; apply this same guard
check to the other start-related flow in lines 117-199 to prevent duplicate
concurrent starts for the same task.

[coderabbitai]

⚠️ Potential issue

Stale-workflow cleanup uses the wrong identifier (guaranteed miss).

cleanupStaleWorkflows() checks and updates using workflow-${workflow.taskId}, but processes and state are keyed by the real workflowId returned at registration (e.g., workflow-<taskId>-<timestamp>-<rand>). This will never match, so stale workflows aren’t cleaned.

Apply this fix (requires exposing IDs from state):

-    const workflows = this.stateManager.listWorkflows();
-
-    for (const workflow of workflows) {
-      const isStale =
-        workflow.status === 'running' &&
-        !this.processSandbox.isProcessRunning(`workflow-${workflow.taskId}`);
-
-      if (isStale) {
-        console.log(`Cleaning up stale workflow for task ${workflow.taskId}`);
-
-        try {
-          await this.stateManager.updateWorkflowStatus(
-            `workflow-${workflow.taskId}`,
-            'failed'
-          );
-
-          // Try to clean up worktree
-          await this.worktreeManager.removeWorktree(workflow.taskId, true);
-        } catch (error) {
-          console.error(`Failed to cleanup stale workflow:`, error);
-        }
-      }
-    }
+    const entries = this.stateManager.listWorkflowsWithIds();
+    for (const { id, context } of entries) {
+      const isStale =
+        context.status === 'running' &&
+        !this.processSandbox.isProcessRunning(id);
+      if (isStale) {
+        console.log(`Cleaning up stale workflow for task ${context.taskId}`);
+        try {
+          await this.stateManager.updateWorkflowStatus(id, 'failed');
+          await this.worktreeManager.removeWorktree(context.taskId, true);
+        } catch (error) {
+          console.error(`Failed to cleanup stale workflow:`, error);
+        }
+      }
+    }

Add this helper to WorkflowStateManager (outside this file) to expose IDs:

// packages/workflow-engine/src/state/workflow-state-manager.ts
listWorkflowsWithIds(): Array<{ id: string; context: WorkflowExecutionContext }> {
  return Array.from(this.activeWorkflows.entries()).map(([id, context]) => ({ id, context }));
}

[coderabbitai]

Review continued from previous batch...

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Unify naming and remove duplicate field.

• Rename atomicOperations → atomicWrites to match StorageConfig.
• basePath is already included via extends Omit<RuntimeStorageConfig, 'apiConfigured'>; redeclaration is redundant.

Apply this diff:

 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 */
   enableBackup: boolean;
   /** Maximum number of backups to retain */
   maxBackups: number;
   /** Enable compression for storage */
   enableCompression: boolean;
   /** File encoding for text files */
   encoding: BufferEncoding;
-  /** Enable atomic file operations */
-  atomicOperations: boolean;
+  /** Enable atomic file writes */
+  atomicWrites: boolean;
 }

---

🏁 Script executed:

#!/bin/bash
# Find all references to the old name to update
rg -n "atomicOperations" -C2 --type ts

Length of output: 1379

---

🏁 Script executed:

#!/bin/bash
set -eo pipefail

echo "Searching for RuntimeStorageConfig occurrences..."
rg -n -S --type ts "RuntimeStorageConfig" -g '!**/node_modules/**' -C3 || true

echo
echo "Searching specifically for interface/type declaration of RuntimeStorageConfig..."
rg -n -S --type ts "interface RuntimeStorageConfig\b|type RuntimeStorageConfig\b" -g '!**/node_modules/**' -C3 || true

FILES=$(rg -n --type ts "interface RuntimeStorageConfig\b|type RuntimeStorageConfig\b" -g '!**/node_modules/**' -l || true)
if [ -n "$FILES" ]; then
  for f in $FILES; do
    echo
    echo "----- Showing $f (first 300 lines) -----"
    sed -n '1,300p' "$f" || true
  done
fi

echo
echo "Searching for basePath occurrences..."
rg -n -S --type ts "\bbasePath\b" -g '!**/node_modules/**' -C3 || true

echo
echo "Searching for atomicWrites occurrences..."
rg -n -S --type ts "\batomicWrites\b" -g '!**/node_modules/**' -C3 || true

Length of output: 958

---

🏁 Script executed:

#!/bin/bash
set -eo pipefail

echo "pwd: $(pwd)"
echo "Top-level repo entries:"
ls -la | sed -n '1,200p'

echo
echo "Search for any declaration or usage of 'RuntimeStorageConfig' (no type filters):"
rg -n "RuntimeStorageConfig" -S -C3 || true

echo
echo "Search for exact interface/type declaration of RuntimeStorageConfig:"
rg -n --multiline "interface\s+RuntimeStorageConfig\b|type\s+RuntimeStorageConfig\b" -S -C3 || true

echo
echo "Print packages/tm-core/src/interfaces/configuration.interface.ts to confirm StorageSettings content:"
sed -n '1,260p' packages/tm-core/src/interfaces/configuration.interface.ts || true

echo
echo "Print config loader files that referenced atomicOperations earlier:"
sed -n '1,260p' packages/tm-core/src/config/services/config-loader.service.ts || true
echo "---- spec ----"
sed -n '1,260p' packages/tm-core/src/config/services/config-loader.service.spec.ts || true

echo
echo "Search for 'basePath' occurrences repository-wide:"
rg -n "\bbasePath\b" -S -C3 || true

echo
echo "Search for 'atomicWrites' occurrences repository-wide:"
rg -n "\batomicWrites\b" -S -C3 || true

Length of output: 45441

---

Rename atomicOperations → atomicWrites and remove duplicate basePath

RuntimeStorageConfig already defines basePath and the storage interface uses atomicWrites — update StorageSettings and all usages to match.

• Update packages/tm-core/src/interfaces/configuration.interface.ts: remove the duplicated basePath declaration from StorageSettings and rename atomicOperations → atomicWrites (align optionality with storage.interface.ts).
• Update packages/tm-core/src/config/services/config-loader.service.ts: replace atomicOperations → atomicWrites in getDefaultConfig.
• Update packages/tm-core/src/config/services/config-loader.service.spec.ts: update test expectations to atomicWrites.

Apply this patch to configuration.interface.ts:

 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 */
 	enableBackup: boolean;
 	/** Maximum number of backups to retain */
 	maxBackups: number;
 	/** Enable compression for storage */
 	enableCompression: boolean;
 	/** File encoding for text files */
 	encoding: BufferEncoding;
-	/** Enable atomic file operations */
-	atomicOperations: boolean;
+	/** Enable atomic file writes */
+	atomicWrites?: 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.

Suggested change

	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 */
	enableBackup: boolean;
	/** Maximum number of backups to retain */
	maxBackups: number;
	/** Enable compression for storage */
	enableCompression: boolean;
	/** File encoding for text files */
	encoding: BufferEncoding;
	/** Enable atomic file operations */
	atomicOperations: boolean;
	}
	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 */
	enableBackup: boolean;
	/** Maximum number of backups to retain */
	maxBackups: number;
	/** Enable compression for storage */
	enableCompression: boolean;
	/** File encoding for text files */
	encoding: BufferEncoding;
	/** Enable atomic file writes */
	atomicWrites?: boolean;
	}

🤖 Prompt for AI Agents

In packages/tm-core/src/interfaces/configuration.interface.ts around lines 109
to 129, remove the duplicate basePath property (since RuntimeStorageConfig
already defines it) and rename atomicOperations to atomicWrites, matching the
name and optionality/type used in storage.interface.ts; then update all usages
accordingly — specifically, in
packages/tm-core/src/config/services/config-loader.service.ts replace
atomicOperations → atomicWrites in getDefaultConfig, and update
packages/tm-core/src/config/services/config-loader.service.spec.ts to expect
atomicWrites instead of atomicOperations.