Pre-push review: T-1608/bugfix-mcp-query-tasks-hides-milestone-fetch-failures

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex c26428e..c58759d 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).  - T-1803: `UpdateStatusIntent` now includes a missing requested `displayId` in its `TASK_NOT_FOUND` hint (`No task with displayId N`), while missing UUIDs retain the existing generic lookup hint and malformed identifiers, duplicate IDs, status validation, and atomic mutation behavior remain unchanged. ### Fixed+- T-1608: MCP `query_tasks` now surfaces a tool error rather than a false successful `[]` when either unscoped milestone-name resolution (`Failed to fetch milestones: <error>`) or `milestoneDisplayId` resolution (`Failed to look up milestone: <error>`) cannot read storage. It validates every filter shape before milestone resolution, so malformed filters return their validation error before any milestone lookup outcome. Valid no-match arrays, cross-project same-name aggregation, project-scoped milestone lookup and T-1938 ambiguity handling, response shape, and later task-fetch errors are unchanged; deterministic MCP coverage asserts both exact errors and the legitimate no-match response. - Display-ID collision guards now build candidate-blocking sets from committed task/milestone IDs fetched through a fresh transient `ModelContext`, unioned with live/pending registered main-context values and allocator-issued IDs (T-1939). The shared guard applies to task and milestone creation, provisional promotion, and duplicate repair, so a stale registered bystander can no longer hide a peer-synced committed ID. Regression coverage commits the peer value through an independent context, proves the receiving bystander remains clean and unrefreshed, and separately verifies unsaved IDs remain blocked. Either store view failing still fails closed; allocation serialization, cancellation, maintenance stale-loser probes, and selective save recovery are unchanged. - MCP `create_task` and JSON `CreateTaskIntent` now reject present non-object `metadata` values (string, array, number, boolean, or null) with field-specific errors before task insertion, while omitted metadata and T-723's tolerant handling of values inside valid objects remain unchanged (T-1991). - T-2036: `QueryTasksIntent` now treats empty `completionDate` and `lastStatusChangeDate` objects as valid no-op filters, including when both are present. Raw-object emptiness is preserved before Codable decoding, while nested nulls, malformed non-empty objects, strict dates, reversed ranges, open bounds, and relative precedence remain covered by regression tests.

