Conversation
…y browsers or firewalls (#1492)
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Ralph Khreish <35776126+Crunchyman-ralph@users.noreply.github.com>
…h.dependencies
🦋 Changeset detectedLatest commit: 3cf3825 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis PR refactors the autopilot workflow system to centralize around a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant tmCore as TM Core Facade
participant WS as Workflow Service
participant Orchestrator as Workflow Orchestrator
participant Git as Git Adapter
participant AuthStore as Auth/State Store
CLI->>tmCore: createTmCore({ projectPath })
tmCore->>WS: initialize with projectRoot
tmCore->>WS: setTasksDomain(for status updates)
alt Start Workflow
CLI->>tmCore: workflow.start(taskId, subtasks, options)
tmCore->>WS: startWorkflow()
WS->>Git: createBranch(tm/{orgSlug|tag}/...)
WS->>Orchestrator: initialize & transition to SUBTASK_LOOP
WS->>AuthStore: persist workflow state
WS-->>tmCore: return status
tmCore-->>CLI: status with taskId, phase, branchName
end
alt Resume & Progress
CLI->>tmCore: workflow.resume()
tmCore->>AuthStore: loadWorkflowState()
tmCore->>Orchestrator: restore & resume
tmCore->>WS: getStatus()
WS-->>tmCore: current status + phase
tmCore-->>CLI: status
end
alt Finalize
CLI->>tmCore: workflow.finalize()
tmCore->>WS: finalizeWorkflow()
WS->>AuthStore: delete state file
WS->>AuthStore: updateTaskStatus(DONE) via updater
WS-->>tmCore: final status
tmCore-->>CLI: success with results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (8)scripts/modules/commands.js📄 CodeRabbit inference engine (.cursor/rules/architecture.mdc)
Files:
**/*.js📄 CodeRabbit inference engine (.cursor/rules/architecture.mdc)
Files:
scripts/**/*.js📄 CodeRabbit inference engine (.cursor/rules/context_gathering.mdc)
Files:
scripts/modules/**/*📄 CodeRabbit inference engine (.cursor/rules/dev_workflow.mdc)
Files:
scripts/modules/*.js📄 CodeRabbit inference engine (.cursor/rules/mcp.mdc)
Files:
**/*.{js,jsx}📄 CodeRabbit inference engine (.cursor/rules/test_workflow.mdc)
Files:
**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
Files:
scripts/modules/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/utilities.mdc)
Files:
🧠 Learnings (20)📓 Common learnings📚 Learning: 2025-11-24T17:58:07.992ZApplied to files:
📚 Learning: 2025-11-24T17:57:31.417ZApplied to files:
📚 Learning: 2025-11-24T17:58:07.992ZApplied to files:
📚 Learning: 2025-11-24T18:00:06.827ZApplied to files:
📚 Learning: 2025-11-24T18:04:43.972ZApplied to files:
📚 Learning: 2025-11-24T17:57:31.417ZApplied to files:
📚 Learning: 2025-11-24T18:02:22.305ZApplied to files:
📚 Learning: 2025-11-24T18:05:23.901ZApplied to files:
📚 Learning: 2025-11-24T17:57:14.743ZApplied to files:
📚 Learning: 2025-11-24T17:57:14.743ZApplied to files:
📚 Learning: 2025-11-24T18:02:36.388ZApplied to files:
📚 Learning: 2025-11-24T18:01:44.169ZApplied to files:
📚 Learning: 2025-11-24T18:02:04.644ZApplied to files:
📚 Learning: 2025-11-24T18:02:36.388ZApplied to files:
📚 Learning: 2025-11-24T18:02:04.644ZApplied to files:
📚 Learning: 2025-11-24T18:04:43.972ZApplied to files:
📚 Learning: 2025-11-24T18:04:43.972ZApplied to files:
📚 Learning: 2025-11-24T17:58:07.992ZApplied to files:
📚 Learning: 2025-11-24T18:02:36.388ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
.github/workflows/claude-docs-updater.yml (1)
1-1: PR description is missing and must be provided before merging.The commit body is empty. Per the repository's Git workflow standards (
.cursor/rules/git_workflow.mdc), PR descriptions must include: Task Overview, Subtasks Completed (with checkmarks), Implementation Details, Testing Approach, Breaking Changes (if any), and Related Tasks.This PR affects a large number of files across multiple packages. Without a description, reviewers cannot understand what was changed, why, or how to test it. Please provide a comprehensive description explaining:
- What functionality was added/refactored and the rationale
- Which subtasks or components were modified
- How the changes were tested (unit, integration, manual workflow tests)
- Any breaking changes for users or developers
- Links to related issues or tasks
packages/ai-sdk-provider-grok-cli/CHANGELOG.md (1)
3-17: Fix malformed changelog entries with "## null" headings.The changelog contains multiple "## null" entries instead of proper version numbers. This indicates a problem with the changelog generation tooling (likely changesets) and will break:
- Changelog parsing by the custom changelog parser in
apps/cli/src/utils/auto-update/changelog.ts- User experience when reading release notes
- Automated version detection
This needs to be fixed before merging. Common causes:
- Missing version field in package.json (though learnings indicate this is intentional for internal packages)
- Malformed changeset files
- Bug in changesets configuration
Run the following to investigate:
#!/bin/bash # Check package.json for version field cat packages/ai-sdk-provider-grok-cli/package.json | jq '.version' # Look for related changeset files fd -e md . .changeset/ -x cat {} \; | head -50apps/cli/CHANGELOG.md (1)
17-22: Duplicate null changelog entries.Multiple "## null" sections with identical "Updated dependencies" content reduce changelog readability. These appear to be changeset generation artifacts that should be consolidated or cleaned up before release.
Based on learnings, the project uses a custom changelog format with PR numbers and author acknowledgements. Consider consolidating these null entries or ensuring changesets generate proper version headers.
CHANGELOG.md (1)
264-276: Remove duplicated 0.36.0 section."## 0.36.0" appears twice back-to-back; keep the first block (lines 251-262) and delete the duplicate (lines 264-276) to avoid parser noise and double-rendering.
Apply this minimal diff:
@@ -## 0.36.0 - -### Minor Changes - -- [#1446](https://github.com/eyaltoledano/claude-task-master/pull/1446) [`2316e94`](https://github.com/eyaltoledano/claude-task-master/commit/2316e94b288915bb906e1a61a87f59e291594fef) Thanks [@Crunchyman-ralph](https://github.com/Crunchyman-ralph)! - Bring back `task-master generate` as a command and mcp tool (after popular demand) - - Generated files are now `.md` instead of `.txt` - - They also follow the markdownlint format making them look like more standard md files - - added parameters to generate allowing you to generate with the `--tag` flag - - If I am on an active tag and want to generate files from another tag, I can with the tag parameter - - See `task-master generate --help` for more information. - -- [#1454](https://github.com/eyaltoledano/claude-task-master/pull/1454) [`38ff7eb`](https://github.com/eyaltoledano/claude-task-master/commit/38ff7ebbc029919ea4cd5257573efbf1ea2f0eeb) Thanks [@Crunchyman-ralph](https://github.com/Crunchyman-ralph)! - Add Hamster rules to task-master rulesapps/mcp/src/tools/autopilot/commit.tool.ts (1)
146-149: Commit type logic is always 'feat' since we're in COMMIT phase.Since line 68 already validates that
status.tddPhase === 'COMMIT', the ternary on line 148 will always evaluate to'feat'. The'test'branch is dead code.If the intent is to determine type based on the work done (tests vs implementation), consider using the previous TDD phase or subtask context instead.
// Determine commit type based on phase and subtask // RED phase = test files, GREEN phase = implementation - const type = status.tddPhase === 'COMMIT' ? 'feat' : 'test'; + const type = 'feat'; // Always in COMMIT phase at this pointOr if you want to preserve the original intent:
// Determine commit type based on phase and subtask // RED phase = test files, GREEN phase = implementation - const type = status.tddPhase === 'COMMIT' ? 'feat' : 'test'; + // Since we're in COMMIT phase after GREEN, the work is implementation + const type = 'feat';apps/cli/src/commands/autopilot/complete.command.ts (1)
103-143: RED phase validation in CLI contradicts core behavior—reconsider the requirement for at least one failing test.The core WorkflowOrchestrator explicitly handles the case where all tests pass in the RED phase as a valid edge case (
feature-already-implemented), automatically completing the subtask. However, the CLI validation (lines 104-107) rejects this case and requires at least one failing test.The GREEN phase validation (lines 116-119) correctly mirrors core logic (both require zero failures), but the RED phase constraint in the CLI prevents a legitimate workflow path that the core supports. Either the CLI should allow this case to reach the core for proper handling, or the core behavior should be changed to match the CLI restriction—clarify which is intended.
♻️ Duplicate comments (2)
packages/build-config/CHANGELOG.md (1)
19-20: Fix malformed "## null" changelog entry.Similar to the issue in
packages/ai-sdk-provider-grok-cli/CHANGELOG.md, this file has malformed "## null" entries instead of proper version numbers. This is part of a systemic changelog generation problem affecting multiple internal packages.See the comment on
packages/ai-sdk-provider-grok-cli/CHANGELOG.mdfor investigation steps. This needs to be resolved before merging.packages/tm-core/CHANGELOG.md (1)
19-20: Fix malformed "## null" changelog entry.This file also has the "## null" changelog generation issue affecting multiple internal packages in this PR. This is part of the same systemic problem.
See the investigation steps in the comment on
packages/ai-sdk-provider-grok-cli/CHANGELOG.md.
🧹 Nitpick comments (19)
packages/tm-core/src/modules/tasks/validation/task-id.ts (2)
139-155: Documentation references unsupported format.The examples show
"1.2.3"which is explicitly documented as not supported in the file header (line 16). While the function would technically handle it, showing this example may cause confusion since such IDs won't pass validation.Consider updating to use supported formats:
/** * Extract parent task ID from a subtask ID * - * @param taskId - Task ID (e.g., "1.2.3") - * @returns Parent ID (e.g., "1") or the original ID if not a subtask + * @param taskId - Task ID (e.g., "1.2") + * @returns Parent ID (e.g., "1") or the original ID if not a subtask * * @example * ```typescript - * extractParentId("1.2.3"); // "1" * extractParentId("1.2"); // "1" * extractParentId("1"); // "1" + * extractParentId("HAM-1"); // "HAM-1" (no subtask notation) * ``` */
157-172: Same documentation inconsistency with unsupported format.Similar to
extractParentId, the example shows"1.2.3"which validation rejects.Consider aligning examples with supported formats:
* @example * ```typescript * isSubtaskId("1.2"); // true - * isSubtaskId("1.2.3"); // true * isSubtaskId("1"); // false + * isSubtaskId("HAM-1"); // false (API IDs don't use dot notation) * ```packages/tm-core/src/modules/workflow/orchestrators/workflow-orchestrator.spec.ts (4)
92-96: Async invalid-transition expectation looks correct; consider consistency across testsUsing
await expect(orchestrator.transition(...)).rejects.toThrow('Invalid transition')is appropriate iftransitionis now promise-based for invalid transitions. You might consider standardizing onawait(orreturn expect(...).rejects...) for other tests that rely ontransitionside effects as well, to keep behavior deterministic iftransitionbecomes uniformly async in the future.
435-461: Guard/test-result/git validation tests now correctly assert async failures; watch skipped specs and namingThe refactors to use
await+.rejects.toThrow(...)for guard failures, missing test results, invalid GREEN results, and git guards align these tests with an asynctransitionAPI and look logically sound. Two minor follow-ups:
- The skipped RED-phase tests (Lines 502-525) still assert the old failure semantics; now that behavior is “auto-complete on all tests passing”, it would be better to either (a) replace them with a positive spec for the new behavior, or (b) remove the skipped tests to avoid stale expectations lingering in the suite.
- The description
"should validate test results before GREEN phase transition"(Lines 489-500) is a bit misleading since you’re asserting thatRED_PHASE_COMPLETEwithouttestResultsis rejected; consider renaming to reflect the RED→GREEN validation more explicitly.Also applies to: 489-525, 548-579, 637-659
1073-1088: Abort behavior test correctly uses async rejection; consider symmetry with the non-async abort testThis test now properly verifies that, after
ABORT, any furthertransitioncall rejects with'Workflow has been aborted', which matches an async failure model. Given this, you may want (optionally) to alsoawaittheABORTtransition in the earlier"should support abort workflow"test to keep the semantics consistent and avoid potential races ifABORTever does more asynchronous work.
1401-1424: Adapter/TestResultValidator async tests look good; reconsider the skipped RED-phase validator specThe GREEN-phase validator test now cleanly exercises the adapter path by awaiting transitions and asserting a rejected
GREEN_PHASE_COMPLETEwith failing tests, which matches the intended contract forTestResultValidator. For the RED-phase validator spec that is now skipped, you’ve documented the behavior change (auto-complete instead of throwing); that’s helpful, but it leaves the new behavior untested. I’d recommend replacing the skipped test with one that asserts the auto-completion semantics via the validator path, so adapter integration remains covered for both phases.Also applies to: 1426-1459
packages/tm-bridge/CHANGELOG.md (1)
31-36: Avoid adding another duplicate## nullchangelog sectionThere are already multiple identical
## null/ “Updated dependencies []: @tm/core@null” blocks above; adding another increases duplication and triggers markdownlint’s MD024 duplicate-heading warning. Consider consolidating these into a single section or replacing them with a real version label when available.packages/build-config/scripts/fix-null-versions.ts (1)
26-35: Consider adding error handling for file operations.The script reads/writes files without try/catch. If a file operation fails (permissions, disk full, etc.), the script crashes mid-execution, potentially leaving some files fixed and others not.
for (const file of packageFiles) { + try { const content = readFileSync(file, 'utf8'); if (content.includes('"version": null')) { const updated = content.replace(/"version": null/g, '"version": ""'); writeFileSync(file, updated); console.log(`Fixed: ${file}`); fixed++; } + } catch (error) { + console.error(`Failed to process: ${file}`, error); + } }packages/tm-core/src/modules/auth/utils/cli-crypto.ts (1)
69-107: Consider validating required fields after JSON parsing.The type assertion at line 99 doesn't provide runtime guarantees. If the decrypted JSON is malformed or missing required fields (
access_token,user_id), downstream code may fail with unclear errors.- return JSON.parse(decrypted.toString('utf8')) as DecryptedTokens; + const parsed = JSON.parse(decrypted.toString('utf8')) as DecryptedTokens; + + // Validate required fields + if (!parsed.access_token || !parsed.user_id) { + throw new Error('Missing required token fields'); + } + + return parsed;apps/docs/CHANGELOG.md (1)
3-4: Empty version entry follows existing pattern.The new
0.0.13entry is consistent with the existing changelog format in this file. Consider adding a brief note about what changed (even if just "dependency updates") for better changelog hygiene.apps/cli/src/commands/set-status.command.ts (1)
6-11: TaskIdSchema-based ID handling looks good; consider unifying JSON error outputThe switch to
TaskIdSchema.safeParsefor each trimmed ID is a solid improvement — it centralizes validation/normalization in tm-core instead of reimplementing ID parsing here, and it correctly collects only successfully parsed IDs before updating status.One behavioral nuance: for an invalid ID you now:
- Log a chalk-colored error with the Zod issue message, then
- Call
process.exit(1)directly from the loop.This matches the existing patterns for missing ID/status and invalid status, but it means that even when
--format jsonis requested, the invalid-ID case won’t go through the JSON error path used later in the catch block.If you want fully machine-friendly behavior in JSON mode, consider (in a follow-up) routing invalid-ID failures through the same error handling as other failures (e.g., throw an Error with the composed message and let the existing
catchblock emit JSON or text as appropriate) instead of callingprocess.exit(1)here. That would also keep CLI exit handling more centralized.Also applies to: 153-167
CHANGELOG.md (2)
3-12: Top entry format looks good; add current PR (#1498) details.Please add a line for this PR under 0.37.2-rc.0 using the repo’s standard format so release notes reflect the actual change set.
Example (adjust text to match the final scope):
- #1498
<commit>Thanks @Crunchyman-ralph! - <short, user-facing summary in imperative mood>
7-12: Wording nit: clarify the second bullet under #1491.Consider “Fix inability to start workflow on master Taskmaster tag for some users” for clarity.
tests/unit/mcp/tools/tool-registration.test.js (1)
123-177: Tests exercise newtoolModebehavior thoroughly; one minor wording nitThe updated cases cover defaulting to
core, explicit modes (all,core,standard,lean), custom lists, invalid/edge inputs, and combined env + parameter scenarios, which matches the newregisterTaskMasterTools(server, toolMode)contract well.Tiny optional cleanup: the test
"should use core tools when empty string passed as toolMode"(Lines 208–214) actually setsTASK_MASTER_TOOLS = ''and then callsregisterTaskMasterTools(mockServer)without a second argument, so it’s really exercising “empty env value” rather than an explicit emptytoolModeparameter. Renaming the description to mention env config instead of “passed as toolMode” would avoid confusion, but behavior itself looks correct.Also applies to: 200-215, 252-304, 322-328, 404-423
apps/cli/src/commands/autopilot/commit.command.ts (1)
5-5: CLI commit now correctly usescreateTmCore+tmCore.workflowSwitching to
createTmCore({ projectPath })and driving all workflow interactions throughtmCore.workflow(existence check, resume, status/context read,commit()) keeps the command in line with the “CLI as thin presentation layer over tm-core” guideline and removes direct dependency on workflow internals.Two tiny, non-blocking nits:
- The comment above
tmCore.workflow.commit()still mentions “handled internally by WorkflowService”; consider updating it to “bytmCore.workflow” for clarity.- If more autopilot commands follow this exact initialization pattern, a small shared helper for “init tmCore + assert active workflow + resume + getStatus” could reduce duplication later.
Also applies to: 42-47, 53-57, 119-121
packages/tm-core/src/modules/workflow/workflow-domain.ts (1)
153-155: Minor:hasWorkflowcreates service instance if none exists.The
hasWorkflowmethod usesgetWorkflowService()which lazily creates a service if none exists. This is acceptable since callers typically follow withstart()orresume()which reset the service anyway. However, if this becomes a performance concern, consider a lighter-weight check that doesn't require full service instantiation.apps/cli/src/commands/autopilot/finalize.command.ts (1)
26-40: Consider simplifying option merging to avoid redundant initialization.The
mergedOptionsis initialized withprojectRoot: ''on line 34 and then immediately overwritten on lines 48-51. This double initialization is unnecessary.private async execute(options: FinalizeOptions): Promise<void> { // Inherit parent options const parentOpts = this.parent?.opts() as AutopilotBaseOptions; - - // Initialize mergedOptions with defaults (projectRoot will be set in try block) - let mergedOptions: FinalizeOptions = { - ...parentOpts, - ...options, - projectRoot: '' // Will be set in try block - }; - const formatter = new OutputFormatter( options.json || parentOpts?.json || false ); try { // Resolve project root inside try block to catch any errors const projectRoot = getProjectRoot( options.projectRoot || parentOpts?.projectRoot ); - // Update mergedOptions with resolved project root - mergedOptions = { + const mergedOptions: FinalizeOptions = { ...parentOpts, ...options, projectRoot };packages/tm-core/src/common/schemas/task-id.schema.ts (1)
70-74: Consider documenting whether the 3-letter prefix is fixed to "HAM" or extensible.The
API_MAIN_PATTERNaccepts any 3-letter prefix (e.g., "ABC-1", "XYZ-1"), not just "HAM". The docstring mentions "HAM-1" specifically. If only "HAM" is valid, consider tightening the pattern; otherwise, update the documentation to clarify the flexibility.apps/cli/src/commands/autopilot/start.command.ts (1)
106-111: Consider typing the subtask mapping parameter.The
anytype annotation onstloses type safety. If aSubtasktype is available from@tm/core, consider using it for better IDE support and compile-time checks.- subtasks: task.subtasks.map((st: any) => ({ + subtasks: task.subtasks.map((st) => ({ id: st.id, title: st.title, status: st.status, maxAttempts })),If the
tasktype fromtmCore.tasks.get()is properly typed, removinganyshould allow TypeScript to infer the correct type.
2 participants
What type of PR is this?
Description
Related Issues
How to Test This
# Example commands or stepsExpected result:
Contributor Checklist
npm run changesetnpm testnpm run format-check(ornpm run formatto fix)Changelog Entry
For Maintainers
Summary by CodeRabbit
New Features
Bug Fixes
Changes
tm/<org-slug>/task-<id>prefix✏️ Tip: You can customize this high-level summary in your review settings.