Pre-push review: T-1613

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex 1089477..0948b98 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-1613: MCP `query_tasks` now returns the exact tool error `Failed to fetch comments: <error>` when comment serialization cannot read storage, rather than reporting a successful task with `comments: []`. The detailed display-ID and task-list paths share this throwing serialization boundary; genuine empty comment collections retain their successful `comments: []` response. `update_task_status` continues serializing the `Comment` returned by its atomic mutation directly (T-1823), so it performs no post-commit comment fetch that could prompt a retry or duplicate a persisted comment. Deterministic MCP regressions cover both query errors, legitimate empty comments, and zero status-response fetches. - 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. - T-1607: Visual App Entity project/task queries and task-creation result resolution now rethrow storage fetch failures through their existing `async throws` contract instead of returning successful empty arrays. Deterministic regression coverage spans `entities(for:)` and `suggestedEntities()` for all three query types, while valid empty/invalid identifier results, ordering, suggestion limits, and partial relationship skipping remain unchanged. - 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.

diff --git a/Transit/Transit/MCP/MCPToolHandler.swift b/Transit/Transit/MCP/MCPToolHandler.swiftindex 05b700e..65ff3ef 100644--- a/Transit/Transit/MCP/MCPToolHandler.swift+++ b/Transit/Transit/MCP/MCPToolHandler.swift@@ -19,6 +19,7 @@ final class MCPToolHandler {     private let taskFetcher: any TaskFetching     private let projectService: ProjectService     private let commentService: CommentService+    private let commentFetcher: any CommentFetching     private let milestoneService: MilestoneService     private let milestoneFetcher: any MilestoneFetching     private let milestoneDisplayIDFinder: any MilestoneDisplayIDFinding@@ -47,6 +48,7 @@ final class MCPToolHandler {         settings: MCPSettings,         persistence: PersistenceAvailability = .shared,         taskFetcher: (any TaskFetching)? = nil,+        commentFetcher: (any CommentFetching)? = nil,         milestoneFetcher: (any MilestoneFetching)? = nil,         milestoneDisplayIDFinder: (any MilestoneDisplayIDFinding)? = nil     ) {@@ -54,6 +56,7 @@ final class MCPToolHandler {         self.taskFetcher = taskFetcher ?? taskService         self.projectService = projectService         self.commentService = commentService+        self.commentFetcher = commentFetcher ?? commentService         self.milestoneService = milestoneService         self.milestoneFetcher = milestoneFetcher ?? milestoneService         self.milestoneDisplayIDFinder = milestoneDisplayIDFinder ?? milestoneService@@ -677,7 +680,12 @@ final class MCPToolHandler {          let filtered = allTasks.filter { filters.matches($0) }         let isoFormatter = ISO8601DateFormatter()-        let results = filtered.map { taskToDict($0, formatter: isoFormatter) }+        let results: [[String: Any]]+        do {+            results = try filtered.map { try taskToDict($0, formatter: isoFormatter) }+        } catch {+            return errorResult("Failed to fetch comments: \(error)")+        }         return textResult(IntentHelpers.encodeJSONArray(results))     } @@ -702,7 +710,12 @@ final class MCPToolHandler {         }          let isoFormatter = ISO8601DateFormatter()-        let dict = taskToDict(task, formatter: isoFormatter, detailed: true)+        let dict: [String: Any]+        do {+            dict = try taskToDict(task, formatter: isoFormatter, detailed: true)+        } catch {+            return errorResult("Failed to fetch comments: \(error)")+        }         return textResult(IntentHelpers.encodeJSONArray([dict]))     } @@ -1448,9 +1461,9 @@ extension MCPToolHandler {      private func taskToDict(         _ task: TransitTask, formatter: ISO8601DateFormatter, detailed: Bool = false-    ) -> [String: Any] {+    ) throws -> [String: Any] {         var dict = IntentHelpers.taskToDict(task, formatter: formatter, detailed: detailed)-        let comments = (try? commentService.fetchComments(for: task.id)) ?? []+        let comments = try commentFetcher.fetchComments(for: task.id)         dict["comments"] = comments.map { [             "id": $0.id.uuidString, "authorName": $0.authorName, "content": $0.content,             "isAgent": $0.isAgent, "creationDate": formatter.string(from: $0.creationDate)

diff --git a/Transit/Transit/Services/CommentService.swift b/Transit/Transit/Services/CommentService.swiftindex 11a4a9c..533491e 100644--- a/Transit/Transit/Services/CommentService.swift+++ b/Transit/Transit/Services/CommentService.swift@@ -1,8 +1,13 @@ import Foundation import SwiftData +@MainActor+protocol CommentFetching {+    func fetchComments(for taskID: UUID) throws -> [Comment]+}+ @MainActor @Observable-final class CommentService {+final class CommentService: CommentFetching {      enum Error: Swift.Error, Equatable {         case emptyContent

diff --git a/Transit/TransitTests/MCPCommentFetchFailureTests.swift b/Transit/TransitTests/MCPCommentFetchFailureTests.swiftnew file mode 100644index 0000000..c305826--- /dev/null+++ b/Transit/TransitTests/MCPCommentFetchFailureTests.swift@@ -0,0 +1,109 @@+#if os(macOS)+import Foundation+import SwiftData+import Testing+@testable import Transit++/// Regression tests for T-1613: MCP must not turn an unreadable comment store+/// into a successful response with missing or empty comment details.+@MainActor @Suite(.serialized)+struct MCPCommentFetchFailureTests {++    private struct CommentFetchFailure: Swift.Error, CustomStringConvertible {+        var description: String { "simulated comment fetch failure" }+    }++    private final class FailingCommentFetcher: CommentFetching {+        private(set) var fetchCallCount = 0++        func fetchComments(for taskID: UUID) throws -> [Transit.Comment] {+            fetchCallCount += 1+            throw CommentFetchFailure()+        }+    }++    @Test func queryDetailedTaskCommentFetchFailureReturnsExactErrorInsteadOfEmptyComments() async throws {+        let failingFetcher = FailingCommentFetcher()+        let env = try MCPTestHelpers.makeEnv(commentFetcher: failingFetcher)+        let project = MCPTestHelpers.makeProject(in: env.context)+        let task = try await env.taskService.createTask(+            name: "Task", description: nil, type: .feature, project: project+        )+        let displayId = try #require(task.permanentDisplayId)++        let response = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "query_tasks", arguments: ["displayId": displayId]+        ))++        #expect(try MCPTestHelpers.isError(response))+        #expect(+            try MCPTestHelpers.errorText(response)+                == "Failed to fetch comments: simulated comment fetch failure"+        )+    }++    @Test func queryTaskListCommentFetchFailureReturnsExactErrorInsteadOfEmptyComments() async throws {+        let failingFetcher = FailingCommentFetcher()+        let env = try MCPTestHelpers.makeEnv(commentFetcher: failingFetcher)+        let project = MCPTestHelpers.makeProject(in: env.context)+        _ = try await env.taskService.createTask(+            name: "Task", description: nil, type: .feature, project: project+        )++        let response = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "query_tasks", arguments: [:]+        ))++        #expect(try MCPTestHelpers.isError(response))+        #expect(+            try MCPTestHelpers.errorText(response)+                == "Failed to fetch comments: simulated comment fetch failure"+        )+    }++    @Test func queryTaskWithValidEmptyCommentsReturnsSuccessfulEmptyArray() async throws {+        let env = try MCPTestHelpers.makeEnv()+        let project = MCPTestHelpers.makeProject(in: env.context)+        let task = try await env.taskService.createTask(+            name: "Task", description: nil, type: .feature, project: project+        )+        let displayId = try #require(task.permanentDisplayId)++        let response = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "query_tasks", arguments: ["displayId": displayId]+        ))++        #expect(try !MCPTestHelpers.isError(response))+        let result = try #require(MCPTestHelpers.decodeArrayResult(response).first)+        #expect((result["comments"] as? [[String: Any]])?.isEmpty == true)+    }++    @Test func updateStatusWithCommentDoesNotFetchResponseDetailsAfterPersisting() async throws {+        let failingFetcher = FailingCommentFetcher()+        let env = try MCPTestHelpers.makeEnv(commentFetcher: failingFetcher)+        let project = MCPTestHelpers.makeProject(in: env.context)+        let task = try await env.taskService.createTask(+            name: "Task", description: nil, type: .feature, project: project+        )+        let displayId = try #require(task.permanentDisplayId)++        let response = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "update_task_status",+            arguments: [+                "displayId": displayId, "status": "planning",+                "comment": "Persisted exactly once", "authorName": "TestBot"+            ]+        ))++        let result = try MCPTestHelpers.decodeResult(response)+        #expect(result["status"] as? String == "planning")+        #expect((result["comment"] as? [String: Any])?["content"] as? String == "Persisted exactly once")+        #expect(failingFetcher.fetchCallCount == 0)++        let comments = try env.commentService.fetchComments(for: task.id)+        #expect(comments.count == 1)+        #expect(comments.first?.content == "Persisted exactly once")+    }+}++#endif

diff --git a/Transit/TransitTests/MCPTestHelpers.swift b/Transit/TransitTests/MCPTestHelpers.swiftindex d8705ad..1938007 100644--- a/Transit/TransitTests/MCPTestHelpers.swift+++ b/Transit/TransitTests/MCPTestHelpers.swift@@ -23,6 +23,7 @@ enum MCPTestHelpers {         taskStatusSave: @escaping (ModelContext) throws -> Void = { try $0.save() },         projectFetcher: (any ModelFetching)? = nil,         taskFetcher: (any TaskFetching)? = nil,+        commentFetcher: (any CommentFetching)? = nil,         milestoneFetcher: (any MilestoneFetching)? = nil,         milestoneDisplayIDFinder: (any MilestoneDisplayIDFinding)? = nil     ) throws -> MCPTestEnv {@@ -54,7 +55,8 @@ enum MCPTestHelpers {             taskService: taskService, projectService: projectService,             commentService: commentService, milestoneService: milestoneService,             maintenanceService: maintenanceService, settings: mcpSettings,-            taskFetcher: taskFetcher, milestoneFetcher: milestoneFetcher,+            taskFetcher: taskFetcher, commentFetcher: commentFetcher,+            milestoneFetcher: milestoneFetcher,             milestoneDisplayIDFinder: milestoneDisplayIDFinder         )         return MCPTestEnv(

diff --git a/specs/bugfixes/mcp-comment-fetch-failures/report.md b/specs/bugfixes/mcp-comment-fetch-failures/report.mdnew file mode 100644index 0000000..d9fd6bb--- /dev/null+++ b/specs/bugfixes/mcp-comment-fetch-failures/report.md@@ -0,0 +1,86 @@+# Bugfix Report: mcp-comment-fetch-failures++**Date:** 2026-08-04+**Status:** Fixed++## Description of the Issue++`query_tasks` serializes comments with `(try? commentService.fetchComments(for: task.id)) ?? []`. A SwiftData read failure therefore appears to MCP clients as a successful task query with `comments: []`, indistinguishable from a task with no comments.++**Reproduction steps:**+1. Configure the MCP handler with a deterministic `CommentService.fetchComments` failure.+2. Query an existing task through `query_tasks` (both detailed display-ID and list paths).+3. Observe a successful response with an empty `comments` array rather than a tool error.++**Impact:** Agents can miss discussion and audit history and incorrectly treat an unreadable store as a valid empty result.++## Investigation Summary++- **Symptoms examined:** Swallowed comment-fetch errors become an empty comments collection in task responses.+- **Code inspected:** `MCPToolHandler.taskToDict`, both `query_tasks` response paths, `CommentService.fetchComments`, status-comment serialization, and MCP regression conventions.+- **Hypotheses tested:** The historical `update_task_status` fetch-and-append path was checked. T-1823 already replaced it with direct serialization of the `Comment` returned by the atomic mutation, so it has no post-commit fetch failure to mask.++## Discovered Root Cause++**Defect type:** Error handling / data-flow error.++**Why it occurred:** `taskToDict` catches all `fetchComments` errors with `try?` and substitutes `[]`, losing the distinction between a valid empty relationship query and storage failure. Both `query_tasks` response paths call this helper.++**Contributing factors:** The status path had historically shared this weakness, but its later direct-result design correctly avoids response-enrichment reads after persistence.++## Resolution for the Issue++**Changes made:**+- `Transit/Transit/Services/CommentService.swift` — Introduced the narrow `CommentFetching` protocol while retaining `CommentService` as its production implementation.+- `Transit/Transit/MCP/MCPToolHandler.swift` — Injects the comment reader and makes task serialization throw. Both detailed display-ID and list responses catch that failure at the MCP boundary and return the established `isError` tool result `Failed to fetch comments: <error>`.+- `Transit/TransitTests/MCPTestHelpers.swift` — Threads an optional comment reader into the handler test environment.+- `Transit/TransitTests/MCPCommentFetchFailureTests.swift` — Adds deterministic error, valid-empty, and status-mutation safety coverage.++**Approach rationale:** A throwing read boundary preserves the semantic difference between an unreadable store and a task that genuinely has no comments, while retaining the existing successful JSON response shapes. The status path intentionally continues to serialize `TaskService.updateStatus`'s returned `Comment` directly. The mutation has already committed before any hypothetical enrichment read; avoiding that read prevents an error response that could encourage callers to repeat a successful status/comment operation.++**Alternatives considered:**+- Reintroduce a post-commit comment fetch for `update_task_status` and return an error when it fails — rejected because T-1823 already removed that stale-result-prone design, and a post-persist error would give clients an ambiguous retry signal.+- Return `comments: []` or omit `comments` after a query failure — rejected because either response is indistinguishable from valid empty data.++## Regression Test++**Test file:** `Transit/TransitTests/MCPCommentFetchFailureTests.swift`++**Test names:**+- `queryDetailedTaskCommentFetchFailureReturnsExactErrorInsteadOfEmptyComments`+- `queryTaskListCommentFetchFailureReturnsExactErrorInsteadOfEmptyComments`+- `queryTaskWithValidEmptyCommentsReturnsSuccessfulEmptyArray`+- `updateStatusWithCommentDoesNotFetchResponseDetailsAfterPersisting`++**What they verify:** A deterministic comment-fetch failure produces the exact MCP tool error on both query response paths; a genuine empty comments result remains successful; status-plus-comment returns the persisted comment without a post-commit fetch or duplicate creation.++**Run command:** `make test-quick`++## Affected Files++| File | Change |+|------|--------|+| `Transit/Transit/MCP/MCPToolHandler.swift` | Propagate comment-read failures from task serialization. |+| `Transit/Transit/Services/CommentService.swift` | Provide the narrow injectable comment-read protocol. |+| `Transit/TransitTests/MCPTestHelpers.swift` | Allow deterministic comment-fetch seam injection. |+| `Transit/TransitTests/MCPCommentFetchFailureTests.swift` | Cover error, valid-empty, and mutation-safety behavior. |+| `CHANGELOG.md` | Record the MCP error-propagation behavior. |++## Verification++**Automated:**+- [x] Regression tests pass — `make test-quick` (macOS result bundle: 1,720 passed, 0 failed)+- [x] Full macOS unit suite passes — `make test-quick`+- [x] Linters/validators pass — `make lint`++**Manual verification:** The deterministic failing reader returns the exact MCP tool error on both query response paths; a real empty comment relationship returns successful `comments: []`; a failing reader is never called by a status-plus-comment response, which still persists and returns exactly one comment.++## Prevention++- Do not use `try?` with a value fallback where storage failure is semantically different from a valid empty result.+- Prefer serializing the object returned by an atomic mutation over refetching it for response enrichment.++## Related++- Transit ticket: T-1613+- T-1823: direct status-comment serialization removed the historical post-mutation fetch path.