Skip to content

Commit ef50b8e

Browse files
jpicklykclaude
jpicklyk
and
claude
authored
refactor: code quality improvements — decompose god class, extract shared patterns (#66)
* fix(deps): exclude RELATES_TO edges from cycle detection RELATES_TO dependencies are purely informational and cannot create blocking deadlocks. The DFS cycle detection was incorrectly traversing RELATES_TO edges, causing false positive cycle rejections — e.g., creating B RELATES_TO A when A BLOCKS B existed would be rejected. Three changes: - DFS filter: `!= IS_BLOCKED_BY` → `== BLOCKS` (excludes RELATES_TO) - insertDependencyInTransaction: skip cycle check for RELATES_TO - createBatch: skip cycle check for RELATES_TO Adds 5 TDD tests covering the fix and regression guards. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: decompose ManageItemsTool into focused handlers Extract create, update, and delete operations from the 702-line god class into CreateItemHandler, UpdateItemHandler, and DeleteItemHandler. Consolidate duplicated hierarchy validation (cycle detection, depth computation) into shared ItemHierarchyValidator service. Extract JSON field helpers into ItemFieldExtractors. ManageItemsTool is now a ~170-line dispatcher. MCP interface unchanged — all existing tests pass without modification. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: extract shared repository transaction wrapper Add DatabaseManager.suspendedTransaction() extension function that wraps newSuspendedTransaction + try/catch into a single call. Apply across all 33 suspend repository methods in SQLiteNoteRepository (7), SQLiteRoleTransitionRepository (5), and SQLiteWorkItemRepository (21). Net reduction of ~80 lines of boilerplate. Consolidates the deprecated newSuspendedTransaction call to a single site for future Exposed migration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: consolidate entity JSON serialization into shared serializers Extract WorkItem, Note, and Dependency serialization into extension functions in EntityJsonSerializers.kt. Replace 17+ inline role.name.lowercase() / priority.name.lowercase() calls across 10 tool files with Role.toJsonString() / Priority.toJsonString() helpers. Removes duplicate private serializer functions from QueryItemsTool and QueryNotesTool, reducing ~40 lines of duplicated code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: unify parameter extraction with shared helpers Add extractItemBoolean to ItemFieldExtractors for handler-context boolean extraction. Replace inline jsonObject["key"]?.jsonPrimitive ?.booleanOrNull patterns with optionalBoolean() across 5 tool files. Replace inline offset extraction with optionalInt() in QueryItemsTool. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: extract shared filter query builder in SQLiteWorkItemRepository Extract buildFilteredQuery() to eliminate exact duplicate filter construction between findByFilters() and countByFilters() — 20 lines of identical condition-building code consolidated into one method. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 2d48c85 commit ef50b8e

20 files changed

+1022
-921
lines changed

‎current/src/main/kotlin/io/github/jpicklyk/mcptask/current/application/service/ItemHierarchyValidator.kt‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
package io.github.jpicklyk.mcptask.current.application.service
2+
3+
import io.github.jpicklyk.mcptask.current.application.tools.ToolValidationException
4+
import io.github.jpicklyk.mcptask.current.domain.repository.Result
5+
import io.github.jpicklyk.mcptask.current.domain.repository.WorkItemRepository
6+
import java.util.UUID
7+
8+
/**
9+
* Validates parent-child hierarchy constraints for WorkItems and computes depth.
10+
*
11+
* Consolidates the duplicated validation logic previously in create and update operations:
12+
* - Self-parent guard
13+
* - Ancestor cycle detection (walk-up loop)
14+
* - Depth computation from parent
15+
* - Maximum depth enforcement
16+
*/
17+
class ItemHierarchyValidator {
18+
19+
companion object {
20+
/** Maximum allowed nesting depth for WorkItems. */
21+
const val MAX_DEPTH = 3
22+
}
23+
24+
/**
25+
* Validates hierarchy constraints and computes the depth for an item given its parent.
26+
*
27+
* @param itemId The UUID of the item being created or updated
28+
* @param parentId The target parent UUID, or null for root items
29+
* @param repo The WorkItem repository for ancestor lookups
30+
* @param errorPrefix Context string for error messages (e.g., "Item at index 2" or "Item 'abc-123'")
31+
* @return The computed depth (0 for root items, parent.depth + 1 for children)
32+
* @throws ToolValidationException if any hierarchy constraint is violated
33+
*/
34+
suspend fun validateAndComputeDepth(
35+
itemId: UUID,
36+
parentId: UUID?,
37+
repo: WorkItemRepository,
38+
errorPrefix: String
39+
): Int {
40+
if (parentId == null) return 0
41+
42+
// Guard: self-parent check
43+
if (parentId == itemId) {
44+
throw ToolValidationException("$errorPrefix: cannot be its own parent")
45+
}
46+
47+
// Guard: ancestor cycle check — walk up from parentId, ensure itemId is not an ancestor
48+
var cursor: UUID? = parentId
49+
repeat(MAX_DEPTH + 1) {
50+
val cursorId = cursor ?: return@repeat
51+
val ancestorResult = repo.getById(cursorId)
52+
val ancestor = when (ancestorResult) {
53+
is Result.Success -> ancestorResult.data
54+
is Result.Error -> return@repeat
55+
}
56+
if (ancestor.id == itemId) {
57+
throw ToolValidationException(
58+
"$errorPrefix: reparenting to '$parentId' would create a circular hierarchy"
59+
)
60+
}
61+
cursor = ancestor.parentId
62+
}
63+
64+
// Compute depth from parent
65+
val parentResult = repo.getById(parentId)
66+
return when (parentResult) {
67+
is Result.Success -> {
68+
val computedDepth = parentResult.data.depth + 1
69+
if (computedDepth > MAX_DEPTH) {
70+
throw ToolValidationException(
71+
"$errorPrefix: depth $computedDepth exceeds maximum depth of $MAX_DEPTH"
72+
)
73+
}
74+
computedDepth
75+
}
76+
is Result.Error -> throw ToolValidationException(
77+
"$errorPrefix: parent '$parentId' not found"
78+
)
79+
}
80+
}
81+
}

‎current/src/main/kotlin/io/github/jpicklyk/mcptask/current/application/tools/EntityJsonSerializers.kt‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
package io.github.jpicklyk.mcptask.current.application.tools
2+
3+
import io.github.jpicklyk.mcptask.current.domain.model.Dependency
4+
import io.github.jpicklyk.mcptask.current.domain.model.Note
5+
import io.github.jpicklyk.mcptask.current.domain.model.Priority
6+
import io.github.jpicklyk.mcptask.current.domain.model.Role
7+
import io.github.jpicklyk.mcptask.current.domain.model.WorkItem
8+
import kotlinx.serialization.json.*
9+
10+
// ──────────────────────────────────────────────
11+
// Enum formatting
12+
// ──────────────────────────────────────────────
13+
14+
/** Canonical JSON representation of a [Role] value (lowercase enum name). */
15+
fun Role.toJsonString(): String = this.name.lowercase()
16+
17+
/** Canonical JSON representation of a [Priority] value (lowercase enum name). */
18+
fun Priority.toJsonString(): String = this.name.lowercase()
19+
20+
// ──────────────────────────────────────────────
21+
// WorkItem serializers
22+
// ──────────────────────────────────────────────
23+
24+
/**
25+
* Full JSON representation of a [WorkItem] with all fields.
26+
* Used for `get` operations and detailed single-item responses.
27+
*/
28+
fun WorkItem.toFullJson(): JsonObject = buildJsonObject {
29+
put("id", JsonPrimitive(id.toString()))
30+
parentId?.let { put("parentId", JsonPrimitive(it.toString())) }
31+
put("title", JsonPrimitive(title))
32+
description?.let { put("description", JsonPrimitive(it)) }
33+
put("summary", JsonPrimitive(summary))
34+
put("role", JsonPrimitive(role.toJsonString()))
35+
statusLabel?.let { put("statusLabel", JsonPrimitive(it)) }
36+
previousRole?.let { put("previousRole", JsonPrimitive(it.toJsonString())) }
37+
put("priority", JsonPrimitive(priority.toJsonString()))
38+
put("complexity", JsonPrimitive(complexity))
39+
put("depth", JsonPrimitive(depth))
40+
metadata?.let { put("metadata", JsonPrimitive(it)) }
41+
tags?.let { put("tags", JsonPrimitive(it)) }
42+
put("createdAt", JsonPrimitive(createdAt.toString()))
43+
put("modifiedAt", JsonPrimitive(modifiedAt.toString()))
44+
put("roleChangedAt", JsonPrimitive(roleChangedAt.toString()))
45+
}
46+
47+
/**
48+
* Minimal JSON representation of a [WorkItem] for list/search responses.
49+
* Includes only: id, parentId, title, role, priority, depth, tags.
50+
*/
51+
fun WorkItem.toMinimalJson(): JsonObject = buildJsonObject {
52+
put("id", JsonPrimitive(id.toString()))
53+
parentId?.let { put("parentId", JsonPrimitive(it.toString())) }
54+
put("title", JsonPrimitive(title))
55+
put("role", JsonPrimitive(role.toJsonString()))
56+
put("priority", JsonPrimitive(priority.toJsonString()))
57+
put("depth", JsonPrimitive(depth))
58+
tags?.let { put("tags", JsonPrimitive(it)) }
59+
}
60+
61+
/**
62+
* JSON object mapping each [Role] to its child count.
63+
* Used in overview responses for child-count-by-role summaries.
64+
*/
65+
fun roleCountToJson(counts: Map<Role, Int>): JsonObject = buildJsonObject {
66+
for (role in Role.entries) {
67+
put(role.toJsonString(), JsonPrimitive(counts[role] ?: 0))
68+
}
69+
}
70+
71+
// ──────────────────────────────────────────────
72+
// Note serializer
73+
// ──────────────────────────────────────────────
74+
75+
/**
76+
* JSON representation of a [Note] with optional body inclusion.
77+
* Used for both single-note and list responses.
78+
*
79+
* @param includeBody When false, omits the `body` field (metadata-only queries).
80+
*/
81+
fun Note.toJson(includeBody: Boolean = true): JsonObject = buildJsonObject {
82+
put("id", JsonPrimitive(id.toString()))
83+
put("itemId", JsonPrimitive(itemId.toString()))
84+
put("key", JsonPrimitive(key))
85+
put("role", JsonPrimitive(role))
86+
if (includeBody) put("body", JsonPrimitive(body))
87+
put("createdAt", JsonPrimitive(createdAt.toString()))
88+
put("modifiedAt", JsonPrimitive(modifiedAt.toString()))
89+
}
90+
91+
// ──────────────────────────────────────────────
92+
// Dependency serializer
93+
// ──────────────────────────────────────────────
94+
95+
/**
96+
* JSON representation of a [Dependency] with core fields.
97+
* Tools that need additional fields (effectiveUnblockRole, item info) extend inline.
98+
*/
99+
fun Dependency.toJson(): JsonObject = buildJsonObject {
100+
put("id", JsonPrimitive(id.toString()))
101+
put("fromItemId", JsonPrimitive(fromItemId.toString()))
102+
put("toItemId", JsonPrimitive(toItemId.toString()))
103+
put("type", JsonPrimitive(type.name))
104+
unblockAt?.let { put("unblockAt", JsonPrimitive(it)) }
105+
}

‎current/src/main/kotlin/io/github/jpicklyk/mcptask/current/application/tools/compound/CreateWorkTreeTool.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,7 @@ Atomically create a hierarchical work tree: root item, child items, dependencies
314314
val rootJson = buildJsonObject {
315315
put("id", JsonPrimitive(rootResultItem.id.toString()))
316316
put("title", JsonPrimitive(rootResultItem.title))
317-
put("role", JsonPrimitive(rootResultItem.role.name.lowercase()))
317+
put("role", JsonPrimitive(rootResultItem.role.toJsonString()))
318318
put("depth", JsonPrimitive(rootResultItem.depth))
319319
rootResultItem.tags?.let { put("tags", JsonPrimitive(it)) }
320320
// Add expectedNotes from schema
@@ -342,7 +342,7 @@ Atomically create a hierarchical work tree: root item, child items, dependencies
342342
put("ref", JsonPrimitive(ref))
343343
put("id", JsonPrimitive(item.id.toString()))
344344
put("title", JsonPrimitive(item.title))
345-
put("role", JsonPrimitive(item.role.name.lowercase()))
345+
put("role", JsonPrimitive(item.role.toJsonString()))
346346
put("depth", JsonPrimitive(item.depth))
347347
item.tags?.let { put("tags", JsonPrimitive(it)) }
348348
// Add expectedNotes from schema

‎current/src/main/kotlin/io/github/jpicklyk/mcptask/current/application/tools/dependency/QueryDependenciesTool.kt‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -179,8 +179,8 @@ Returns dependencies with counts breakdown and optional graph traversal data.
179179
val item = fromResult.data
180180
put("fromItem", buildJsonObject {
181181
put("title", JsonPrimitive(item.title))
182-
put("role", JsonPrimitive(item.role.name.lowercase()))
183-
put("priority", JsonPrimitive(item.priority.name.lowercase()))
182+
put("role", JsonPrimitive(item.role.toJsonString()))
183+
put("priority", JsonPrimitive(item.priority.toJsonString()))
184184
})
185185
}
186186
is Result.Error -> { /* item may have been deleted; skip */ }
@@ -192,8 +192,8 @@ Returns dependencies with counts breakdown and optional graph traversal data.
192192
val item = toResult.data
193193
put("toItem", buildJsonObject {
194194
put("title", JsonPrimitive(item.title))
195-
put("role", JsonPrimitive(item.role.name.lowercase()))
196-
put("priority", JsonPrimitive(item.priority.name.lowercase()))
195+
put("role", JsonPrimitive(item.role.toJsonString()))
196+
put("priority", JsonPrimitive(item.priority.toJsonString()))
197197
})
198198
}
199199
is Result.Error -> { /* item may have been deleted; skip */ }

0 commit comments

Comments
 (0)