Pre-push review: T-1768/bugfix-create-with-milestone-cleanup-is-not-atomic

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex 7870fe7..e80d88f 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -1,90 +1,92 @@ # Changelog  All notable changes to this project will be documented in this file.  The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).  ## [Unreleased]  ### Fixed +- MCP `create_task`, `CreateTaskIntent`, and `AddTaskSheet.persist` now create a task plus optional milestone atomically in one `TaskService` save (T-1768). The relationship is validated and attached before insertion; failed saves delete the pending aggregate instead of relying on a second, fallible compensating deletion. Surface-level failure regressions verify no task remains or is resurrected by a later save. `CreateTaskIntent` maps the merged save's failures per error kind, so a storage failure that was previously reported as `INTERNAL_ERROR` by the separate milestone-assignment step still reports `INTERNAL_ERROR` rather than collapsing into `INVALID_INPUT`; `milestoneProjectMismatch` from the service boundary surfaces as `MILESTONE_PROJECT_MISMATCH`.+ - SwiftData test fixtures now retain their backing `ModelContainer` for the full test lifetime (T-2003). `TestModelContainer` is a container-owning fixture instead of a bare-context factory, 207 context-acquisition call sites across 92 test files were migrated, and bespoke context helpers in task-entity, task-creation-result, and report tests now delegate to the shared fixture. An escaped-context lifetime regression plus an executable ownership guard with positive/negative fixtures prevent raw container/context factories from recurring.  - Cross-device milestone name conflicts no longer make name-based operations target an arbitrary record (T-1938). `MilestoneService.findByName` now throws on multiple project-scoped matches; all MCP and App Intent callers report the ambiguity (`AMBIGUOUS_MILESTONE` for intents). Launch/foreground/connectivity maintenance deterministically keeps the oldest milestone name and renames other UUID-distinct records with UUID-derived suffixes, preserving records and task assignments. Cross-context tests simulate CloudKit imports and verify ambiguity reporting, reconciliation, idempotence, and data preservation. - Task mutation surfaces no longer hide duplicate task display IDs behind a not-found error (T-1837). `TaskService.findByDisplayID` already threw `duplicateDisplayID` when several tasks share a `permanentDisplayId` (possible under CloudKit, which cannot enforce uniqueness), but MCP `update_task_status` / `update_task` / `add_comment`, `UpdateStatusIntent`, `IntentHelpers.resolveTask`, and the visual `AddCommentIntent` all collapsed it into `TASK_NOT_FOUND` / "Provide either displayId or taskId", so operators could not tell missing data from store corruption needing display-ID maintenance. Duplicates now report `INTERNAL_ERROR` with the hint `Duplicate task identifier for displayId N` (App Intents — matching the milestone paths and the T-1097 `QueryTasksIntent` fix) or an `isError` result reading `Duplicate task identifier detected for displayId N` (MCP — matching the milestone tools). MCP `query_tasks` uses the same wording instead of leaking `Lookup failed: duplicateDisplayID`. No new error code was introduced. The three MCP call sites now share `resolveTaskArgument`, `UpdateStatusIntent` routes through `IntentHelpers.resolveTask` instead of its own copy of the mapping, and `VisualIntentError` gains a `duplicateIdentifier` case (code `INTERNAL_ERROR`). Regression coverage in `DuplicateTaskDisplayIDIntentTests` and `MCPDuplicateTaskDisplayIDTests`. - The MCP `add_comment` handler now distinguishes a missing required field from a present-but-non-string one for `content` and `authorName` (T-1579). Previously `guard let value = args[key] as? String, !value.isEmpty` collapsed numbers, booleans, arrays, and null into `Missing required argument: content` / `Missing required argument: authorName`, misleading callers into thinking they omitted a field they actually supplied with the wrong type. A present non-string value now returns the field-specific `content must be a string` / `authorName must be a string` before any task resolution or comment creation, matching the hardened pattern used by `update_task_status` after T-1205. Extracted a `requiredString(_:key:)` helper to keep `handleAddComment` within the cyclomatic-complexity limit. Regression coverage added in `MCPAddCommentTypeValidationTests` (numeric/boolean/array/null `content` and `authorName`). - `IntentHelpers.resolveMilestone` now rejects a present-but-non-string `name` identifier (number, boolean, null, array) with `INVALID_INPUT` and the field-specific hint `name must be a string`, instead of treating the failed `as? String` cast as an absent identifier and returning the generic `Provide displayId, milestoneId, or name with project` hint (T-1572). This only affects the `UpdateMilestoneIntent` name+project identifier path; no update fields are applied when the identifier is malformed. The MCP `update_milestone` tool is unaffected (it does not support name-based identification). Regression tests added to `MilestoneRenameTypeValidationTests`. - The MCP `update_milestone` handler and `UpdateMilestoneIntent` now support clearing a milestone description by passing an empty or whitespace-only `description` string, mirroring `update_task` and the milestone edit UI (T-1555). Previously a present `description` was applied verbatim, so `""`/whitespace persisted an empty string and the MCP response kept emitting a `description` key. Non-empty values are now trimmed before storage, and a cleared description sets `milestoneDescription` to nil (the MCP `milestoneToDict` serializer already omits the key when nil). For cross-surface parity, `UpdateMilestoneIntent`'s response now also emits `description` when set and omits it when cleared (previously it never emitted the key). T-343 had fixed only the edit-UI path. Cross-surface regression tests added in `MCPMilestoneClearDescriptionTests` and `UpdateMilestoneClearDescIntentTests`. - MCP `update_task` apply/save failure messages are now uniformly worded `"Update failed: \(error)"` (T-650 phase 4). Previously the handler emitted three separate templates (`"Failed to save: ..."`, `"Failed to set milestone: ..."`, `"Failed to clear milestone: ..."`); none of these are referenced by existing tests, but agents matching the old strings will see new wording. Validation, identifier-resolution, and milestone error messages are unchanged. - `QueryTasksIntentTests` no longer fails to compile after T-1146 added a required `milestoneService:` argument to `QueryTasksIntent.execute`; the missing argument is now supplied at the one stale call site so the test target builds. - Milestone status updates are now idempotent: re-sending the current status no longer rewrites `lastStatusChangeDate` and `completionDate`, so old terminal milestones no longer re-enter the current report window via `ReportLogic`'s `completionDate ?? lastStatusChangeDate` fallback. Applied across `MilestoneService.updateStatus`, `UpdateMilestoneIntent.applyUpdate`, and `MCPToolHandler.applyMilestoneUpdate` (T-923). - `DisplayIDMaintenanceService.reassignDuplicates` now surfaces a fetch failure as a counter-advance warning on both record types instead of silently no-oping the run with empty arrays. - `DisplayIDMaintenanceService` group processing no longer fabricates winner identity (`UUID()` / `""`) for the unreachable empty-group case; the invariant is asserted via `preconditionFailure` and the existing `RecordRef` is propagated. - `IntentHelpers.resolveMilestone` now rejects malformed `milestoneId` and `projectId` with `INVALID_INPUT` instead of silently falling back to name-based lookup (T-753) - Sync toggle in Settings now updates `SyncManager` runtime state immediately instead of only taking effect on next launch (T-699)  ### Changed  - **`update_task` response shape changed (T-650)**. The new `IntentHelpers.taskUpdateResponseDict` builder (used by both the MCP tool and `UpdateTaskIntent`) no longer emits `comments`, `lastStatusChangeDate`, or `completionDate`, and now emits `description` and `metadata` only when non-empty (previously `description` was present as `null` and `metadata` was absent). The new shape is fixed by AC 9.1; clients parsing those removed keys from `update_task` responses must instead call `query_tasks` to fetch them. Identifier-resolution and error envelopes are unchanged. - `DisplayIDMaintenanceService` init no longer takes `taskCounterStore`/`milestoneCounterStore`; the stores are read from each `DisplayIDAllocator.counterStore` (single source of truth). - `IntentHelpers.encodeAsJSONString(_:)` is the shared JSON encoding helper used by `MCPToolHandler` and the maintenance App Intents, replacing three per-site copies of the same `JSONEncoder()` + UTF-8 guard. - `MCPToolHandler` now derives the gated maintenance tool name set from `MCPToolDefinitions.maintenanceToolNames` so the gate stays in sync with the definitions list. - Service-level error messages in `DisplayIDMaintenanceService` use `error.localizedDescription` rather than `"\(error)"` interpolation for more actionable output in the result envelope. - Decision 11 in the duplicate-displayid-cleanup decision log records the choice to use `FetchDescriptor` for the stale-ID guard (instead of `ModelContext.refresh(_:mergeChanges:)`) since the service hands `RecordRef` value types across the scan/reassign boundary rather than `@Model` references.  ### Added  - Effective-priority invariant cross-surface regression test (T-1463, Verification phase). `EffectivePriorityInvariantTests` creates a legacy task with `priorityRawValue = ""` (set directly, bypassing the `priority` setter) and asserts it reads/serializes as `medium` and is matched by a `medium` priority filter on all five read surfaces — board `DashboardLogic`, `MCPQueryFilters.matches`, `IntentHelpers.taskToDict`, `QueryTasksIntent` `applyFilters`, and `IntentHelpers.taskUpdateResponseDict` — plus the accessor itself. This is the guard against any surface reading `priorityRawValue` directly instead of the computed `priority` accessor and silently breaking Req 1.4. - Task priority in the app UI (T-1463, UI phase). The kanban dashboard gains a `PriorityFilterMenu` (multi-select high/medium/low, displayed high→medium→low, ephemeral state that resets on launch) wired into `DashboardView`'s filter row and `DashboardLogic` predicate, matching on the computed `task.priority` so legacy/unset tasks read as medium. Task cards show a new `PriorityIndicator` glyph for high/low only (medium suppressed, read from `TaskPriority.glyphSymbol`). The create sheet (`AddTaskSheet`) and edit screen (`TaskEditView`) get priority pickers (default medium / current value, both iOS and macOS layouts), and `TaskDetailView` shows a priority row. Adds `TaskPriority.displayOrder` (`[.high, .medium, .low]`) for consistent picker/filter ordering. `DashboardView`'s filter inputs were bundled into a `FilterSelection` value type as part of this. - Task priority across the MCP server and App Intents (T-1463, Automation phase). New dedicated `INVALID_PRIORITY` error code (its own `IntentError` case + hint, shared by both surfaces). `TaskUpdateValidator.validatePriority` mirrors `validateType` and threads an optional, non-clearable priority through `ValidatedTaskUpdate` (Decision 8). MCP `create_task` parses/defaults priority (invalid → error, no task created — Req 5.5) and echoes it; `query_tasks` filters by a multi-value `priority` array (mirrors `status`) via `MCPQueryFilters.priorities`; schemas updated in `MCPToolDefinitions`. App Intents: `CreateTaskIntent` validates optional-with-default priority (absent → medium, invalid → `INVALID_PRIORITY`) and echoes it in the response; `QueryTasksIntent` gains a scalar `priority` filter; `UpdateTaskIntent` sets priority via the validator. `IntentHelpers.taskToDict`/`taskUpdateResponseDict` (and the milestone task sub-dicts) serialize `task.priority.rawValue` — always the computed accessor, never `priorityRawValue`, upholding the effective-priority invariant (Req 1.4). The `createTaskIntentJsonContractRemainsCompatible` contract test was updated for the new `priority` key (existing keys unchanged, so callers ignoring new keys stay compatible). - Task priority foundation (T-1463, Foundation phase): new `TaskPriority` enum (low/medium/high) mirroring `TaskType` — `tintColor` for all three, `glyphSymbol` (`nil` only for medium, the single source of truth for "no card glyph"), and `accessibilityLabel`. `TransitTask` gains a CloudKit-safe `priorityRawValue` stored property (default `medium`, same shape as `statusRawValue`/`typeRawValue`) and a computed `priority` accessor that reads any absent/empty/unrecognized raw value as `.medium` (the effective-priority invariant, Req 1.4 — no migration needed). `TaskService.createTask` (both overloads) accepts a defaulted `priority`, and `updateTask` accepts an optional `priority` where omitting it leaves the value unchanged (Decision 8, non-clearable). Automation and UI wiring follow in later phases. - Spec for the task priority feature (T-1463): requirements, design, decision log, and a 20-task implementation plan written to `specs/task-priority/`, with `specs/OVERVIEW.md` updated. Plans a low/medium/high priority field (default medium) shown as a board-card glyph for high/low (medium suppressed), a board filter, in-app create/edit/detail editing, and MCP + App Intent read/set/filter support — using a default-value + computed-accessor backfill (no migration) and a dedicated `INVALID_PRIORITY` error code. Planning only; not yet implemented. - The dashboard now shows SwiftUI's `ContentUnavailableView.search` empty state ("No Results for '<query>'") when text search is the only active filter and no task matches (T-198). Project/type/milestone filters keep the existing "No matching tasks. Clear filters" message, and an empty database still shows "No tasks yet". The search bar stays visible so the query can be edited or cleared. The choice of empty state is a pure `DashboardLogic.emptyStateKind` function (empty-DB > search-only > generic-filter precedence) with unit-test coverage in `DashboardSearchTests` and UI coverage in `SearchEmptyStateUITests`. - Spec for adding a `ContentUnavailableView.search` empty state to the dashboard when text search is the only active filter and yields no matches (T-198). Includes a smolspec, decision log (search-only-vs-generic-filter precedence, empty-database precedence, and extracting empty-state selection into a pure `DashboardLogic.emptyStateKind` function for unit-testability), and a 4-task implementation plan with distributed testing. Deferred follow-up from the text-filter feature. - `docs/agent-notes/mcp-server.md` Tools Exposed table now describes the extended `update_task` (name / description / type / metadata + milestone) with a gotcha noting the omit-vs-clear distinction and the shared validator + response-builder pipeline both surfaces use (T-650 phase 7). - `UpdateTaskAllFieldsParityTests` (T-650 phase 6): 10 cross-surface parity cases (each new field individually, combined multi-field, no-op echo, milestone assign and clear) call both `MCPToolHandler.handleUpdateTask` and `UpdateTaskIntent.execute` against a shared task fixture and assert byte-equal JSON responses. Verifies AC 8.1 in practice. All cases passed on the first run with zero production diffs — both surfaces route through `TaskUpdateValidator` + `IntentHelpers.taskUpdateResponseDict`, so structural parity is a property of the architecture rather than an emergent coincidence. - `UpdateTaskIntent` (App Intent) now accepts the same `name` / `description` / `type` / `metadata` updates as the MCP tool (T-650 phase 5). `execute()` rewritten as a thin orchestrator over `TaskUpdateValidator` + `IntentHelpers.taskUpdateResponseDict`, mirroring `MCPToolHandler.handleUpdateTask` structurally so success responses are byte-equivalent across surfaces (AC 8.1). The old `applyMilestoneChange` and `buildResponse` helpers are deleted. The `@Parameter` description enumerates every supported field including the empty-string-clears and `{}`-clears semantics. A preflight identifier guard returns `INVALID_INPUT` ("Provide either displayId or taskId") for payloads with no `taskId`/`displayId` — previously these landed as a misleading `TASK_NOT_FOUND` from the downstream resolver, including the pre-existing failing test `noTaskIdentifierReturnsInvalidInput`, which now passes. - MCP `update_task` now accepts name, description (empty/whitespace clears), type, and metadata (`{}` clears) alongside the existing milestone fields (T-650 phase 4). `MCPToolHandler.handleUpdateTask` rewritten as a thin orchestrator over `TaskUpdateValidator` + `IntentHelpers.taskUpdateResponseDict`. The two-stage do/catch (apply with explicit `taskService.rollback()`; save relying on `TaskService.save()`'s built-in `safeRollback`) keeps the task untouched when any field validation, apply, or save step fails — atomic per AC 5.1. Schema (`MCPToolDefinitions.updateTask`) extended with name/description/type/metadata properties; description and metadata prose include `"clear"` so clients know the clear semantics. Identifier-resolution preamble (T-634 / T-808) and milestone error literals are byte-equivalent to the pre-rewrite handler so existing tests pass unchanged. - `IntentHelpers.taskUpdateResponseDict` (T-650 phase 3): shared response builder for `update_task` across MCP and App Intent surfaces. Always emits `taskId`, `name`, `type`, `status`; emits `displayId`, `projectId`, `projectName`, `description`, `milestone` only when non-nil; emits `metadata` only when non-empty. Excludes `comments` and the date fields. 13 tests cover every conditional clause of AC 9.1. - `TaskUpdateValidator.validate` / `apply` / `strictStringMetadata` implementations (T-650 phase 2). `validate` parses raw MCP/Intent argument dicts into a `ValidatedTaskUpdate` covering name (trim + non-empty), description (trim, empty/whitespace clears), type (lowercase enum), metadata (strict-string, `{}` clears), and milestone (precedence `clearMilestone` → `milestoneId` → `milestoneDisplayId` → `milestone`). Milestone error literals are byte-identical to the existing MCP handler so Phase 3 wiring is drop-in. `apply` walks fields in deterministic order with `save: false`, throwing service-layer errors only — callers handle rollback via `TaskService.rollback()`. `TaskUpdateValidatorTests` covers 33 scenarios across all acceptance criteria 1–7. - `TaskUpdateValidator` scaffolding (T-650 phase 1): new `Transit/Transit/Intents/TaskUpdateValidator.swift` declares the `FieldChange<T>`, `MilestoneAction`, `ValidatedTaskUpdate`, and `TaskUpdateValidationError` value types plus stub `validate`/`apply`/`strictStringMetadata` signatures so subsequent phases can write tests against the contract. `TaskService.rollback()` wraps `modelContext.safeRollback()` for handler-level undo after a mid-update throw. - Spec for extending `update_task` MCP tool and `UpdateTaskIntent` to update name, description, type, and metadata atomically alongside the existing milestone fields (T-650). Includes requirements, design (shared `TaskUpdateValidator` with `FieldChange<T>`-based update model, new `IntentHelpers.taskUpdateResponseDict`, `TaskService.rollback` for apply-failure rollback), decision log, and a 12-task implementation plan across 7 phases. - Test for AC 8.1: an allocation failure on one duplicate group does not abort subsequent groups; the loser loop breaks for the failing group only and the next group still reassigns. - `DisplayIDMaintenanceService` is now constructed cross-platform in `TransitApp`, registered via `AppDependencyManager` for App Intents, and exposed via `.environment` on the root `NavigationStack` and `withCoreEnvironments` so `DataMaintenanceView` resolves it on iOS, the macOS Settings window, and the macOS Task Detail window. - `ScanDuplicateDisplayIDsIntent` and `ReassignDuplicateDisplayIDsIntent` App Intents for running duplicate cleanup from Shortcuts. Both reuse the same `JSONEncoder` path as the MCP tools so payloads are byte-equal across surfaces; service errors land inside the JSON envelope rather than thrown. - `DataMaintenanceView` in Settings — scan → confirm (destructive alert) → reassign → result flow, available on iOS and macOS. macOS Settings adds a "Data Maintenance" sidebar category and an "Expose maintenance tools" Toggle in the MCP Server section. - `NavigationDestination.dataMaintenance` case wires the new view into both the iOS settings stack and the macOS settings window. - UI test for the Data Maintenance golden path with seeded duplicates via a new `UITestScenario.duplicateDisplayIds` case; matched accessibility identifiers `dataMaintenance.scanButton/.reassignButton/.confirmButton/.resultList`. - MCP server exposes `scan_duplicate_display_ids` and `reassign_duplicate_display_ids` tools, gated behind a new `MCPSettings.maintenanceToolsEnabled` toggle (UserDefaults `mcpMaintenanceToolsEnabled`, default off). When the toggle is off, both tools are excluded from `tools/list` and `tools/call` returns JSON-RPC `methodNotFound` with a distinct "Tool '<name>' is disabled. Enable maintenance tools in Transit Settings." message. - `MCPToolDefinitions.tools(includingMaintenance:)` helper splits core tools from maintenance tools so the maintenance tools no longer cost MCP context for every agent session. - `DisplayIDAllocator.counterStore` accessor exposes the underlying `CounterStore` so callers needing direct counter access (e.g. `DisplayIDMaintenanceService`'s counter-advance fence) can share the allocator's store. - `DisplayIDMaintenanceService` with `scanDuplicates` and `reassignDuplicates` for repairing tasks/milestones sharing a `permanentDisplayId`. Counter advance happens before loser allocation; reassigned tasks receive a "Transit Maintenance" audit comment recording the old and new display IDs and the date. - `DisplayIDMaintenanceTypes`: `DuplicateReport`, `ReassignmentResult`, `FailureCode`, and Codable encoders matching the JSON shape shared by MCP and App Intents. - `CounterStore.advanceCounter(toAtLeast:retryLimit:)` extension with default implementation that retries on conflict and short-circuits when a racing writer has already moved the counter past the target. - Spec for duplicate display ID cleanup: requirements, design, decision log, and 18-task implementation plan in `specs/duplicate-displayid-cleanup/` - Task detail view shows creation date in secondary style on both iOS and macOS (T-755) - macOS: Task detail opens in a dedicated window instead of a sheet, with share and edit buttons in the toolbar (T-35) - macOS: New Task opens in a dedicated window instead of a sheet (T-35) - macOS: Edit Task replaces detail content in the same window instead of presenting a sheet (T-35) - macOS: `BoardBackground` applied to task detail, edit, and new task windows for consistent styling (T-35) - `TaskDetailWindowView` — macOS-only wrapper managing detail/edit state transitions within a window - `UITestScenario` extracted to its own file for cleaner `TransitApp` organisation - `withCoreEnvironments` helper in `TransitApp` to consolidate shared environment modifiers across window scenes  ### Changed  - macOS: Cmd+N command opens a new task window via `openWindow` instead of using focused bindings (T-35) - macOS: Save buttons use `.confirmationAction` toolbar placement with "Save" text; iOS retains checkmark icon (T-35) - macOS: Edit Task back chevron uses `.navigation` placement (leading side) instead of `.cancellationAction` (T-35) - macOS: Removed inline `.borderedProminent` save buttons from ProjectEditView, MilestoneEditView, and TaskEditView macOS forms — save is now in the toolbar (T-35) - Fixed milestone picker in TaskEditView and AddTaskSheet to use UUID-based tags, avoiding `Hashable` requirement on `@Model` classes - iOS: Always hide system back button on ProjectEditView and MilestoneEditView to prevent duplicate chevrons  - Specs overview catalogue (`specs/OVERVIEW.md`) — tabular index of all 27 feature specs with creation dates, statuses, summaries, and per-spec file listings - CloudKit sync heartbeat: periodic SwiftData write every 60s while the MCP server is running on macOS, forcing `NSPersistentCloudKitContainer` to pull remote changes from iPhone/iPad within ~60s (T-631) - `SyncHeartbeat` SwiftData model — singleton record that triggers sync cycles - `SyncManager.startHeartbeat(context:)` / `stopHeartbeat()` — timer lifecycle management - `ContainerFactory` service for graceful `ModelContainer` creation with automatic fallback - Regression tests for container fallback behaviour (`ModelContainerFallbackTests`) - Home Screen Quick Actions: long-press the app icon on iOS to create a new task via "New Task" shortcut (T-27) - Static `UIApplicationShortcutItems` in Info.plist with SF Symbol `plus.square`

