Pre-push review: PR #201

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex ce9dfa5..9b26644 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -8,6 +8,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).  ### Fixed +- MCP `update_task_status` now returns the exact comment created by its atomic status-plus-comment mutation (T-1823). `TaskService.updateStatus` propagates the created `Comment` to the handler for direct serialization instead of refetching every task comment and taking the last `creationDate`; a future-dated existing or concurrently imported comment can no longer replace the mutation result. If the atomic save fails, the newly inserted comment is deleted before the task state is rolled back so a later save cannot resurrect it. Regressions verify exact UUID propagation, future-clock-skew behavior, and service/MCP failure recovery.+ - MCP `initialize` now requires object params with string `protocolVersion`, object `capabilities`, and typed `clientInfo` identity fields (T-1778). Missing or malformed method parameters return JSON-RPC `-32602 Invalid Params`. JSON-RPC request decoding now rejects scalar/null top-level `params` server-wide with `-32600 Invalid Request`. Supported `2025-03-26` requests are echoed and unsupported versions receive the server's latest advertised supported version. Focused handshake tests cover absent params/fields, wrong types, malformed client information, request-shape errors, and both negotiation paths. - MCP server start, stop, and same-port restart now run through one async desired-state lifecycle coordinator (T-1826). Each active listener owns its Hummingbird `ServiceGroup` and run task; teardown explicitly triggers graceful shutdown and awaits the group before rebinding, rather than assuming cancellation completion releases the socket. Graceful shutdown is time-bounded so a stalled request cannot wedge the coordinator, and the group installs no SIGTERM/SIGINT traps (`runService()`'s default) because those change the process's signal disposition permanently. Requests arriving during teardown replace the pending target so a burst converges on the latest state, Settings uses an explicit restart operation, and live loopback regressions cover stop, 20 serialized same-port repetitions, different-port restart, rapid off/on, an occupied port surfacing the bind failure, an invalid port releasing the running listener, bounded shutdown escalation, empty signal configuration, and reusable bind-probe behavior. - Already-cancelled task and milestone creates now stop before an uncontended display-ID allocation, and cancellation during a successful non-cooperative allocation is re-checked before SwiftData insertion (T-1765). `AllocationGate` checks cancellation once it holds the lock and before running the allocation body, while both create services guard their persistence boundary; existing selective `insertOrDelete` save-failure cleanup is unchanged. The gate check is shared by every allocator caller, so a cancelled display-ID promotion or duplicate-cleanup pass now stops at the current record and retries on the next pass. If the counter advance already succeeded, cancellation may leave a harmless gap in the display-ID sequence rather than persisting a ghost record. Four deterministic regressions cover the new timing windows across tasks and milestones, and the two retained queued-cancellation cases now prove that their contender entered and was removed from the gate queue.

