Task decompose handler

[fblgit]

This PR:

• feat: task.decompose (modern general version of swarm.decompose)
• docs: task.decompose documentation

[claude]

Claude finished @fblgit's task —— View job

---

✅ Code Review Complete

I've posted a comprehensive review of the task.decompose and task.create_project handlers. The implementation shows good architecture and excellent documentation, but there are critical issues that need addressing:

Key Findings:

• ❌ BLOCKER: Missing all required tests (contract and integration)
• ⚠️ Critical: Persistence disabled unlike swarm.decompose - needs justification
• ⚠️ Data Issue: Subtask Redis keys lack expiration → potential orphaned data
• 🔧 Performance: Sequential operations could be batched/pipelined
• 📏 Size: Handlers exceed 50-line guideline by 5-7x

Verdict: Request Changes ❌

The lack of test coverage violates repository standards and must be addressed before merging. Please see the detailed review comment for all findings and recommendations.

[claude]

Code Review: task.decompose Handler

I've reviewed the implementation of the new task.decompose handler and related changes. Here's my comprehensive feedback:

✅ Strengths

1. Clean Migration Path: The migration from swarm.decompose to task.decompose is well-designed with:

   • Preserved core functionality while improving domain alignment
   • Comprehensive migration documentation
   • Backwards-compatible response structure with added attachment support

2. Architecture Adherence: The implementation follows CLAUDE.md patterns excellently:

   • Proper use of @eventhandler, @Instrumented, and @resilient decorators
   • Atomic Redis operations for subtask storage
   • Event publishing pattern (task.decomposed, task.subtask.ready)
   • Under 253 lines (well within the 50-line guideline target)

3. Documentation Quality: Excellent documentation with:

   • Clear API documentation in docs/api/task/decompose.md
   • Architecture deep-dive in docs/architecture/task-decomposition.md
   • Migration guide with code examples

🔍 Issues & Suggestions

1. Missing Test Coverage (Critical)

No integration or contract tests were found for the new handler. Per CLAUDE.md:

• Need contract test in apps/server/tests/contract/
• Need integration test in apps/server/tests/integration/

2. Inconsistent Persistence Configuration

Line 14 in task.decompose.handler.ts has persist: false while swarm.decompose has persist: true. This might cause data loss if Redis is flushed. Consider if this is intentional.

3. Error Handling Improvements

Line 84-89: The fallback to Prisma when task not in Redis is good, but the error message is identical in both cases. Consider differentiating: "Task not found in Redis or database" vs "Task not found in Redis (database not available)"

Line 96-103: Default specialists fallback silently creates dummy specialists. Consider:

• Making this behavior configurable
• Emitting a warning event for monitoring
• Document this fallback behavior in API docs

4. Redis Key Expiration Strategy

Line 143: 7-day expiration for decomposition cache - consider if this aligns with task lifecycle. What if tasks take longer than 7 days?

Lines 187-206: Subtask keys have no expiration set. This could lead to orphaned data if tasks are never completed.

5. Performance Considerations

Lines 187-206: Multiple Redis operations in a loop without batching. Consider using pipelining or a Lua script for atomic bulk operations.

6. Type Safety Gap

The handler accepts 'task' as a string parameter but also expects 'taskId'. This could be confusing. The schema allows up to 1000 chars for task description but the handler doesn't validate if this matches the actual task text in the system.

📝 Recommendations

1. Add Tests Immediately:

   • Contract test verifying JSON-RPC interface
   • Integration test checking Redis state changes
   • Test the attachment creation flow

2. Consider Persistence Strategy:

   • Either enable PostgreSQL persistence or document why it's not needed
   • Add cleanup strategy for old decompositions

3. Improve Error Messages:

   • Add context about where lookups failed
   • Include suggestions for resolution

4. Add Monitoring Hooks:

   • Emit metrics for decomposition complexity distribution
   • Track specialist availability issues
   • Monitor attachment storage success rate

🎯 Overall Assessment

The implementation is solid and well-documented. The migration from swarm to task domain makes conceptual sense and the attachment-based storage is cleaner. However, the lack of tests is a blocker and should be addressed before merging. The other issues are minor but worth considering for production robustness.

Verdict: Approve with required changes (add tests, review persistence strategy)

[claude]

Code Review Complete 🔍