diff --git a/Transit/Transit/Intents/CreateTaskIntent.swift b/Transit/Transit/Intents/CreateTaskIntent.swiftindex 8cdf6c6..78e7d52 100644--- a/Transit/Transit/Intents/CreateTaskIntent.swift+++ b/Transit/Transit/Intents/CreateTaskIntent.swift@@ -64,184 +64,191 @@ struct CreateTaskIntent: AppIntent {      // MARK: - Logic (testable without @Dependency)      @MainActor     // swiftlint:disable:next cyclomatic_complexity function_body_length     static func execute(         input: String,         taskService: TaskService,         projectService: ProjectService,         milestoneService: MilestoneService? = nil,         persistence: PersistenceAvailability = .shared     ) async -> String {         // Refuse to write while the in-memory fallback container is active [T-1836].         if let unavailable = IntentHelpers.fallbackStorageErrorJSON(persistence) { return unavailable }          guard let json = IntentHelpers.parseJSON(input) else {             return IntentError.invalidInput(hint: "Expected valid JSON object").json         }          if let error = validateInput(json) { return error.json }          // Safe to force-unwrap: validateInput already verified these exist         let name = json["name"] as! String // swiftlint:disable:this force_cast         let typeRaw = json["type"] as! String // swiftlint:disable:this force_cast         let taskType = TaskType(rawValue: typeRaw)! // swiftlint:disable:this force_unwrapping          // Priority is optional and defaults to medium. Unlike the required `type`         // force-unwrap above, this is an optional-with-default path: absent -> .medium;         // present-and-invalid -> INVALID_PRIORITY (Decision 9); present-and-valid -> parse.         let priority: TaskPriority         switch parsePriority(json) {         case .failure(let error): return error.json         case .success(let parsed): priority = parsed         }          // Resolve project: projectId takes precedence over project name.         // Validate key presence separately from string/UUID parsing so non-string         // values (numbers, bools, etc.) are rejected too [T-743, T-788].         let projectId: UUID?         switch IntentHelpers.validateUUIDField("projectId", in: json) {         case .failure(let error): return error.json         case .success(let parsed): projectId = parsed         }         // Reject a present non-string `project` when projectId is absent [T-1453].         // Without this guard `as? String` silently drops the malformed value and the         // request falls through to the generic missing-project error instead of         // surfacing the type mismatch. projectId-takes-precedence is preserved: when a         // valid projectId is present the `project` name is ignored regardless of type.         if projectId == nil, let rawProject = json["project"], !(rawProject is String) {             return IntentError.invalidInput(hint: "project must be a string").json         }         let projectName = json["project"] as? String         let lookupResult = projectService.findProject(id: projectId, name: projectName)          let project: Project         switch lookupResult {         case .success(let found):             project = found         case .failure(let error):             return IntentHelpers.mapProjectLookupError(error).json         }          // Resolve milestone before creating task to avoid orphaned tasks [T-260]         let resolvedMilestone: Milestone?         if let milestoneService {             let (milestone, error) = resolveMilestone(from: json, in: project, using: milestoneService)             if let error { return error }             resolvedMilestone = milestone         } else {             resolvedMilestone = nil         }          let task: TransitTask         do {             task = try await taskService.createTask(                 name: name,                 description: json["description"] as? String,                 type: taskType,                 project: project,                 metadata: IntentHelpers.stringMetadata(from: json["metadata"]),-                priority: priority+                priority: priority,+                milestone: resolvedMilestone             )         } catch {-            return IntentError.invalidInput(hint: "Task creation failed").json-        }--        // Assign pre-resolved milestone [req 13.6]-        // Safety: resolveMilestone already verified the milestone exists and belongs to the-        // correct project, so setMilestone should only fail on an unexpected persistence error.-        // On failure, delete the task to avoid orphans [T-558], matching MCP create_task behavior.-        if let resolvedMilestone {-            do {-                try milestoneService?.setMilestone(resolvedMilestone, on: task)-            } catch {-                try? taskService.deleteTask(task)-                return IntentError.internalError(hint: "Failed to assign milestone").json-            }+            return mapCreateTaskError(error).json         }          return buildResponse(task)     }      // MARK: - Private Helpers +    /// Maps a `createTask` failure onto an intent error code. Milestone attachment and+    /// task creation now share one save, so this catch also covers what used to be a+    /// separate `setMilestone` failure. Keep input-shaped service errors on their+    /// specific codes and everything else (storage, allocator, cancellation) on+    /// INTERNAL_ERROR, so CLI callers can still tell a bad request from a retryable+    /// condition [T-1768].+    private static func mapCreateTaskError(_ error: Swift.Error) -> IntentError {+        switch error as? TaskService.Error {+        case .invalidName:+            .invalidInput(hint: "Task name cannot be empty")+        case .projectNotFound:+            .projectNotFound(hint: "The selected project could not be found")+        case .milestoneProjectMismatch:+            .milestoneProjectMismatch(hint: "Milestone and task must belong to the same project")+        default:+            .internalError(hint: "Task creation failed")+        }+    }+     @MainActor     private static func buildResponse(_ task: TransitTask) -> String {         var response: [String: Any] = [             "taskId": task.id.uuidString,             "status": task.statusRawValue,             // Effective-priority invariant (Req 1.4): echo the computed accessor.             "priority": task.priority.rawValue         ]         if let displayId = task.permanentDisplayId {             response["displayId"] = displayId         }         if let milestone = task.milestone {             response["milestone"] = IntentHelpers.milestoneInfoDict(milestone)         }         return IntentHelpers.encodeJSON(response)     }      /// Resolves a milestone from JSON before task creation. Returns `(milestone, nil)` on success     /// or `(nil, errorJSON)` on failure. Both nil means no milestone was requested.     @MainActor     private static func resolveMilestone(         from json: [String: Any],         in project: Project,         using milestoneService: MilestoneService     ) -> (milestone: Milestone?, error: String?) {         // Parse via the shared helper so JSON booleans are rejected: JSONSerialization         // delivers `true`/`false` as NSNumber(CFBoolean), which `as? Int` would otherwise         // accept as 1/0 and silently target M-1/M-0 [T-1211, T-1283].         let milestoneDisplayId = IntentHelpers.parseIntValue(json["milestoneDisplayId"])          // Reject non-integer milestoneDisplayId when key is present [T-613]         if json["milestoneDisplayId"] != nil, milestoneDisplayId == nil {             return (nil, IntentError.invalidInput(hint: "milestoneDisplayId must be an integer").json)         }          // Reject non-string milestone only when milestoneDisplayId is absent. When both keys         // are present, milestoneDisplayId takes priority and the `milestone` field is ignored,         // matching the MCP handler and IntentHelpers.assignMilestone [T-1114].         let milestoneName: String?         if milestoneDisplayId == nil, json["milestone"] != nil {             guard let name = json["milestone"] as? String else {                 return (nil, IntentError.invalidInput(hint: "milestone must be a string").json)             }             milestoneName = name         } else {             milestoneName = nil         }          if let milestoneDisplayId {             return resolveMilestone(                 displayId: milestoneDisplayId, in: project, using: milestoneService             )         }         if let milestoneName {             return resolveMilestone(named: milestoneName, in: project, using: milestoneService)         }         return (nil, nil)     }      @MainActor     private static func resolveMilestone(         displayId: Int,         in project: Project,         using milestoneService: MilestoneService     ) -> (milestone: Milestone?, error: String?) {         do {             let milestone = try milestoneService.findByDisplayID(displayId)             guard milestone.project?.id == project.id else {                 return (nil, IntentHelpers.mapMilestoneError(.projectMismatch).json)             }             return (milestone, nil)         } catch let error as MilestoneService.Error {             return (nil, IntentHelpers.mapMilestoneError(error).json)         } catch {             return (nil, IntentError.milestoneNotFound(                 hint: "No milestone with displayId \(displayId)"             ).json)         }     } 

diff --git a/Transit/Transit/MCP/MCPToolHandler.swift b/Transit/Transit/MCP/MCPToolHandler.swiftindex 159873a..bacedeb 100644--- a/Transit/Transit/MCP/MCPToolHandler.swift+++ b/Transit/Transit/MCP/MCPToolHandler.swift@@ -242,176 +242,167 @@ final class MCPToolHandler {         // Reject malformed or non-string projectId when the key is present         // [T-743, T-788].         let projectId: UUID?         switch parseProjectIdArgument(args) {         case .failure(.message(let message)): return errorResult(message)         case .success(let parsed): projectId = parsed         }         // Reject a present non-string `project` when projectId is absent [T-1453].         // Without this guard `as? String` silently drops the malformed value and the         // request falls through to the generic missing-project error instead of         // surfacing the type mismatch. projectId-takes-precedence is preserved: when a         // valid projectId is present the `project` name is ignored regardless of type.         if projectId == nil, let rawProject = args["project"], !(rawProject is String) {             return errorResult("project must be a string")         }         let projectName = args["project"] as? String         let project: Project         switch projectService.findProject(id: projectId, name: projectName) {         case .success(let found):             project = found         case .failure(let error):             return errorResult(IntentHelpers.mapProjectLookupError(error).hint)         }          // Pre-validate milestone before creating the task to avoid orphans.         // 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")         }         var resolvedMilestone: Milestone?         if let milestoneDisplayId = IntentHelpers.parseIntValue(args["milestoneDisplayId"]) {             do {                 resolvedMilestone = try milestoneService.findByDisplayID(milestoneDisplayId)             } catch MilestoneService.Error.milestoneNotFound {                 return errorResult("No milestone with displayId \(milestoneDisplayId)")             } catch MilestoneService.Error.duplicateDisplayID {                 return errorResult("Duplicate milestone identifier detected for displayId \(milestoneDisplayId)")             } catch {                 return errorResult("Failed to find milestone: \(error)")             }             guard let milestoneProject = resolvedMilestone?.project,                   milestoneProject.id == project.id else {                 return errorResult("Milestone and task must belong to the same project")             }         } else if args["milestone"] != nil {             // Reject non-string milestone values [T-1114]. Without this guard             // a numeric or boolean milestone arg would fall through to the             // "absent" branch and the task would be created without the             // requested assignment.             guard let milestoneName = args["milestone"] as? String else {                 return errorResult("milestone must be a string")             }             do {                 guard let milestone = try milestoneService.findByName(milestoneName, in: project) else {                     return errorResult("No milestone named '\(milestoneName)' in project '\(project.name)'")                 }                 resolvedMilestone = milestone             } catch MilestoneService.Error.ambiguousName {                 return errorResult(                     "Multiple milestones named '\(milestoneName)' exist in project '\(project.name)'"                 )             } catch {                 return errorResult("Failed to look up milestone: \(error)")             }         }          // Reject non-string description: as? String silently drops         // present-but-wrong-type values, making a malformed request look successful. [T-1192]         if args["description"] != nil, args["description"] as? String == nil {             return errorResult("description must be a string")         }          let task: TransitTask         do {             task = try await taskService.createTask(                 name: name,                 description: args["description"] as? String,                 type: taskType,                 project: project,                 metadata: IntentHelpers.stringMetadata(from: args["metadata"]),-                priority: priority+                priority: priority,+                milestone: resolvedMilestone             )         } catch {             return errorResult("Task creation failed: \(error)")         } -        if let milestone = resolvedMilestone {-            do {-                try milestoneService.setMilestone(milestone, on: task)-            } catch {-                // Pre-validation should prevent this, but clean up the task on unexpected failures.-                try? taskService.deleteTask(task)-                return errorResult("Failed to set milestone: \(error)")-            }-        }-         var response: [String: Any] = [             "taskId": task.id.uuidString,             "status": task.statusRawValue,             "priority": task.priority.rawValue         ]         if let displayId = task.permanentDisplayId {             response["displayId"] = displayId         }         if let milestone = task.milestone {             var milestoneDict: [String: Any] = [                 "milestoneId": milestone.id.uuidString,                 "name": milestone.name             ]             if let mDisplayId = milestone.permanentDisplayId {                 milestoneDict["displayId"] = mDisplayId             }             response["milestone"] = milestoneDict         }         return textResult(IntentHelpers.encodeJSON(response))     }      // MARK: - update_task_status      private func handleUpdateStatus(_ args: [String: Any]) -> MCPToolResult {         // When the "status" argument is present it MUST be a string — a non-string         // value would otherwise be silently dropped by `as? String` and misreported         // as missing. Reject it before any mutation, like the milestone paths [T-1544].         guard args["status"] != nil else {             return errorResult("Missing required argument: status")         }         guard let statusString = args["status"] as? String else {             return errorResult("status must be a string")         }         guard let newStatus = TaskStatus(rawValue: statusString) else {             let valid = TaskStatus.allCases.map(\.rawValue).joined(separator: ", ")             return errorResult("Invalid status: \(statusString). Must be one of: \(valid)")         }          // Reject non-integer displayId when key is present [T-634]         if args["displayId"] != nil, IntentHelpers.parseIntValue(args["displayId"]) == nil {             return errorResult("displayId must be an integer")         }          let task: TransitTask         switch resolveTaskArgument(args) {         case .success(let resolved): task = resolved         case .failure(.message(let message)): return errorResult(message)         }          // Reject non-string comment/authorName: as? String silently drops         // present-but-wrong-type values, dropping the audit-trail entry. [T-1205]         if args["comment"] != nil, !(args["comment"] is String) {             return errorResult("comment must be a string")         }         if args["authorName"] != nil, !(args["authorName"] is String) {             return errorResult("authorName must be a string")         }         let commentText = args["comment"] as? String         let commentAuthor = args["authorName"] as? String         let (commentError, hasComment) = validateCommentArgs(comment: commentText, author: commentAuthor)         if let commentError {             return commentError         }          let previousStatus = task.statusRawValue         do {             try taskService.updateStatus(                 task: task, to: newStatus,                 comment: commentText, commentAuthor: commentAuthor, commentService: commentService             )         } catch {             return errorResult("Status update failed: \(error)")         }          var response = statusResponse(task: task, previousStatus: previousStatus, newStatus: newStatus)         appendCommentDetails(to: &response, taskID: task.id, hasComment: hasComment)         return textResult(IntentHelpers.encodeJSON(response))     }      // MARK: - query_tasks

diff --git a/Transit/Transit/Services/TaskService.swift b/Transit/Transit/Services/TaskService.swiftindex 7f3efcb..d1091d8 100644--- a/Transit/Transit/Services/TaskService.swift+++ b/Transit/Transit/Services/TaskService.swift@@ -1,218 +1,238 @@ import Foundation import SwiftData  /// Coordinates task creation, status changes, and lookups. Uses StatusEngine /// for all status transitions and DisplayIDAllocator for display ID assignment. @MainActor @Observable final class TaskService {      enum Error: Swift.Error, LocalizedError, Equatable {         case invalidName         case taskNotFound         case projectNotFound         case duplicateDisplayID         case restoreRequiresAbandonedTask+        case milestoneProjectMismatch         /// Identifier key present but malformed; field name surfaces a field-specific INVALID_INPUT [T-808]         case invalidIdentifier(field: String)          var errorDescription: String? {             switch self {             case .invalidName:                 "Task name cannot be empty."             case .taskNotFound:                 "The specified task could not be found."             case .projectNotFound:                 "The selected project could not be found."             case .duplicateDisplayID:                 "A duplicate task identifier was detected."             case .restoreRequiresAbandonedTask:                 "Only abandoned tasks can be restored."+            case .milestoneProjectMismatch:+                "Milestone and task must belong to the same project."             case .invalidIdentifier(let field):                 "The supplied \(field) is not a valid task identifier."             }         }     }      private let modelContext: ModelContext     private let displayIDAllocator: DisplayIDAllocator+    private let createSave: (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);     /// production reads the same context.     private let usedDisplayIDs: UsedDisplayIDs      init(         modelContext: ModelContext,         displayIDAllocator: DisplayIDAllocator,-        fetcher: (any ModelFetching)? = nil+        fetcher: (any ModelFetching)? = nil,+        createSave: @escaping (ModelContext) throws -> Void = { try $0.save() }     ) {         self.modelContext = modelContext         self.displayIDAllocator = displayIDAllocator         self.usedDisplayIDs = UsedDisplayIDs(fetcher ?? modelContext)+        self.createSave = createSave     }      // MARK: - Task Creation      /// Creates a new task in `.idea` status, looking up the project by UUID.     /// Fetches the project from the model context, then delegates to the     /// primary `createTask` overload.     @discardableResult     func createTask(         name: String,         description: String?,         type: TaskType,         projectID: UUID,         metadata: [String: String]? = nil,-        priority: TaskPriority = .medium+        priority: TaskPriority = .medium,+        milestone: Milestone? = nil     ) async throws -> TransitTask {         let descriptor = FetchDescriptor<Project>(             predicate: #Predicate { $0.id == projectID }         )         guard let project = try modelContext.fetch(descriptor).first else {             throw Error.projectNotFound         }         return try await createTask(             name: name,             description: description,             type: type,             project: project,             metadata: metadata,-            priority: priority+            priority: priority,+            milestone: milestone         )     } -    /// Creates a new task in `.idea` status. Attempts to allocate a permanent-    /// display ID from CloudKit; falls back to provisional on failure.+    /// Creates a new task in `.idea` status. The optional milestone is validated+    /// and attached before insertion so the aggregate is persisted in one save.+    /// Attempts to allocate a permanent display ID from CloudKit; falls back to+    /// provisional on failure. `createSave` is injectable for surface-level+    /// persistence failure tests.     @discardableResult     func createTask(         name: String,         description: String?,         type: TaskType,         project: Project,         metadata: [String: String]? = nil,         priority: TaskPriority = .medium,-        save: (ModelContext) throws -> Void = { try $0.save() }+        milestone: Milestone? = nil,+        save: ((ModelContext) throws -> Void)? = nil     ) async throws -> TransitTask {         let trimmedName = name.trimmingCharacters(in: .whitespacesAndNewlines)         guard !trimmedName.isEmpty else {             throw Error.invalidName         } +        // Keep this validation at the aggregate boundary even though automation+        // surfaces pre-validate resolved milestones: direct UI and future callers+        // must not allocate an ID or insert a cross-project relationship.+        if let milestone, milestone.project?.id != project.id {+            throw Error.milestoneProjectMismatch+        }+         let displayID: DisplayID         do {             // Pass a closure so the set of display IDs already committed locally             // is snapshotted inside the allocation gate — fresh relative to any             // concurrent create that committed just before us — so the allocator             // never hands back an in-use ID even against a stale counter read             // (T-1395).             let id = try await displayIDAllocator.allocateNextID(excluding: { try self.usedDisplayIDs.tasks() })             displayID = .permanent(id)         } catch let error as CancellationError {             // The allocation gate propagates CancellationError when this caller is             // cancelled while waiting for an ID (T-1395). Cancellation must abort the             // create before any insert/save so a cancelled request never mutates             // persistent state — only genuine CloudKit/offline failures fall back to             // a provisional ID (T-1426).             throw error         } catch DisplayIDAllocator.Error.usedIDLookupFailed(let description) {             // The local store could not be read, so the collision guard never ran.             // That is not the offline condition provisional IDs exist for, and a             // provisional ID would hide a broken store behind a normal-looking             // create — surface it instead (T-1621).             throw DisplayIDAllocator.Error.usedIDLookupFailed(description: description)         } catch {             displayID = .provisional         }          let task = TransitTask(             name: trimmedName,             description: description,             type: type,             project: project,             displayID: displayID,             metadata: metadata,             priority: priority         )         StatusEngine.initializeNewTask(task)+        task.milestone = milestone -        try modelContext.insertOrDelete(task, save: save)+        try modelContext.insertOrDelete(task, save: save ?? createSave)         return task     }      // MARK: - Status Changes      /// Transitions a task to a new status via StatusEngine.     /// When comment parameters are provided, creates a comment atomically     /// in the same save operation.     /// 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.     func updateStatus(         task: TransitTask,         to newStatus: TaskStatus,         comment: String? = nil,         commentAuthor: String? = nil,         commentService: CommentService? = nil,         save: Bool = true     ) throws {         let statusChanged = task.status != newStatus         let hasComment = comment.map { !$0.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty } ?? false         guard statusChanged || hasComment else { return }          if statusChanged {             StatusEngine.applyTransition(task: task, to: newStatus)         }          try modelContext.saveOrRollback(save: save) {             if let comment, !comment.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty,                let commentAuthor, let commentService {                 try commentService.addComment(                     to: task,                     content: comment,                     authorName: commentAuthor,                     isAgent: true,                     save: nil                 )             }         }     }      /// Moves a task to `.abandoned` status.     func abandon(task: TransitTask) throws {         StatusEngine.applyTransition(task: task, to: .abandoned)         try modelContext.saveOrRollback()     }      /// Restores an abandoned task back to `.idea` status.     func restore(task: TransitTask) throws {         guard task.status == .abandoned else {             throw Error.restoreRequiresAbandonedTask         }         StatusEngine.applyTransition(task: task, to: .idea)         try modelContext.saveOrRollback()     }      // MARK: - Project Management      /// Changes a task's project. Clears milestone before the change to enforce     /// Decision 6 (milestones are scoped to a project).     func changeProject(task: TransitTask, to newProject: Project, save: Bool = true) throws {         if task.project?.id != newProject.id {             task.milestone = nil         }         task.project = newProject          try modelContext.saveOrRollback(save: save)     }      // MARK: - Resolution      /// Resolves a task from a string identifier (display ID as integer string, or UUID string).     func resolveTask(from identifier: String) throws -> TransitTask {         if let displayId = Int(identifier) {             return try findByDisplayID(displayId)         } else if let uuid = UUID(uuidString: identifier) {             return try findByID(uuid)         }         throw Error.taskNotFound     }

diff --git a/Transit/Transit/Views/AddTask/AddTaskSheet+Save.swift b/Transit/Transit/Views/AddTask/AddTaskSheet+Save.swiftindex d617095..32bbead 100644--- a/Transit/Transit/Views/AddTask/AddTaskSheet+Save.swift+++ b/Transit/Transit/Views/AddTask/AddTaskSheet+Save.swift@@ -1,80 +1,63 @@ import SwiftData import SwiftUI  // MARK: - Actions  extension AddTaskSheet {      func save() async {         guard let project = selectedProject else { return }         let trimmedName = name.trimmedForFormInput()         guard !trimmedName.isEmpty else { return }          let description = taskDescription.trimmedForFormInput()         let draft = TaskDraft(             name: trimmedName,             description: description.isEmpty ? nil : description,             type: selectedType,             priority: selectedPriority,             projectID: project.id,             milestone: selectedMilestone         )          isSaving = true         defer { isSaving = false }          do {             try await Self.persist(                 draft: draft,-                taskService: taskService,-                milestoneService: milestoneService+                taskService: taskService             )             dismiss()         } catch {             errorMessage = error.localizedDescription         }     }      /// Fields collected by the New Task form, ready to be persisted.     struct TaskDraft {         let name: String         let description: String?         let type: TaskType         let priority: TaskPriority         let projectID: UUID         let milestone: Milestone?     } -    /// Persists a new task and optionally assigns a milestone. Extracted as a+    /// Persists a new task and optional milestone as one aggregate. Extracted as a     /// static helper because the SwiftUI view layer cannot be invoked directly     /// from tests.-    ///-    /// When milestone assignment fails after `createTask` has already saved-    /// the task, the newly-created task is deleted before rethrowing so the-    /// operation is atomic from the user's perspective. Matches the cleanup-    /// pattern used by `CreateTaskIntent` and MCP `create_task` [T-558,-    /// T-855].     static func persist(         draft: TaskDraft,-        taskService: TaskService,-        milestoneService: MilestoneService+        taskService: TaskService     ) async throws {-        let task = try await taskService.createTask(+        _ = try await taskService.createTask(             name: draft.name,             description: draft.description,             type: draft.type,             projectID: draft.projectID,-            priority: draft.priority+            priority: draft.priority,+            milestone: draft.milestone         )-        guard let milestone = draft.milestone else { return }-        do {-            try milestoneService.setMilestone(milestone, on: task)-        } catch {-            // Avoid leaving an orphaned task in the store when the milestone-            // cannot be attached. `deleteTask` is best-effort: if it also-            // fails we still surface the original assignment error.-            try? taskService.deleteTask(task)-            throw error-        }     } }

diff --git a/Transit/TransitTests/AddTaskSheetSaveErrorTests.swift b/Transit/TransitTests/AddTaskSheetSaveErrorTests.swiftindex ee04537..16c837b 100644--- a/Transit/TransitTests/AddTaskSheetSaveErrorTests.swift+++ b/Transit/TransitTests/AddTaskSheetSaveErrorTests.swift@@ -1,245 +1,281 @@ import Foundation import SwiftData import Testing @testable import Transit  /// Regression tests for T-153: AddTaskSheet must not dismiss before task creation /// completes. The fix awaits `taskService.createTask` and only dismisses on success, /// showing an error alert on failure. /// /// Since AddTaskSheet is a SwiftUI view, these tests verify the service-layer contract /// the fix depends on: that `createTask` errors propagate (are not silently swallowed) /// and that the task is not persisted when creation fails. ///-/// Regression tests for T-855: AddTaskSheet must not leave an orphaned task when-/// the milestone assignment that follows `createTask` fails. The fix mirrors the-/// established cleanup pattern from `CreateTaskIntent` / MCP `create_task`-/// (T-558): on `setMilestone` failure after `createTask` succeeded, delete the-/// newly-created task before surfacing the error.+/// Regression tests for T-855 and T-1768: AddTaskSheet must not leave an orphaned+/// task when milestone validation or persistence fails. TaskService now attaches the+/// optional milestone before insertion and persists the aggregate in one save. @MainActor @Suite(.serialized) struct AddTaskSheetSaveErrorTests {      // MARK: - Helpers      private struct Services {         let task: TaskService         let milestone: MilestoneService         let context: ModelContext     }      private func makeService() throws -> (TaskService, ModelContext) {         let testContainer = try TestModelContainer()         let context = testContainer.context         let store = InMemoryCounterStore()         let allocator = DisplayIDAllocator(store: store)         let service = TaskService(modelContext: context, displayIDAllocator: allocator)         return (service, context)     } -    private func makeServices() throws -> Services {+    private func makeServices(+        createSave: @escaping (ModelContext) throws -> Void = { try $0.save() }+    ) throws -> Services {         let testContainer = try TestModelContainer()         let context = testContainer.context         let taskAllocator = DisplayIDAllocator(store: InMemoryCounterStore())         let milestoneAllocator = DisplayIDAllocator(store: InMemoryCounterStore())         return Services(-            task: TaskService(modelContext: context, displayIDAllocator: taskAllocator),+            task: TaskService(+                modelContext: context,+                displayIDAllocator: taskAllocator,+                createSave: createSave+            ),             milestone: MilestoneService(modelContext: context, displayIDAllocator: milestoneAllocator),             context: context         )     }      private func makeProject(in context: ModelContext, name: String = "Test Project") -> Project {         let project = Project(             name: name,             description: "A test project",             gitRepo: nil,             colorHex: "#FF0000"         )         context.insert(project)         return project     }      @discardableResult     private func makeMilestone(         in context: ModelContext,         name: String,         project: Project,         displayId: Int     ) -> Milestone {         let milestone = Milestone(             name: name,             description: nil,             project: project,             displayID: .permanent(displayId)         )         context.insert(milestone)         return milestone     }      // MARK: - Error propagation from createTask      @Test func createTaskWithInvalidProjectIDThrowsProjectNotFound() async throws {         let (service, _) = try makeService()         let bogusProjectID = UUID()          // This error was silently discarded by the old code (`_ = try await` in a         // detached Task). The fix uses do/catch so this error surfaces to the user.         do {             _ = try await service.createTask(                 name: "Test Task",                 description: nil,                 type: .feature,                 projectID: bogusProjectID             )             Issue.record("Expected TaskService.Error.projectNotFound")         } catch let error as TaskService.Error {             #expect(error == .projectNotFound)         }     }      /// Verifies the service rejects empty names even though AddTaskSheet.save()     /// guards this before calling createTask. Documents the service-level contract     /// so callers that skip view-level validation still get a clear error.     @Test func createTaskWithEmptyNameThrowsInvalidName() async throws {         let (service, context) = try makeService()         let project = makeProject(in: context)          do {             _ = try await service.createTask(                 name: "   ",                 description: nil,                 type: .feature,                 project: project             )             Issue.record("Expected TaskService.Error.invalidName")         } catch let error as TaskService.Error {             #expect(error == .invalidName)         }     }      @Test func noTaskPersistedWhenCreationFailsDueToInvalidProject() async throws {         let (service, context) = try makeService()         let bogusProjectID = UUID()          _ = try? await service.createTask(             name: "Ghost Task",             description: nil,             type: .feature,             projectID: bogusProjectID         )          // Verify no task was inserted into the context         let descriptor = FetchDescriptor<TransitTask>()         let tasks = try context.fetch(descriptor)         #expect(tasks.isEmpty)     }      @Test func successfulCreationReturnsPersistableTask() async throws {         let (service, context) = try makeService()         let project = makeProject(in: context)          let task = try await service.createTask(             name: "Valid Task",             description: "Description",             type: .feature,             projectID: project.id         )          // Verify the task is persisted and queryable         #expect(task.name == "Valid Task")         #expect(task.status == .idea)          let descriptor = FetchDescriptor<TransitTask>()         let tasks = try context.fetch(descriptor)         #expect(tasks.count == 1)         #expect(tasks.first?.name == "Valid Task")     } -    // MARK: - T-855: Milestone assignment failure must not leave an orphan+    // MARK: - Atomic milestone creation -    /// Reproduces the orphan scenario in `AddTaskSheet.persist`: the user-    /// selects a milestone, the task is created and saved, then `setMilestone`-    /// rejects the assignment (here via a project-mismatch — an unlikely but-    /// possible state if the milestone picker shows stale data). Before the-    /// T-855 fix the catch path only surfaced an error to the UI and left the-    /// newly-created task in the database.+    /// A stale milestone from another project is rejected before TaskService inserts+    /// the aggregate, so there is no persisted task to compensate for or clean up.     @Test func persistRollsBackTaskWhenMilestoneFailsProjectMismatch() async throws {         let svc = try makeServices()         let projectA = makeProject(in: svc.context, name: "Project A")         let projectB = makeProject(in: svc.context, name: "Project B")         let milestoneInB = makeMilestone(in: svc.context, name: "v1.0", project: projectB, displayId: 1)          // Simulate the user picking projectA but a milestone bound to projectB.         let draft = AddTaskSheet.TaskDraft(             name: "Orphan Candidate",             description: nil,             type: .bug,             priority: .medium,             projectID: projectA.id,             milestone: milestoneInB         )-        await #expect(throws: MilestoneService.Error.projectMismatch) {+        await #expect(throws: TaskService.Error.milestoneProjectMismatch) {             try await AddTaskSheet.persist(                 draft: draft,-                taskService: svc.task,-                milestoneService: svc.milestone+                taskService: svc.task             )         }          let descriptor = FetchDescriptor<TransitTask>()         let tasks = try svc.context.fetch(descriptor)-        #expect(tasks.isEmpty, "Task must not remain in the database when milestone assignment fails [T-855]")+        #expect(tasks.isEmpty, "Task must not be inserted when milestone validation fails")+    }++    /// T-1768: the requested milestone must be attached before the only create+    /// save. A failed save must remove the entire aggregate so a later save+    /// cannot resurrect a task the sheet reported as failed.+    @Test func persistWithMilestoneIsAtomicWhenCreateSaveFails() async throws {+        var expectedMilestoneID: UUID?+        let svc = try makeServices { context in+            let pendingTasks = try context.fetch(FetchDescriptor<TransitTask>())+            #expect(pendingTasks.count == 1)+            #expect(pendingTasks.first?.milestone?.id == expectedMilestoneID)+            throw SaveFailure.simulated+        }+        let project = makeProject(in: svc.context)+        let milestone = makeMilestone(in: svc.context, name: "v1.0", project: project, displayId: 1)+        expectedMilestoneID = milestone.id++        let draft = AddTaskSheet.TaskDraft(+            name: "Atomic Task",+            description: nil,+            type: .bug,+            priority: .medium,+            projectID: project.id,+            milestone: milestone+        )++        await #expect(throws: SaveFailure.self) {+            try await AddTaskSheet.persist(+                draft: draft,+                taskService: svc.task+            )+        }++        #expect(try svc.context.fetch(FetchDescriptor<TransitTask>()).isEmpty)+        try svc.context.save()+        #expect(+            try svc.context.fetch(FetchDescriptor<TransitTask>()).isEmpty,+            "A later save must not resurrect a failed aggregate create [T-1768]"+        )     }      /// Verifies the happy-path: when a valid milestone in the same project is     /// supplied, `persist` saves the task and assigns the milestone in one go.     @Test func persistSucceedsWithValidMilestone() async throws {         let svc = try makeServices()         let project = makeProject(in: svc.context)         let milestone = makeMilestone(in: svc.context, name: "v1.0", project: project, displayId: 1)          let draft = AddTaskSheet.TaskDraft(             name: "Happy Task",             description: nil,             type: .feature,             priority: .medium,             projectID: project.id,             milestone: milestone         )         try await AddTaskSheet.persist(             draft: draft,-            taskService: svc.task,-            milestoneService: svc.milestone+            taskService: svc.task         )          let tasks = try svc.context.fetch(FetchDescriptor<TransitTask>())         #expect(tasks.count == 1)         #expect(tasks.first?.milestone?.id == milestone.id)     }      /// Verifies that when no milestone is selected the helper still saves the     /// task without attempting an assignment. This guards against an overly     /// aggressive cleanup that might delete tasks unconditionally on any error.     @Test func persistSucceedsWithoutMilestone() async throws {         let svc = try makeServices()         let project = makeProject(in: svc.context)          let draft = AddTaskSheet.TaskDraft(             name: "No Milestone Task",             description: nil,             type: .feature,             priority: .medium,             projectID: project.id,             milestone: nil         )         try await AddTaskSheet.persist(             draft: draft,-            taskService: svc.task,-            milestoneService: svc.milestone+            taskService: svc.task         )          let tasks = try svc.context.fetch(FetchDescriptor<TransitTask>())         #expect(tasks.count == 1)         #expect(tasks.first?.milestone == nil)     } }

diff --git a/Transit/TransitTests/CreateTaskIntentMilestoneTests.swift b/Transit/TransitTests/CreateTaskIntentMilestoneTests.swiftindex 463dbbe..847f048 100644--- a/Transit/TransitTests/CreateTaskIntentMilestoneTests.swift+++ b/Transit/TransitTests/CreateTaskIntentMilestoneTests.swift@@ -1,234 +1,243 @@ import Foundation import SwiftData import Testing @testable import Transit  /// Milestone-related tests for CreateTaskIntent [req 13.6] @MainActor @Suite(.serialized) struct CreateTaskIntentMilestoneTests {      // MARK: - Helpers      private struct Services {         let task: TaskService         let milestone: MilestoneService         let project: ProjectService         let context: ModelContext     } -    private func makeServices() throws -> Services {+    private func makeServices(+        createSave: @escaping (ModelContext) throws -> Void = { try $0.save() }+    ) throws -> Services {         let testContainer = try TestModelContainer()         let context = testContainer.context         let taskStore = InMemoryCounterStore()         let taskAllocator = DisplayIDAllocator(store: taskStore)         let milestoneStore = InMemoryCounterStore()         let milestoneAllocator = DisplayIDAllocator(store: milestoneStore)         return Services(-            task: TaskService(modelContext: context, displayIDAllocator: taskAllocator),+            task: TaskService(+                modelContext: context,+                displayIDAllocator: taskAllocator,+                createSave: createSave+            ),             milestone: MilestoneService(modelContext: context, displayIDAllocator: milestoneAllocator),             project: ProjectService(modelContext: context),             context: context         )     }      @discardableResult     private func makeProject(in context: ModelContext, name: String = "Test Project") -> Project {         let project = Project(name: name, description: "A test project", gitRepo: nil, colorHex: "#FF0000")         context.insert(project)         return project     }      @discardableResult     private func makeMilestone(         in context: ModelContext,         name: String,         project: Project,         displayId: Int     ) -> Milestone {         let milestone = Milestone(name: name, description: nil, project: project, displayID: .permanent(displayId))         context.insert(milestone)         return milestone     }      private func parseJSON(_ string: String) throws -> [String: Any] {         let data = try #require(string.data(using: .utf8))         return try #require(try JSONSerialization.jsonObject(with: data) as? [String: Any])     }      // MARK: - Milestone Assignment at Creation      @Test func createTaskWithMilestoneDisplayId() async throws {         let svc = try makeServices()         let project = makeProject(in: svc.context)         makeMilestone(in: svc.context, name: "v1.0", project: project, displayId: 1)          let input = """         {"name":"Task","type":"feature","project":"\(project.name)","milestoneDisplayId":1}         """          let result = await CreateTaskIntent.execute(             input: input, taskService: svc.task,             projectService: svc.project, milestoneService: svc.milestone         )          let parsed = try parseJSON(result)         #expect(parsed["taskId"] is String)         let milestoneInfo = parsed["milestone"] as? [String: Any]         #expect(milestoneInfo?["name"] as? String == "v1.0")         #expect(milestoneInfo?["displayId"] as? Int == 1)     }      @Test func createTaskWithMilestoneName() async throws {         let svc = try makeServices()         let project = makeProject(in: svc.context)         makeMilestone(in: svc.context, name: "v1.0", project: project, displayId: 1)          let input = """         {"name":"Task","type":"feature","project":"\(project.name)","milestone":"v1.0"}         """          let result = await CreateTaskIntent.execute(             input: input, taskService: svc.task,             projectService: svc.project, milestoneService: svc.milestone         )          let parsed = try parseJSON(result)         #expect(parsed["taskId"] is String)         let milestoneInfo = parsed["milestone"] as? [String: Any]         #expect(milestoneInfo?["name"] as? String == "v1.0")     } +    @Test func createTaskWithMilestoneSaveFailureIsAtomic() async throws {+        var expectedMilestoneID: UUID?+        let svc = try makeServices { context in+            let pendingTasks = try context.fetch(FetchDescriptor<TransitTask>())+            #expect(pendingTasks.count == 1)+            #expect(pendingTasks.first?.milestone?.id == expectedMilestoneID)+            throw SaveFailure.simulated+        }+        let project = makeProject(in: svc.context)+        let milestone = makeMilestone(in: svc.context, name: "v1.0", project: project, displayId: 1)+        expectedMilestoneID = milestone.id++        let input = """+        {"name":"Atomic Task","type":"bug","project":"\(project.name)","milestoneDisplayId":1}+        """+        let result = await CreateTaskIntent.execute(+            input: input,+            taskService: svc.task,+            projectService: svc.project,+            milestoneService: svc.milestone+        )++        let parsed = try parseJSON(result)+        // A storage failure is retryable, so it must not be reported as a bad request.+        #expect(parsed["error"] as? String == "INTERNAL_ERROR")+        #expect(try svc.context.fetch(FetchDescriptor<TransitTask>()).isEmpty)+        try svc.context.save()+        #expect(+            try svc.context.fetch(FetchDescriptor<TransitTask>()).isEmpty,+            "A later save must not resurrect a failed aggregate create [T-1768]"+        )+    }+     @Test func createTaskWithUnknownMilestoneReturnsMilestoneNotFound() async throws {         let svc = try makeServices()         let project = makeProject(in: svc.context)          let input = """         {"name":"Task","type":"feature","project":"\(project.name)","milestoneDisplayId":999}         """          let result = await CreateTaskIntent.execute(             input: input, taskService: svc.task,             projectService: svc.project, milestoneService: svc.milestone         )          let parsed = try parseJSON(result)         #expect(parsed["error"] as? String == "MILESTONE_NOT_FOUND")     }      // Regression test for T-260: milestone failure must not leave an orphaned task     @Test func createTaskWithUnknownMilestoneDoesNotPersistTask() async throws {         let svc = try makeServices()         let project = makeProject(in: svc.context)          let input = """         {"name":"Orphan Check","type":"bug","project":"\(project.name)","milestoneDisplayId":999}         """          let result = await CreateTaskIntent.execute(             input: input, taskService: svc.task,             projectService: svc.project, milestoneService: svc.milestone         )          let parsed = try parseJSON(result)         #expect(parsed["error"] as? String == "MILESTONE_NOT_FOUND")          // Verify no task was persisted         let descriptor = FetchDescriptor<TransitTask>()         let tasks = try svc.context.fetch(descriptor)         #expect(tasks.isEmpty, "Task should not be created when milestone lookup fails")     }      @Test func createTaskWithUnknownMilestoneNameDoesNotPersistTask() async throws {         let svc = try makeServices()         let project = makeProject(in: svc.context)          let input = """         {"name":"Orphan Check","type":"bug","project":"\(project.name)","milestone":"nonexistent"}         """          let result = await CreateTaskIntent.execute(             input: input, taskService: svc.task,             projectService: svc.project, milestoneService: svc.milestone         )          let parsed = try parseJSON(result)         #expect(parsed["error"] as? String == "MILESTONE_NOT_FOUND")          let descriptor = FetchDescriptor<TransitTask>()         let tasks = try svc.context.fetch(descriptor)         #expect(tasks.isEmpty, "Task should not be created when milestone name lookup fails")     }      // Regression test for T-260: project mismatch must not leave an orphaned task     @Test func createTaskWithMilestoneInDifferentProjectDoesNotPersistTask() async throws {         let svc = try makeServices()         let projectA = makeProject(in: svc.context, name: "Project A")         let projectB = makeProject(in: svc.context, name: "Project B")         makeMilestone(in: svc.context, name: "v1.0", project: projectB, displayId: 1)          let input = """         {"name":"Orphan Check","type":"bug","project":"\(projectA.name)","milestoneDisplayId":1}         """          let result = await CreateTaskIntent.execute(             input: input, taskService: svc.task,             projectService: svc.project, milestoneService: svc.milestone         )          let parsed = try parseJSON(result)         #expect(parsed["error"] as? String == "MILESTONE_PROJECT_MISMATCH")          let descriptor = FetchDescriptor<TransitTask>()         let tasks = try svc.context.fetch(descriptor)         #expect(tasks.isEmpty, "Task should not be created when milestone belongs to a different project")     } -    // Regression test for T-558: verify that task cleanup via context.delete + save-    // removes a persisted task. This validates the cleanup mechanism used in-    // CreateTaskIntent.execute when setMilestone fails after task creation.-    @Test func taskCleanupDeletesPersistedTask() async throws {-        let svc = try makeServices()-        let project = makeProject(in: svc.context)--        // Create a task (simulating what execute does before setMilestone)-        let task = try await svc.task.createTask(-            name: "T-558 Orphan Test",-            description: nil,-            type: .bug,-            project: project-        )--        // Verify the task exists-        let beforeDescriptor = FetchDescriptor<TransitTask>()-        let beforeTasks = try svc.context.fetch(beforeDescriptor)-        #expect(beforeTasks.count == 1)--        // Simulate the cleanup path from the T-558 fix:-        // taskService.deleteTask(task)-        try svc.task.deleteTask(task)--        // Verify the task was removed-        let afterDescriptor = FetchDescriptor<TransitTask>()-        let afterTasks = try svc.context.fetch(afterDescriptor)-        #expect(afterTasks.isEmpty, "Task should be deleted after cleanup [T-558]")-    }-     @Test func createTaskWithoutMilestoneServiceSkipsMilestone() async throws {         let svc = try makeServices()         let project = makeProject(in: svc.context)          let input = """         {"name":"Task","type":"feature","project":"\(project.name)","milestoneDisplayId":1}         """          let result = await CreateTaskIntent.execute(             input: input, taskService: svc.task,             projectService: svc.project         )          let parsed = try parseJSON(result)         // Should succeed — milestone param is silently ignored without milestoneService         #expect(parsed["taskId"] is String)         #expect(parsed["milestone"] == nil)     } }

diff --git a/Transit/TransitTests/MCPCreateTaskAtomicityTests.swift b/Transit/TransitTests/MCPCreateTaskAtomicityTests.swiftnew file mode 100644index 0000000..92f240f--- /dev/null+++ b/Transit/TransitTests/MCPCreateTaskAtomicityTests.swift@@ -0,0 +1,43 @@+#if os(macOS)+import Foundation+import SwiftData+import Testing+@testable import Transit++@MainActor @Suite(.serialized)+struct MCPCreateTaskAtomicityTests {++    @Test func createTaskWithMilestoneSaveFailureIsAtomic() async throws {+        var expectedMilestoneID: UUID?+        let env = try MCPTestHelpers.makeEnv { context in+            let pendingTasks = try context.fetch(FetchDescriptor<TransitTask>())+            #expect(pendingTasks.count == 1)+            #expect(pendingTasks.first?.milestone?.id == expectedMilestoneID)+            throw SaveFailure.simulated+        }+        let project = MCPTestHelpers.makeProject(in: env.context)+        let milestone = try await env.milestoneService.createMilestone(+            name: "v1.0", description: nil, project: project+        )+        expectedMilestoneID = milestone.id++        let response = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "create_task",+            arguments: [+                "name": "Atomic Task",+                "type": "bug",+                "projectId": project.id.uuidString,+                "milestoneDisplayId": milestone.permanentDisplayId!+            ]+        ))++        #expect(try MCPTestHelpers.isError(response))+        #expect(try env.context.fetch(FetchDescriptor<TransitTask>()).isEmpty)+        try env.context.save()+        #expect(+            try env.context.fetch(FetchDescriptor<TransitTask>()).isEmpty,+            "A later save must not resurrect a failed aggregate create [T-1768]"+        )+    }+}+#endif

diff --git a/Transit/TransitTests/MCPTestHelpers.swift b/Transit/TransitTests/MCPTestHelpers.swiftindex d8f1fd5..ca2dba0 100644--- a/Transit/TransitTests/MCPTestHelpers.swift+++ b/Transit/TransitTests/MCPTestHelpers.swift@@ -1,106 +1,112 @@ #if os(macOS) import Foundation import SwiftData import Testing @testable import Transit  struct MCPTestEnv {     let handler: MCPToolHandler     let taskService: TaskService     let projectService: ProjectService     let commentService: CommentService     let milestoneService: MilestoneService     let maintenanceService: DisplayIDMaintenanceService     let mcpSettings: MCPSettings     let context: ModelContext }  @MainActor enum MCPTestHelpers { -    static func makeEnv() throws -> MCPTestEnv {+    static func makeEnv(+        taskCreateSave: @escaping (ModelContext) throws -> Void = { try $0.save() }+    ) throws -> MCPTestEnv {         let testContainer = try TestModelContainer()         let context = testContainer.context         let taskStore = InMemoryCounterStore()         let taskAllocator = DisplayIDAllocator(store: taskStore)-        let taskService = TaskService(modelContext: context, displayIDAllocator: taskAllocator)+        let taskService = TaskService(+            modelContext: context,+            displayIDAllocator: taskAllocator,+            createSave: taskCreateSave+        )         let projectService = ProjectService(modelContext: context)         let commentService = CommentService(modelContext: context)         let milestoneStore = InMemoryCounterStore()         let milestoneAllocator = DisplayIDAllocator(store: milestoneStore)         let milestoneService = MilestoneService(modelContext: context, displayIDAllocator: milestoneAllocator)         let maintenanceService = DisplayIDMaintenanceService(             modelContext: context,             taskAllocator: taskAllocator,             milestoneAllocator: milestoneAllocator,             commentService: commentService         )         let mcpSettings = MCPSettings()         // Default to off so existing tests don't see maintenance tools unless they opt in.         mcpSettings.maintenanceToolsEnabled = false         let handler = MCPToolHandler(             taskService: taskService, projectService: projectService,             commentService: commentService, milestoneService: milestoneService,             maintenanceService: maintenanceService, settings: mcpSettings         )         return MCPTestEnv(             handler: handler,             taskService: taskService,             projectService: projectService,             commentService: commentService,             milestoneService: milestoneService,             maintenanceService: maintenanceService,             mcpSettings: mcpSettings,             context: context         )     }      @discardableResult     static func makeProject(         in context: ModelContext, name: String = "Test Project", gitRepo: String? = nil     ) -> Project {         let project = Project(name: name, description: "A test project", gitRepo: gitRepo, colorHex: "#FF0000")         context.insert(project)         return project     }      static func request(method: String, id: Int = 1, params: [String: Any]? = nil) -> JSONRPCRequest {         let paramsValue = params.map { AnyCodable($0) }         return JSONRPCRequest(jsonrpc: "2.0", id: .integer(id), method: method, params: paramsValue)     }      static func toolCallRequest(tool: String, arguments: [String: Any], id: Int = 1) -> JSONRPCRequest {         request(method: "tools/call", id: id, params: ["name": tool, "arguments": arguments])     }      static func decodeResult(_ response: JSONRPCResponse?) throws -> [String: Any] {         let unwrapped = try #require(response, "Expected a JSON-RPC response but got nil")         let data = try JSONEncoder().encode(unwrapped)         let json = try JSONSerialization.jsonObject(with: data) as? [String: Any]         let result = try #require(json?["result"] as? [String: Any])         let content = try #require(result["content"] as? [[String: Any]])         let text = try #require(content.first?["text"] as? String)         let textData = try #require(text.data(using: .utf8))         return try #require(try JSONSerialization.jsonObject(with: textData) as? [String: Any])     }      static func decodeArrayResult(_ response: JSONRPCResponse?) throws -> [[String: Any]] {         let unwrapped = try #require(response, "Expected a JSON-RPC response but got nil")         let data = try JSONEncoder().encode(unwrapped)         let json = try JSONSerialization.jsonObject(with: data) as? [String: Any]         let result = try #require(json?["result"] as? [String: Any])         let content = try #require(result["content"] as? [[String: Any]])         let text = try #require(content.first?["text"] as? String)         let textData = try #require(text.data(using: .utf8))         return try #require(try JSONSerialization.jsonObject(with: textData) as? [[String: Any]])     }      static func isError(_ response: JSONRPCResponse?) throws -> Bool {         let unwrapped = try #require(response, "Expected a JSON-RPC response but got nil")         let data = try JSONEncoder().encode(unwrapped)         let json = try JSONSerialization.jsonObject(with: data) as? [String: Any]         let result = try #require(json?["result"] as? [String: Any])         return result["isError"] as? Bool == true     }      static func errorText(_ response: JSONRPCResponse?) throws -> String {

diff --git a/specs/bugfixes/create-with-milestone-cleanup-not-atomic/report.md b/specs/bugfixes/create-with-milestone-cleanup-not-atomic/report.mdnew file mode 100644index 0000000..5b419eb--- /dev/null+++ b/specs/bugfixes/create-with-milestone-cleanup-not-atomic/report.md@@ -0,0 +1,100 @@+# Bugfix Report: Create-with-milestone cleanup is not atomic++**Date:** 2026-08-01+**Status:** Fixed++## Description of the Issue++MCP `create_task`, `CreateTaskIntent`, and `AddTaskSheet.persist` persisted a new task before assigning its optional milestone in a second save. If milestone assignment failed, each caller attempted a compensating task deletion. That deletion also required a save and could fail for the same storage reason, leaving a task persisted even though creation reported failure.++**Reproduction steps:**+1. Create a task with a valid milestone through MCP, CreateTaskIntent, or AddTaskSheet.+2. Allow the task's first save to succeed.+3. Make the milestone-assignment save fail, then make the compensating deletion save fail.+4. Observe that the caller reports failure while the task from the first save remains persisted without the requested aggregate state.++**Impact:** Failed create retries could produce hidden tasks or duplicates because the caller could not distinguish the persisted partial create from a fully rejected operation.++## Investigation Summary++- **Symptoms examined:** All three create surfaces used pre-validation followed by task save, milestone save, and best-effort deletion.+- **Code inspected:** `TaskService.createTask`, `MilestoneService.setMilestone`, `MCPToolHandler.handleCreateTask`, `CreateTaskIntent.execute`, `AddTaskSheet.persist`, and existing save-failure helpers/tests.+- **Hypotheses tested:** Milestone pre-validation prevents lookup and project-mismatch orphans, but cannot make two persistence operations atomic. `insertOrDelete` correctly removes a newly inserted task after a failed first save, while compensating deletion cannot undo an already committed save when its own save fails.++## Discovered Root Cause++Aggregate creation was split across service boundaries: `TaskService.createTask` inserted and saved the task, then each surface called `MilestoneService.setMilestone`, which mutated and saved again. Error recovery was therefore a third persistence operation rather than cleanup of one failed transaction.++**Defect type:** Non-atomic composite persistence operation++**Why it occurred:** Earlier fixes added milestone pre-validation and compensating deletion, addressing validation failures but retaining a create-then-update sequence. SwiftData's shared context and fallible saves mean a later delete cannot guarantee reversal of the first committed save.++**Contributing factors:** The create save seam was only available as a per-call parameter on `TaskService.createTask`, so the three surface entry points could not test whether a milestone was attached before their persistence boundary.++## Resolution for the Issue++**Changes made:**+- `Transit/Transit/Services/TaskService.swift` — Added optional milestone aggregate creation, project validation, and an initializer-injected create-save seam. The task relationship is attached before `insertOrDelete` performs the only save.+- `Transit/Transit/MCP/MCPToolHandler.swift` — Passes the pre-resolved milestone to `TaskService.createTask`; removed the second save and best-effort task deletion.+- `Transit/Transit/Intents/CreateTaskIntent.swift` — Uses the same aggregate create path; removed milestone assignment and compensating deletion.+- `Transit/Transit/Views/AddTask/AddTaskSheet+Save.swift` — Delegates task plus selected milestone to `TaskService` in one call.++**Approach rationale:** Constructing the complete aggregate before insertion lets SwiftData persist the task and relationship in one save. The existing `insertOrDelete` helper removes the inserted task when that save fails, so no already-committed task needs a second fallible operation to reverse it.++**Alternatives considered:**+- Keep compensating deletion and retry it — rejected because retries cannot make two independent commits atomic and can still leave partial data.+- Roll back the shared context — rejected because creation uses `insertOrDelete` by design; context-wide rollback can discard unrelated edits and does not reliably re-fault new models (T-452/T-486).++## Regression Test++**Test files:**+- `Transit/TransitTests/MCPCreateTaskAtomicityTests.swift`+- `Transit/TransitTests/CreateTaskIntentMilestoneTests.swift`+- `Transit/TransitTests/AddTaskSheetSaveErrorTests.swift`++**Test names:**+- `createTaskWithMilestoneSaveFailureIsAtomic`+- `persistWithMilestoneIsAtomicWhenCreateSaveFails`++**What they verify:** Each surface reaches one task-creation save with the requested milestone already attached. When that save seam throws, no task remains in the context or appears after a later unrelated save.++**Run command:** `make test-quick`++## Affected Files++| File | Change |+|------|--------|+| `Transit/Transit/Services/TaskService.swift` | Central aggregate create, milestone validation, injectable create-save seam |+| `Transit/Transit/MCP/MCPToolHandler.swift` | Route MCP create through aggregate persistence |+| `Transit/Transit/Intents/CreateTaskIntent.swift` | Route App Intent create through aggregate persistence |+| `Transit/Transit/Views/AddTask/AddTaskSheet+Save.swift` | Route UI create through aggregate persistence |+| `Transit/TransitTests/MCPCreateTaskAtomicityTests.swift` | MCP save-failure regression |+| `Transit/TransitTests/CreateTaskIntentMilestoneTests.swift` | App Intent save-failure regression |+| `Transit/TransitTests/AddTaskSheetSaveErrorTests.swift` | Add Task sheet save-failure regression |+| `Transit/TransitTests/MCPTestHelpers.swift` | Injectable MCP task-create save fixture |++## Verification++**Automated:**+- [x] Red phase confirmed: `make test-quick` failed before implementation because the required aggregate create seam did not exist.+- [x] Regression and macOS unit suite pass: `make test-quick`.+- [x] iOS and macOS builds pass: `make build`.+- [x] SwiftLint and SwiftData ownership guard pass: `make lint`.+- [ ] Full iOS simulator suite: `make test` compiled the changed code/tests but timed out while Xcode repeatedly failed to establish simulator/device debugger services (`DTDKRemoteDeviceConnection` secure-connection failures and missing LLDB version). No source test failure was reported before the environment timeout.+- [ ] UI suite: `make test-ui` was retried separately with a five-minute bound and hit the same `com.apple.mobile.notification_proxy` secure-connection and missing-LLDB failures before timing out.++**Manual verification:**+- Confirmed all three create surfaces pass the optional milestone into `TaskService.createTask`.+- Confirmed the create-specific `try? taskService.deleteTask(task)` paths were removed.++## Prevention++- Treat a task plus its requested initial relationships as one aggregate insert.+- Expose persistence failure seams at the service boundary used by real surfaces.+- Avoid compensating saves when the aggregate can be constructed before its first persistence operation.++## Related++- Transit T-1768+- T-240, T-558, and T-855: earlier pre-validation and compensating-deletion fixes+- T-452 and T-486: create-save failure cleanup requirements