diff --git a/Transit/Transit/MCP/MCPToolHandler.swift b/Transit/Transit/MCP/MCPToolHandler.swiftindex 636914b..a5a5503 100644--- a/Transit/Transit/MCP/MCPToolHandler.swift+++ b/Transit/Transit/MCP/MCPToolHandler.swift@@ -462,14 +462,14 @@ final class MCPToolHandler {         }         let commentText = args["comment"] as? String         let commentAuthor = args["authorName"] as? String-        let (commentError, hasComment) = validateCommentArgs(comment: commentText, author: commentAuthor)-        if let commentError {+        if let commentError = validateCommentArgs(comment: commentText, author: commentAuthor) {             return commentError         }          let previousStatus = task.statusRawValue+        let createdComment: Comment?         do {-            try taskService.updateStatus(+            createdComment = try taskService.updateStatus(                 task: task, to: newStatus,                 comment: commentText, commentAuthor: commentAuthor, commentService: commentService             )@@ -478,7 +478,7 @@ final class MCPToolHandler {         }          var response = statusResponse(task: task, previousStatus: previousStatus, newStatus: newStatus)-        appendCommentDetails(to: &response, taskID: task.id, hasComment: hasComment)+        response["comment"] = createdComment.map(commentResponse)         return textResult(IntentHelpers.encodeJSON(response))     } @@ -1137,26 +1137,19 @@ extension MCPToolHandler {             return errorResult("Failed to add comment: \(error)")         } -        let isoFormatter = ISO8601DateFormatter()-        let response: [String: Any] = [-            "id": comment.id.uuidString,-            "authorName": comment.authorName,-            "content": comment.content,-            "creationDate": isoFormatter.string(from: comment.creationDate)-        ]-        return textResult(IntentHelpers.encodeJSON(response))+        return textResult(IntentHelpers.encodeJSON(commentResponse(comment)))     }      // MARK: - Helpers -    private func validateCommentArgs(comment: String?, author: String?) -> (MCPToolResult?, Bool) {+    private func validateCommentArgs(comment: String?, author: String?) -> MCPToolResult? {         guard let comment, !comment.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else {-            return (nil, false)+            return nil         }         guard let author, !author.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else {-            return (errorResult("authorName is required when comment is provided"), false)+            return errorResult("authorName is required when comment is provided")         }-        return (nil, true)+        return nil     }      private func statusResponse(@@ -1169,14 +1162,14 @@ extension MCPToolHandler {         return res     } -    private func appendCommentDetails(to response: inout [String: Any], taskID: UUID, hasComment: Bool) {-        guard hasComment,-              let last = (try? commentService.fetchComments(for: taskID))?.last else { return }-        let fmt = ISO8601DateFormatter()-        response["comment"] = [-            "id": last.id.uuidString, "authorName": last.authorName,-            "content": last.content, "creationDate": fmt.string(from: last.creationDate)-        ] as [String: Any]+    private func commentResponse(_ comment: Comment) -> [String: Any] {+        let formatter = ISO8601DateFormatter()+        return [+            "id": comment.id.uuidString,+            "authorName": comment.authorName,+            "content": comment.content,+            "creationDate": formatter.string(from: comment.creationDate)+        ]     }      enum ResolveError: Error {

diff --git a/Transit/Transit/Services/TaskService.swift b/Transit/Transit/Services/TaskService.swiftindex b342d53..43714cc 100644--- a/Transit/Transit/Services/TaskService.swift+++ b/Transit/Transit/Services/TaskService.swift@@ -40,6 +40,7 @@ final class TaskService {     private let modelContext: ModelContext     private let displayIDAllocator: DisplayIDAllocator     private let createSave: (ModelContext) throws -> Void+    private let statusSave: (ModelContext) throws -> Void      /// Committed display IDs feeding the allocator's collision guard. Reads through     /// an injectable seam only so tests can simulate an unreadable store (T-1621);@@ -50,12 +51,14 @@ final class TaskService {         modelContext: ModelContext,         displayIDAllocator: DisplayIDAllocator,         fetcher: (any ModelFetching)? = nil,-        createSave: @escaping (ModelContext) throws -> Void = { try $0.save() }+        createSave: @escaping (ModelContext) throws -> Void = { try $0.save() },+        statusSave: @escaping (ModelContext) throws -> Void = { try $0.save() }     ) {         self.modelContext = modelContext         self.displayIDAllocator = displayIDAllocator         self.usedDisplayIDs = UsedDisplayIDs(fetcher ?? modelContext)         self.createSave = createSave+        self.statusSave = statusSave     }      // MARK: - Task Creation@@ -170,10 +173,11 @@ final class TaskService {      /// Transitions a task to a new status via StatusEngine.     /// When comment parameters are provided, creates a comment atomically-    /// in the same save operation.+    /// in the same save operation and returns that exact comment.     /// Same-status updates are treated as no-ops so callers can safely retry     /// or re-send the current status without mutating timestamps.     /// Comments are always persisted regardless of whether the status changed.+    @discardableResult     func updateStatus(         task: TransitTask,         to newStatus: TaskStatus,@@ -181,19 +185,19 @@ final class TaskService {         commentAuthor: String? = nil,         commentService: CommentService? = nil,         save: Bool = true-    ) throws {+    ) throws -> Comment? {         let statusChanged = task.status != newStatus         let hasComment = comment.map { !$0.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty } ?? false-        guard statusChanged || hasComment else { return }+        guard statusChanged || hasComment else { return nil } -        if statusChanged {-            StatusEngine.applyTransition(task: task, to: newStatus)-        }--        try modelContext.saveOrRollback(save: save) {+        var createdComment: Comment?+        do {+            if statusChanged {+                StatusEngine.applyTransition(task: task, to: newStatus)+            }             if let comment, !comment.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty,                let commentAuthor, let commentService {-                try commentService.addComment(+                createdComment = try commentService.addComment(                     to: task,                     content: comment,                     authorName: commentAuthor,@@ -201,7 +205,20 @@ final class TaskService {                     save: nil                 )             }+            if save {+                try statusSave(modelContext)+            }+        } catch {+            // A status update is an update, but its optional comment is a create.+            // Delete the inserted model before rolling back so a failed save cannot+            // leave a ghost comment that a later unrelated save would persist.+            if let createdComment {+                modelContext.delete(createdComment)+            }+            modelContext.safeRollback()+            throw error         }+        return createdComment     }      /// Moves a task to `.abandoned` status.

diff --git a/Transit/TransitTests/MCPTestHelpers.swift b/Transit/TransitTests/MCPTestHelpers.swiftindex ca2dba0..e52f479 100644--- a/Transit/TransitTests/MCPTestHelpers.swift+++ b/Transit/TransitTests/MCPTestHelpers.swift@@ -19,7 +19,8 @@ struct MCPTestEnv { enum MCPTestHelpers {      static func makeEnv(-        taskCreateSave: @escaping (ModelContext) throws -> Void = { try $0.save() }+        taskCreateSave: @escaping (ModelContext) throws -> Void = { try $0.save() },+        taskStatusSave: @escaping (ModelContext) throws -> Void = { try $0.save() }     ) throws -> MCPTestEnv {         let testContainer = try TestModelContainer()         let context = testContainer.context@@ -28,7 +29,8 @@ enum MCPTestHelpers {         let taskService = TaskService(             modelContext: context,             displayIDAllocator: taskAllocator,-            createSave: taskCreateSave+            createSave: taskCreateSave,+            statusSave: taskStatusSave         )         let projectService = ProjectService(modelContext: context)         let commentService = CommentService(modelContext: context)

diff --git a/Transit/TransitTests/MCPToolHandlerCommentTests.swift b/Transit/TransitTests/MCPToolHandlerCommentTests.swiftindex e4bb914..f2f0a29 100644--- a/Transit/TransitTests/MCPToolHandlerCommentTests.swift+++ b/Transit/TransitTests/MCPToolHandlerCommentTests.swift@@ -40,6 +40,67 @@ struct MCPToolHandlerCommentTests {         #expect(comment["authorName"] as? String == "NewAgent")     } +    @Test func updateStatusWithCommentReturnsExactCreatedCommentWhenExistingCommentIsFutureDated() 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 futureComment = try env.commentService.addComment(+            to: task, content: "Future comment", authorName: "SkewedPeer", isAgent: true+        )+        futureComment.creationDate = Date.now.addingTimeInterval(60 * 60)+        try env.context.save()++        let response = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "update_task_status",+            arguments: [+                "displayId": displayId, "status": "planning",+                "comment": "Created by this call", "authorName": "CurrentAgent"+            ]+        ))++        let result = try MCPTestHelpers.decodeResult(response)+        let returnedComment = try #require(result["comment"] as? [String: Any])+        let comments = try env.commentService.fetchComments(for: task.id)+        let createdComment = try #require(comments.first { $0.content == "Created by this call" })++        #expect(comments.count == 2)+        #expect(returnedComment["id"] as? String == createdComment.id.uuidString)+        #expect(returnedComment["id"] as? String != futureComment.id.uuidString)+        #expect(returnedComment["content"] as? String == "Created by this call")+        #expect(returnedComment["authorName"] as? String == "CurrentAgent")+    }++    @Test func updateStatusWithCommentSaveFailureReturnsErrorAndRollsBack() async throws {+        let env = try MCPTestHelpers.makeEnv(+            taskStatusSave: { _ in throw SaveFailure.simulated }+        )+        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": "Must not survive", "authorName": "CurrentAgent"+            ]+        ))++        #expect(try MCPTestHelpers.isError(response))+        #expect(try MCPTestHelpers.errorText(response).contains("Status update failed"))+        #expect(task.status == .idea)+        #expect(try env.commentService.fetchComments(for: task.id).isEmpty)++        try env.context.save()+        #expect(try env.commentService.fetchComments(for: task.id).isEmpty)+    }+     @Test func updateStatusNoOpWithoutCommentOmitsCommentDetails() async throws {         let env = try MCPTestHelpers.makeEnv()         let project = MCPTestHelpers.makeProject(in: env.context)

diff --git a/Transit/TransitTests/TaskServiceStatusCommentFailureTests.swift b/Transit/TransitTests/TaskServiceStatusCommentFailureTests.swiftnew file mode 100644index 0000000..945602b--- /dev/null+++ b/Transit/TransitTests/TaskServiceStatusCommentFailureTests.swift@@ -0,0 +1,49 @@+import Foundation+import SwiftData+import Testing+@testable import Transit++@MainActor @Suite(.serialized)+struct TaskServiceStatusCommentFailureTests {++    @Test func saveFailureRollsBackStatusAndDeletesInsertedComment() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let service = TaskService(+            modelContext: context,+            displayIDAllocator: DisplayIDAllocator(store: InMemoryCounterStore()),+            statusSave: { _ in throw SaveFailure.simulated }+        )+        let commentService = CommentService(modelContext: context)+        let project = Project(+            name: "Test Project",+            description: nil,+            gitRepo: nil,+            colorHex: "#FF0000"+        )+        context.insert(project)+        let task = try await service.createTask(+            name: "Task", description: nil, type: .feature, project: project+        )++        do {+            _ = try service.updateStatus(+                task: task,+                to: .planning,+                comment: "Must not survive",+                commentAuthor: "Agent",+                commentService: commentService+            )+            Issue.record("Expected SaveFailure to be thrown")+        } catch is SaveFailure {+            // Expected+        }++        #expect(task.status == .idea)+        #expect(try commentService.fetchComments(for: task.id).isEmpty)++        // A later unrelated save must not resurrect the failed comment.+        try context.save()+        #expect(try commentService.fetchComments(for: task.id).isEmpty)+    }+}

diff --git a/Transit/TransitTests/TaskServiceTests.swift b/Transit/TransitTests/TaskServiceTests.swiftindex 47a1b63..552227c 100644--- a/Transit/TransitTests/TaskServiceTests.swift+++ b/Transit/TransitTests/TaskServiceTests.swift@@ -268,7 +268,7 @@ struct TaskServiceTests {             name: "Task", description: nil, type: .feature, project: project         ) -        try service.updateStatus(+        let returnedComment = try service.updateStatus(             task: task,             to: .planning,             comment: "Moving to planning",@@ -278,9 +278,12 @@ struct TaskServiceTests {          #expect(task.status == .planning)         let comments = try commentService.fetchComments(for: task.id)+        let persistedComment = try #require(comments.first)+        let exactReturnedComment = try #require(returnedComment)         #expect(comments.count == 1)-        #expect(comments.first?.content == "Moving to planning")-        #expect(comments.first?.authorName == "Agent")+        #expect(persistedComment.id == exactReturnedComment.id)+        #expect(persistedComment.content == "Moving to planning")+        #expect(persistedComment.authorName == "Agent")     }      @Test func updateStatusWithCommentSetsIsAgentTrue() async throws {

diff --git a/specs/bugfixes/update-task-status-wrong-comment/report.md b/specs/bugfixes/update-task-status-wrong-comment/report.mdnew file mode 100644index 0000000..6a78af4--- /dev/null+++ b/specs/bugfixes/update-task-status-wrong-comment/report.md@@ -0,0 +1,100 @@+# Bugfix Report: update_task_status can return the wrong comment++**Date:** 2026-08-02+**Status:** Fixed++## Description of the Issue++When MCP `update_task_status` atomically changes a status and creates a comment, its response can serialize a different comment already associated with the task. A pre-existing future-dated comment caused by peer-device clock skew, or a concurrently imported remote comment, can be returned instead of the comment created by the request.++**Reproduction steps:**+1. Create a task with an existing comment whose `creationDate` is in the future.+2. Call `update_task_status` with a new status, comment, and author.+3. Observe that the status and new comment persist, but the response's `comment` object identifies the future-dated comment.++**Impact:** MCP clients receive a false confirmation payload and may attribute the wrong comment ID, content, author, and timestamp to their mutation.++## Investigation Summary++- **Symptoms examined:** The mutation succeeds and persists the requested comment, but response identity depends on global comment ordering.+- **Code inspected:** `MCPToolHandler.handleUpdateStatus`, `MCPToolHandler.appendCommentDetails`, `TaskService.updateStatus`, `CommentService.addComment`, and MCP comment tests.+- **Hypotheses tested:** Comment creation itself is correct and atomic. The defect occurs only during response assembly, where the handler refetches every task comment and selects the latest by `creationDate`.++## Discovered Root Cause++`TaskService.updateStatus` calls `CommentService.addComment` but discards the returned `Comment`. `MCPToolHandler.appendCommentDetails` therefore tries to rediscover the created record by fetching all comments sorted by `creationDate` and taking `.last`. Creation time is not a mutation identity: clock skew or concurrent sync can place another record last.++**Defect type:** Data-flow and identity-selection logic error.++**Five Whys:**+1. Why is the wrong comment returned? The handler serializes the last sorted comment.+2. Why does it select the last comment? It does not receive the comment created by the mutation.+3. Why is that identity unavailable? `TaskService.updateStatus` discards `CommentService.addComment`'s return value.+4. Why was refetching considered sufficient? The implementation assumed the newest timestamp uniquely identifies the local mutation.+5. Why is that assumption invalid? Comment timestamps originate on devices with potentially skewed clocks and remote comments can arrive concurrently.++**Contributing factors:** `creationDate` is useful for presentation order but is neither unique nor causally tied to the current request.++## Resolution for the Issue++**Changes made:**+- `Transit/Transit/Services/TaskService.swift` — `updateStatus` now returns the optional `Comment` created inside its atomic save operation. Its save operation is injectable for failure coverage, and a failed save deletes the newly inserted comment before rolling back task state so a later save cannot persist a ghost record.+- `Transit/Transit/MCP/MCPToolHandler.swift` — `handleUpdateStatus` serializes that exact object; the timestamp-based `appendCommentDetails` refetch was removed, and both MCP comment mutation responses share one serializer.+- `Transit/TransitTests/TaskServiceTests.swift` — directly verifies the returned service object matches the persisted comment UUID.+- `Transit/TransitTests/TaskServiceStatusCommentFailureTests.swift` — verifies save failure restores status and permanently removes the insertion.+- `Transit/TransitTests/MCPToolHandlerCommentTests.swift` — verifies the future-dated comment cannot replace the response identity and that save failure produces an MCP error without persisting status or comment state.++**Approach rationale:** Mutation identity is known at creation time and should be propagated directly. Returning `Comment?` is minimal, preserves atomicity, and leaves existing callers source-compatible through `@discardableResult`. Explicit insertion cleanup is required in addition to rollback because SwiftData rollback does not reliably unregister newly inserted models.++**Alternatives considered:**+- Fetch by expected content/author after saving — rejected because those fields are not unique and concurrent comments may match.+- Fetch the latest comment by local timestamp or insertion order — rejected because neither is a reliable mutation identity across CloudKit peers.+- Generate a UUID in the MCP handler and pass it into comment creation — rejected as unnecessary API surface when `CommentService.addComment` already returns the created model.++## Regression Test++**Test file:** `Transit/TransitTests/MCPToolHandlerCommentTests.swift`+**Test name:** `updateStatusWithCommentReturnsExactCreatedCommentWhenExistingCommentIsFutureDated`++**What it verifies:** A future-dated existing comment cannot replace the exact comment created by `update_task_status` in the response payload.++**Run command:** `make test-quick`++## Affected Files++| File | Change |+|------|--------|+| `Transit/Transit/Services/TaskService.swift` | Return the exact comment and clean up the inserted model when an atomic save fails |+| `Transit/Transit/MCP/MCPToolHandler.swift` | Serialize the returned comment directly and share comment response serialization |+| `Transit/TransitTests/TaskServiceTests.swift` | Verify the returned service object has the persisted comment UUID |+| `Transit/TransitTests/TaskServiceStatusCommentFailureTests.swift` | Verify save-failure cleanup, status rollback, and no later ghost persistence |+| `Transit/TransitTests/MCPTestHelpers.swift` | Inject status-save behavior for MCP failure-path coverage |+| `Transit/TransitTests/MCPToolHandlerCommentTests.swift` | Cover future-dated identity selection and MCP save-failure behavior |+| `CHANGELOG.md` | Document the user-visible fix under Unreleased |+| `specs/bugfixes/update-task-status-wrong-comment/report.md` | Record the investigation, resolution, and verification |++## Verification++**Automated:**+- [x] Regression test fails before the fix (returned future-dated comment UUID differed from the created comment UUID)+- [x] Regression test passes after the fix as part of the macOS unit suite+- [x] `make test-quick` passes after adding the identity and save-failure regressions+- [x] `make test` passes after the final shared-service changes+- [x] `make lint` passes, including the SwiftData ownership guard and strict SwiftLint+- [ ] `make test-ui`: 15 passed and 6 unrelated UI navigation/data-maintenance tests failed; all six failed again in a targeted retry. The change set contains no UI production or UI test changes.++**Manual verification:**+- `CommentService.addComment` returns the model it inserts; its existing success test compares that UUID with the fetched record.+- `TaskService.updateStatus` now has a direct assertion that its returned UUID equals the persisted comment UUID.+- The MCP future-dated regression compares the response UUID with the newly persisted record and explicitly rejects the skewed peer UUID.+- Injected save failures prove both the service and MCP paths restore the original status, expose the error, remove the inserted comment, and do not resurrect it on a later save.++## Prevention++**Recommendations to avoid similar bugs:**+- Propagate mutation results directly across service boundaries instead of rediscovering them with ordering heuristics.+- Use timestamps for sorting only, not for record identity.++## Related++- Transit ticket: T-1823