I've completed my review of the task.decompose handler implementation. The code is well-architected with excellent documentation, but there are critical issues that need to be addressed before merging.

Key Findings:

• ❌ BLOCKER: Missing required tests (contract and integration)
• ⚠️ Important: Persistence disabled unlike swarm.decompose - needs clarification
• ⚠️ Data Issue: Subtask Redis keys lack expiration (potential orphans)
• 🔧 Performance: Multiple Redis operations could be batched

Please see the detailed review above for all findings and recommendations.

Next Steps:

1. Add the required test coverage
2. Address the persistence configuration
3. Fix Redis key lifecycle management
4. Consider the performance optimizations

Feel free to ask if you need clarification on any of the review points!

[claude]

Code Review: task.decompose & task.create_project Handlers

📊 Summary

This PR introduces two new handlers (task.decompose and task.create_project) to modernize the task decomposition system. While the implementation is well-structured with excellent documentation, there are critical issues that must be addressed before merging.

🚨 Critical Issues (Blockers)

1. Missing Test Coverage

❌ No tests found for either new handler. This violates CLAUDE.md requirements:

• Missing: apps/server/tests/contract/task.decompose.contract.test.ts
• Missing: apps/server/tests/contract/task.create_project.contract.test.ts
• Missing: apps/server/tests/integration/task-decompose.test.ts
• Missing: apps/server/tests/integration/task-project.test.ts

2. Inconsistent Persistence Configuration

• task.decompose.handler.ts:14: persist: false (differs from swarm.decompose which has persist: true)
• task.create_project.handler.ts:12: persist: false
• Risk: Data loss if Redis is flushed. Need clarification on whether this is intentional.

⚠️ Important Issues

3. Redis Key Lifecycle Management

• Lines 187-206 (task.decompose): Subtask keys have no expiration set → potential orphaned data
• Line 143: Decomposition cache expires in 7 days, but tasks might take longer
• Line 121 (task.create_project): Project keys expire in 7 days - same concern

4. Performance Bottlenecks

• Lines 187-206 (task.decompose): Multiple Redis operations in loop without pipelining
• Lines 198-242 (task.create_project): Creating multiple tasks sequentially without batching
• Recommendation: Use Redis pipeline or Lua script for atomic bulk operations

5. Error Handling Gaps

• Lines 84-89 (task.decompose): Identical error messages for Redis vs database failures
• Lines 96-103: Silent fallback to dummy specialists without warning events
• Lines 239-241 (task.create_project): Errors logged but execution continues, potentially leaving partial state

✅ Strengths

1. Excellent Documentation: Comprehensive API docs, architecture deep-dive, and migration guide
2. Clean Architecture: Proper use of decorators, event publishing, and attachment system
3. Good Domain Alignment: Moving from swarm to task domain makes conceptual sense
4. Session Support: Enhanced tracking with sessionId throughout the flow
5. Attachment-Based Storage: Clean integration with the attachment system

🔧 Additional Observations

6. Handler Complexity

• task.create_project: 373 lines (exceeds CLAUDE.md 50-line guideline by 7x)
• task.decompose: 251 lines (exceeds guideline by 5x)
• Consider splitting into smaller, focused handlers

7. Type Safety

• The "task" parameter in decompose could be confusing (is it taskId or description?)
• Consider renaming to "taskDescription" for clarity

8. Dependency on External Services

• Both handlers depend on getSamplingService() which could fail
• Circuit breaker fallbacks return empty results - might be better to fail fast

📝 Required Actions Before Merge

1. Add comprehensive test coverage (contract and integration tests)
2. Clarify persistence strategy - should these handlers persist to PostgreSQL?
3. Fix Redis key expiration for subtasks
4. Optimize bulk operations with pipelining or Lua scripts
5. Improve error messages to distinguish failure points
6. Consider handler decomposition to meet size guidelines

💡 Suggestions

• Add metrics emission for decomposition complexity distribution
• Consider making specialist fallback behavior configurable
• Document the 7-day expiration rationale or make it configurable
• Add cleanup strategy for orphaned subtasks

Verdict: Request Changes ❌

The implementation is solid with excellent documentation, but the lack of tests is a blocker per repository standards. The persistence inconsistency and Redis key lifecycle issues need resolution before this can be safely merged.

---

Review based on CLAUDE.md guidelines and repository best practices