feat: unassign tasks

[fblgit]

This PR:

• feat: task.unasasign handler
• feat: ui task unassign action

[claude]

Claude finished @fblgit's task —— View job

---

✅ Code Review Complete for PR #21: feat: unassign tasks

I've completed my review of the task unassign functionality implementation.

Review Summary

The PR successfully implements task unassignment with good UI integration and documentation. However, there are some important issues to address:

Key Issues Found:

1. Missing Atomic Operations - Multiple Redis operations should be combined into a Lua script for atomicity
2. No Test Coverage - Missing both contract and integration tests
3. Potential Race Conditions - Task status validation should be part of the atomic operation
4. Missing Prisma Fields - The updatedAt field isn't updated in the database
5. Metrics Validation - Instance metrics could go negative without bounds checking

Recommendations:

The implementation is solid but needs refinement around atomicity and testing. Please see the detailed review comment for specific recommendations and code examples.

Once these issues are addressed, this will be a valuable addition to the task management system. The UI integration and documentation are particularly well done.

[claude]

Code Review for PR #21: feat: unassign tasks

I've reviewed the implementation of the task unassign functionality. Here's my detailed feedback:

✅ Strengths

1. Well-structured implementation - The handler follows the established patterns with appropriate decorators (@EventHandler, @Instrumented, @Resilient)
2. Good error handling - Properly validates task existence, assignment status, and prevents unassigning completed/failed tasks
3. Comprehensive documentation - The API documentation is thorough with clear examples and use cases
4. UI integration - Clean integration in both TaskCard and TaskKanban components with appropriate UI feedback
5. Event emission - Correctly publishes task.unassigned event following past-tense convention
6. Schema definitions - Properly defined Zod schemas with input/output types

⚠️ Issues & Recommendations

1. Missing Atomic Operations ❗

The handler performs multiple Redis operations that should be atomic (apps/server/src/handlers/task/task.unassign.handler.ts:53-69):

// Multiple non-atomic operations
await ctx.redis.stream.hdel(taskKey, "assignedTo", "assignedAt");
await ctx.redis.stream.hset(taskKey, { status: "pending", updatedAt: now });
await ctx.redis.stream.zadd(pendingQueueKey, score, input.taskId);
await ctx.redis.stream.lrem(instanceQueueKey, 0, input.taskId);

Recommendation: According to CLAUDE.md, use a Lua script for atomicity:

• Create an UNASSIGN_TASK Lua script in core/redis-scripts.ts
• Bundle all Redis operations into a single atomic transaction
• This prevents race conditions where task state could become inconsistent

2. Missing Test Coverage ❗

No tests found for the unassign functionality:

• No contract tests in apps/server/tests/contract/
• No integration tests in apps/server/tests/integration/

Recommendation: Add tests following the patterns in existing test files:

• Contract test to verify API shape matches specification
• Integration test to verify Redis state changes and queue management

3. Potential Race Condition ⚠️

Between checking task status (line 45-48) and performing the unassign operations, the task could be modified by another process.

Recommendation: Include status validation in the atomic Lua script to ensure consistency.

4. Handler Length 📏

The handler is 114 lines, exceeding the <50 lines guideline from CLAUDE.md (apps/server/src/handlers/task/task.unassign.handler.ts).

Recommendation: Moving to a Lua script will significantly reduce handler size.

5. Missing Prisma Update Fields ⚠️

The Prisma update (line 86-92) doesn't update updatedAt:

await ctx.prisma.task.update({
  where: { id: input.taskId },
  data: {
    assignedTo: null,
    status: "pending",
    // Missing: updatedAt: new Date()
  },
});

6. Instance Metrics Handling ⚠️

The instance metrics decrement (line 82) doesn't check if the metric exists or handle negative values:

await ctx.redis.stream.hincrby(instanceTasksKey, "assignedTasks", -1);

Recommendation: Add bounds checking to prevent negative metrics.

📝 Required Actions

1. Implement atomic Redis operations using a Lua script
2. Add test coverage (both contract and integration tests)
3. Update Prisma operation to include updatedAt field
4. Add metrics validation to prevent negative values

💡 Minor Suggestions

• Consider adding a reason/comment field for audit purposes
• The UI could show a confirmation dialog before unassigning
• Consider rate limiting per task to prevent rapid assign/unassign cycles

Summary

The implementation is solid and follows most conventions well. The main concerns are around atomicity and test coverage. Once these issues are addressed, this will be a valuable addition to the task management system.