diff --git a/Transit/Transit/MCP/MCPToolHandler.swift b/Transit/Transit/MCP/MCPToolHandler.swiftindex 751e15a..05b700e 100644--- a/Transit/Transit/MCP/MCPToolHandler.swift+++ b/Transit/Transit/MCP/MCPToolHandler.swift@@ -4,6 +4,13 @@ import SwiftData  // swiftlint:disable file_length +@MainActor+protocol MilestoneDisplayIDFinding {+    func findByDisplayID(_ displayId: Int) throws -> Milestone+}++extension MilestoneService: MilestoneDisplayIDFinding {}+ @MainActor // swiftlint:disable:next type_body_length final class MCPToolHandler {@@ -13,6 +20,8 @@ final class MCPToolHandler {     private let projectService: ProjectService     private let commentService: CommentService     private let milestoneService: MilestoneService+    private let milestoneFetcher: any MilestoneFetching+    private let milestoneDisplayIDFinder: any MilestoneDisplayIDFinding     private let maintenanceService: DisplayIDMaintenanceService     private let settings: MCPSettings     private let persistence: PersistenceAvailability@@ -37,13 +46,17 @@ final class MCPToolHandler {         maintenanceService: DisplayIDMaintenanceService,         settings: MCPSettings,         persistence: PersistenceAvailability = .shared,-        taskFetcher: (any TaskFetching)? = nil+        taskFetcher: (any TaskFetching)? = nil,+        milestoneFetcher: (any MilestoneFetching)? = nil,+        milestoneDisplayIDFinder: (any MilestoneDisplayIDFinding)? = nil     ) {         self.taskService = taskService         self.taskFetcher = taskFetcher ?? taskService         self.projectService = projectService         self.commentService = commentService         self.milestoneService = milestoneService+        self.milestoneFetcher = milestoneFetcher ?? milestoneService+        self.milestoneDisplayIDFinder = milestoneDisplayIDFinder ?? milestoneService         self.maintenanceService = maintenanceService         self.settings = settings         self.persistence = persistence@@ -546,8 +559,10 @@ final class MCPToolHandler {             }         } -        // Resolve milestone filter-        // Reject non-integer milestoneDisplayId when key is present [T-613]+        // Validate every remaining filter before resolving a milestone. A no-match or+        // storage failure must not let a malformed filter look like a valid empty result.+        // This also preserves validation when displayId would otherwise return early. [T-1608]+        // Reject non-integer milestoneDisplayId when key is present [T-613].         if args["milestoneDisplayId"] != nil, IntentHelpers.parseIntValue(args["milestoneDisplayId"]) == nil {             return errorResult("milestoneDisplayId must be an integer")         }@@ -557,19 +572,50 @@ final class MCPToolHandler {         if args["milestone"] != nil, !(args["milestone"] is String) {             return errorResult("milestone must be a string")         }+        // Validate enum filters before building MCPQueryFilters [T-732].+        if let error = validateEnumFilter(args, key: "status", type: TaskStatus.self) { return error }+        if let error = validateEnumFilter(args, key: "not_status", type: TaskStatus.self) { return error }+        // type is a single-value filter (schema declares a string enum; read back as+        // args["type"] as? String). Reject arrays so they aren't silently dropped. [T-1404]+        if let error = validateEnumFilter(args, key: "type", type: TaskType.self, allowArray: false) {+            return error+        }+        // priority is a multi-value filter (schema declares an array, mirroring status).+        if let error = validateEnumFilter(args, key: "priority", type: TaskPriority.self, allowArray: true) {+            return error+        }+        // Reject a present-but-non-boolean `unfinished` flag [T-1095]. A plain+        // `as? Bool` would silently coerce "true"/1/null to false, returning+        // done/abandoned tasks even though the caller requested unfinished-only.+        if let unfinishedArg = args["unfinished"], IntentHelpers.parseBoolValue(unfinishedArg) == nil {+            return errorResult("unfinished must be a boolean")+        }+        // Reject non-string `search` filter [T-1156]. A present non-string value must not be+        // silently dropped by `as? String`, which would broaden results instead of erroring.+        if args["search"] != nil, !(args["search"] is String) {+            return errorResult("search must be a string")+        }+        // Reject non-integer displayId before a milestone no-match or failure can return early [T-634].+        if args["displayId"] != nil, IntentHelpers.parseIntValue(args["displayId"]) == nil {+            return errorResult("displayId must be an integer")+        }++        // Resolve milestone filter.         var milestoneFilter: Set<UUID>?         if let milestoneDisplayId = IntentHelpers.parseIntValue(args["milestoneDisplayId"]) {             do {-                milestoneFilter = [try milestoneService.findByDisplayID(milestoneDisplayId).id]+                milestoneFilter = [try milestoneDisplayIDFinder.findByDisplayID(milestoneDisplayId).id]+            } catch MilestoneService.Error.milestoneNotFound {+                return textResult(IntentHelpers.encodeJSONArray([]))             } catch MilestoneService.Error.duplicateDisplayID {                 return errorResult("Duplicate milestone identifier detected for displayId \(milestoneDisplayId)")             } catch {-                return textResult(IntentHelpers.encodeJSONArray([]))+                return errorResult("Failed to look up milestone: \(error)")             }         } else if let milestoneName = args["milestone"] as? String,                   !milestoneName.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {             if let projectFilter {-                // Scoped to a single project — at most one milestone matches+                // Scoped to a single project — at most one milestone matches.                 guard let project = resolvedProject else {                     return textResult(IntentHelpers.encodeJSONArray([]))                 }@@ -586,8 +632,14 @@ final class MCPToolHandler {                     return errorResult("Failed to look up milestone: \(error)")                 }             } else {-                // No project filter — collect ALL milestones with this name across projects-                let allMilestones = (try? milestoneService.fetchAllMilestones()) ?? []+                // No project filter — collect ALL milestones with this name across projects.+                // A storage failure must remain distinct from a valid no-match response. [T-1608]+                let allMilestones: [Milestone]+                do {+                    allMilestones = try milestoneFetcher.fetchAllMilestones()+                } catch {+                    return errorResult("Failed to fetch milestones: \(error)")+                }                 let matchingIds = Set(                     allMilestones                         .filter {@@ -603,32 +655,6 @@ final class MCPToolHandler {             }         } -        // Validate enum filters before building MCPQueryFilters [T-732]-        if let error = validateEnumFilter(args, key: "status", type: TaskStatus.self) { return error }-        if let error = validateEnumFilter(args, key: "not_status", type: TaskStatus.self) { return error }-        // type is a single-value filter (schema declares a string enum; read back as-        // args["type"] as? String). Reject arrays so they aren't silently dropped. [T-1404]-        if let error = validateEnumFilter(args, key: "type", type: TaskType.self, allowArray: false) {-            return error-        }-        // priority is a multi-value filter (schema declares an array, mirroring status).-        if let error = validateEnumFilter(args, key: "priority", type: TaskPriority.self, allowArray: true) {-            return error-        }--        // Reject a present-but-non-boolean `unfinished` flag [T-1095]. A plain-        // `as? Bool` would silently coerce "true"/1/null to false, returning-        // done/abandoned tasks even though the caller requested unfinished-only.-        if let unfinishedArg = args["unfinished"], IntentHelpers.parseBoolValue(unfinishedArg) == nil {-            return errorResult("unfinished must be a boolean")-        }--        // Reject non-string `search` filter [T-1156]. A present non-string value must not be-        // silently dropped by `as? String`, which would broaden results instead of erroring.-        if args["search"] != nil, !(args["search"] is String) {-            return errorResult("search must be a string")-        }-         let search = (args["search"] as? String)?.trimmingCharacters(in: .whitespacesAndNewlines)         let filters = MCPQueryFilters.from(             args: args, type: args["type"] as? String, projectId: projectFilter,@@ -636,16 +662,12 @@ final class MCPToolHandler {             milestoneIds: milestoneFilter         ) -        // Single-task lookup by displayId — returns early with detailed response-        // Reject non-integer displayId when key is present [T-634]-        if args["displayId"] != nil {-            guard let displayId = IntentHelpers.parseIntValue(args["displayId"]) else {-                return errorResult("displayId must be an integer")-            }+        // Single-task lookup by displayId — returns early with detailed response.+        if let displayId = IntentHelpers.parseIntValue(args["displayId"]) {             return handleDisplayIdLookup(displayId, filters: filters)         } -        // Full-table query+        // Full-table query.         let allTasks: [TransitTask]         do {             allTasks = try taskFetcher.fetchAllTasks()

diff --git a/Transit/TransitTests/MCPQueryProjectNameTests.swift b/Transit/TransitTests/MCPQueryProjectNameTests.swiftindex 9bbe361..cd58b20 100644--- a/Transit/TransitTests/MCPQueryProjectNameTests.swift+++ b/Transit/TransitTests/MCPQueryProjectNameTests.swift@@ -9,6 +9,10 @@ struct MCPQueryProjectNameTests {      private struct FetchFailure: Swift.Error {} +    private struct MilestoneFetchFailure: Swift.Error, CustomStringConvertible {+        var description: String { "simulated milestone fetch failure" }+    }+     private struct FailingProjectFetcher: ModelFetching {         func fetch<T: PersistentModel>(_ descriptor: FetchDescriptor<T>) throws -> [T] {             throw FetchFailure()@@ -19,6 +23,14 @@ struct MCPQueryProjectNameTests {         func fetchAllTasks() throws -> [TransitTask] { throw FetchFailure() }     } +    private struct FailingMilestoneFetcher: MilestoneFetching {+        func fetchAllMilestones() throws -> [Milestone] { throw MilestoneFetchFailure() }+    }++    private struct FailingMilestoneDisplayIDFinder: MilestoneDisplayIDFinding {+        func findByDisplayID(_ displayId: Int) throws -> Milestone { throw MilestoneFetchFailure() }+    }+     @Test func queryByProjectNameReturnsMatchingTasks() async throws {         let env = try MCPTestHelpers.makeEnv()         let alpha = MCPTestHelpers.makeProject(in: env.context, name: "Alpha")@@ -128,6 +140,61 @@ struct MCPQueryProjectNameTests {         #expect(try MCPTestHelpers.errorText(response).hasPrefix("Failed to fetch tasks:"))     } +    @Test func queryMilestoneNameFetchFailureReturnsExactErrorInsteadOfEmptyArray() async throws {+        let env = try MCPTestHelpers.makeEnv(milestoneFetcher: FailingMilestoneFetcher())++        let response = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "query_tasks",+            arguments: ["milestone": "v1.0"]+        ))++        #expect(try MCPTestHelpers.isError(response), "A failed filter fetch must not look like no matches")+        #expect(+            try MCPTestHelpers.errorText(response)+                == "Failed to fetch milestones: simulated milestone fetch failure"+        )+    }++    @Test func queryUnscopedMilestoneNameWithNoMatchReturnsEmptyArray() async throws {+        let env = try MCPTestHelpers.makeEnv()+        let project = MCPTestHelpers.makeProject(in: env.context)+        _ = try await env.taskService.createTask(name: "Unrelated", description: nil, type: .bug, project: project)++        let response = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "query_tasks",+            arguments: ["milestone": "v9.0"]+        ))++        #expect(try MCPTestHelpers.decodeArrayResult(response).isEmpty)+    }++    @Test func queryMilestoneDisplayIDFetchFailureReturnsExactErrorInsteadOfEmptyArray() async throws {+        let env = try MCPTestHelpers.makeEnv(milestoneDisplayIDFinder: FailingMilestoneDisplayIDFinder())++        let response = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "query_tasks",+            arguments: ["milestoneDisplayId": 1]+        ))++        #expect(try MCPTestHelpers.isError(response), "A failed lookup must not look like no matches")+        #expect(+            try MCPTestHelpers.errorText(response)+                == "Failed to look up milestone: simulated milestone fetch failure"+        )+    }++    @Test func queryMilestoneFetchFailureDoesNotMaskMalformedStatus() async throws {+        let env = try MCPTestHelpers.makeEnv(milestoneFetcher: FailingMilestoneFetcher())++        let response = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "query_tasks",+            arguments: ["milestone": "v1.0", "status": "not-a-status"]+        ))++        #expect(try MCPTestHelpers.isError(response))+        #expect(try MCPTestHelpers.errorText(response).contains("Invalid status: not-a-status"))+    }+     @Test func queryByNonexistentProjectIdMatchesIntentProjectNotFound() async throws {         let env = try MCPTestHelpers.makeEnv()         let projectID = UUID()

diff --git a/Transit/TransitTests/MCPTestHelpers.swift b/Transit/TransitTests/MCPTestHelpers.swiftindex 4902e02..d8705ad 100644--- a/Transit/TransitTests/MCPTestHelpers.swift+++ b/Transit/TransitTests/MCPTestHelpers.swift@@ -22,7 +22,9 @@ enum MCPTestHelpers {         taskCreateSave: @escaping (ModelContext) throws -> Void = { try $0.save() },         taskStatusSave: @escaping (ModelContext) throws -> Void = { try $0.save() },         projectFetcher: (any ModelFetching)? = nil,-        taskFetcher: (any TaskFetching)? = nil+        taskFetcher: (any TaskFetching)? = nil,+        milestoneFetcher: (any MilestoneFetching)? = nil,+        milestoneDisplayIDFinder: (any MilestoneDisplayIDFinding)? = nil     ) throws -> MCPTestEnv {         let testContainer = try TestModelContainer()         let context = testContainer.context@@ -52,7 +54,8 @@ enum MCPTestHelpers {             taskService: taskService, projectService: projectService,             commentService: commentService, milestoneService: milestoneService,             maintenanceService: maintenanceService, settings: mcpSettings,-            taskFetcher: taskFetcher+            taskFetcher: taskFetcher, milestoneFetcher: milestoneFetcher,+            milestoneDisplayIDFinder: milestoneDisplayIDFinder         )         return MCPTestEnv(             handler: handler,

diff --git a/specs/bugfixes/mcp-query-tasks-milestone-fetch-failures/report.md b/specs/bugfixes/mcp-query-tasks-milestone-fetch-failures/report.mdnew file mode 100644index 0000000..506ea39--- /dev/null+++ b/specs/bugfixes/mcp-query-tasks-milestone-fetch-failures/report.md@@ -0,0 +1,91 @@+# Bugfix Report: MCP Query Tasks Milestone Fetch Failures++**Date:** 2026-08-03+**Status:** Fixed++## Description of the Issue++MCP `query_tasks` silently returned a successful empty array when a query supplied a milestone name without a project scope and SwiftData failed while fetching milestones.++**Reproduction steps:**+1. Call `query_tasks` with a non-empty `milestone` name and no `project` or `projectId`.+2. Make the full-table milestone fetch throw.+3. Observe `[]` instead of an MCP tool error.++**Impact:** MCP clients could not distinguish a failed filter evaluation from a legitimate query with no matching tasks.++## Investigation Summary++- **Symptoms examined:** The no-project milestone-name branch used `try? ... ?? []`; later task-fetch failures and `query_milestones` already returned tool errors.+- **Code inspected:** `MCPToolHandler.handleQueryTasks`, `MCPQueryFilters`, `MilestoneService`, `MilestoneFetching`, MCP query tests, and all three `MCPToolHandler` construction callers.+- **Hypotheses tested:** The failure was isolated to the no-project milestone-name fetch. Project-scoped lookup, no-match results, cross-project ID aggregation, T-1938 ambiguity handling, malformed-input order, response serialization, and later task fetch failures use separate existing paths.++## Discovered Root Cause++**Defect type:** Error-handling logic error.++The no-project milestone-name branch converted a thrown `fetchAllMilestones()` error into an empty list with `try?`. Its empty-ID branch then returned `[]`, the same successful response used for a legitimate no-match.++## Resolution for the Issue++**Changes made:**+- `Transit/Transit/MCP/MCPToolHandler.swift` — injects read seams for unscoped milestone names and display IDs, returns `Failed to fetch milestones: <error>` for failed unscoped-name reads, and returns `Failed to look up milestone: <error>` for unexpected display-ID lookup failures while retaining the normal no-match and duplicate-ID outcomes.+- `Transit/TransitTests/MCPTestHelpers.swift` — accepts both deterministic milestone read seams.+- `Transit/TransitTests/MCPQueryProjectNameTests.swift` — adds exact-error, no-false-success, malformed-filter precedence, and legitimate no-match regression coverage.++**Approach rationale:** The handler now follows the explicit storage-error contract used by `query_milestones` and the later `query_tasks` task fetch. It preserves valid no-match responses, cross-project aggregation, and scoped lookup behavior while making malformed filters fail before milestone resolution.++**Alternatives considered:**+- Keep `try?` and add logging — rejected because callers still receive an indistinguishable successful `[]`.+- Change shared service behavior — rejected because the defect is MCP response translation, and the existing service/API contract already throws.++## Independent Review Follow-up++The PR review found two adjacent paths that could still hide or mis-prioritize failures:++- `milestoneDisplayId` lookup now preserves its legitimate `milestoneNotFound` empty result and duplicate-ID error, while surfacing unexpected storage failures through a narrow injected `MilestoneDisplayIDFinding` seam.+- Every remaining `query_tasks` filter shape is validated before milestone resolution. This intentionally gives malformed filters precedence over milestone no-match, duplicate, or storage-failure outcomes; regression coverage proves an invalid status is not masked by a failing milestone fetch.++The follow-up coverage also directly proves that an unscoped milestone name with no matches still returns a successful empty array.++## Regression Test++**Test file:** `Transit/TransitTests/MCPQueryProjectNameTests.swift`+**Tests:**+- `queryMilestoneNameFetchFailureReturnsExactErrorInsteadOfEmptyArray`+- `queryMilestoneDisplayIDFetchFailureReturnsExactErrorInsteadOfEmptyArray`+- `queryUnscopedMilestoneNameWithNoMatchReturnsEmptyArray`+- `queryMilestoneFetchFailureDoesNotMaskMalformedStatus`++The deterministic seams verify `isError == true` with the exact name- and display-ID failure text, preserve a legitimate no-match empty array, and prove input validation is not masked by a failing milestone fetch. Before the original fix, the unscoped-name failure returned a successful `[]`.++**Run command:** `make test-quick`++## Affected Files++| File | Change |+|------|--------|+| `Transit/Transit/MCP/MCPToolHandler.swift` | Injected milestone read seams, surfaced name/display-ID storage failures, and validates filter shapes before milestone resolution. |+| `Transit/TransitTests/MCPTestHelpers.swift` | Passed deterministic milestone read seams into MCP handler tests. |+| `Transit/TransitTests/MCPQueryProjectNameTests.swift` | Added exact-error, no-false-success, no-match, and validation-order regression coverage. |+| `CHANGELOG.md` | Recorded the complete fixed MCP error and validation-order contract. |++## Verification++**Automated:**+- [x] Regression test fails before the fix (`make test-quick`)+- [x] Regression test passes after the fix (`make test-quick`)+- [x] `make test-quick` passes+- [x] `make lint` passes++**Manual verification:** Not required; the handler is exercised directly with an injected failing store seam.++## Prevention++Milestone-resolution paths must propagate unexpected storage errors as tool errors rather than recover with empty collections when an empty result is a valid response. Validate every filter shape before resolving milestones so lookup outcomes cannot mask malformed input.++## Related++- T-1608+- T-292 (cross-project same-name milestone aggregation)+- T-1938 (project-scoped milestone-name ambiguity handling)