PR review: #226 — Fix T-1675 scoped milestone lookup errors

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex acdc0b0..c15eae7 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -1,18 +1,20 @@ # 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] +- T-1675: Project-scoped milestone-name lookups now preserve the exact SwiftData fetch failure through shared assignment as `INTERNAL_ERROR` (`Failed to look up milestone: <error>`), matching JSON/MCP create, update, and scoped-query adapters. The milestone service's injected fetcher is now available in MCP test setup; deterministic regressions prove no-match and ambiguity remain distinct, cross-project unscoped filtering is untouched, and failed lookups create or mutate nothing.+ - T-1657: Project lookup storage failures now remain distinguishable from missing projects across JSON intents, MCP create/query/milestone tools, and visual Add Task. `QueryMilestonesIntent` no longer returns a successful empty array when its project-name lookup is unreadable, while valid missing/ambiguous names and project-ID precedence retain their prior behavior. Visual Add Task now reports `INTERNAL_ERROR` for a failed project read rather than a stale-selection `PROJECT_NOT_FOUND`; deterministic failing-fetch regressions verify cross-surface parity and no task or milestone insertion. - T-1800: Open task details now observe a task-scoped SwiftData comment query instead of a one-time snapshot, so MCP and local insertions/deletions update both the visible comment list and Share export without reopening the view. The query filters through the CloudKit-compatible optional child relationship and uses creation date plus UUID for stable ordering.  - T-1770: JSON mutation intents now preserve their established typed validation and domain-error payloads while mapping untyped SwiftData fetch/save failures to `INTERNAL_ERROR`. Shared task and milestone identifier resolution uses deterministic injected fetchers; milestone mutations also expose an injected saver. Regression coverage spans create, status update, milestone update, and deletion paths, asserting both the internal-error envelope and no persisted mutation on failure.  - T-1803: `UpdateStatusIntent` now includes a missing requested `displayId` in its `TASK_NOT_FOUND` hint (`No task with displayId N`), while missing UUIDs retain the existing generic lookup hint and malformed identifiers, duplicate IDs, status validation, and atomic mutation behavior remain unchanged. ### Fixed - T-1628: `make test-quick` and every relevant source-building Makefile target now keep Clang module caches and SwiftPM manifest modules/diagnostics workspace-local. `CLANG_MODULE_CACHE_PATH` is passed as an Xcode build setting, `SWIFTPM_MODULECACHE_OVERRIDE` is exported for manifest compilation, and the cache guard now asserts these supported controls while explicitly preserving the intentional omission of `-clonedSourcePackagesDirPath` and `-packageCachePath` so package resolution continues to work. Xcode recipes also explicitly enable `pipefail`, preventing `xcbeautify` from masking build failures on macOS's bundled GNU Make 3.81. - T-1613: MCP `query_tasks` now returns the exact tool error `Failed to fetch comments: <error>` when comment serialization cannot read storage, rather than reporting a successful task with `comments: []`. The detailed display-ID and task-list paths share this throwing serialization boundary; genuine empty comment collections retain their successful `comments: []` response. `update_task_status` continues serializing the `Comment` returned by its atomic mutation directly (T-1823), so it performs no post-commit comment fetch that could prompt a retry or duplicate a persisted comment. Deterministic MCP regressions cover both query errors, legitimate empty comments, and zero status-response fetches. - T-1620: `GenerateReportIntent` now returns the established `INTERNAL_ERROR` JSON envelope with a source-specific stable hint when terminal task or milestone fetches fail, instead of false empty-report Markdown. Successful empty reports retain their existing Markdown and date-range formatting; deterministic regressions cover both failures and the valid-empty response.

diff --git a/Transit/Transit/Intents/IntentHelpers.swift b/Transit/Transit/Intents/IntentHelpers.swiftindex 1b6ff58..55ba209 100644--- a/Transit/Transit/Intents/IntentHelpers.swift+++ b/Transit/Transit/Intents/IntentHelpers.swift@@ -475,15 +475,15 @@ nonisolated enum IntentHelpers {             }             try milestoneService.setMilestone(milestone, on: task)             return nil         } catch MilestoneService.Error.ambiguousName {             return IntentError.ambiguousMilestone(                 hint: "Multiple milestones named '\(name)' exist in project '\(project.name)'"             ).json         } catch let error as MilestoneService.Error {             return mapMilestoneError(error).json         } catch {-            return IntentError.internalError(hint: "Failed to assign milestone").json+            return IntentError.internalError(hint: "Failed to look up milestone: \(error)").json         }     } } // swiftlint:enable type_body_length file_length

diff --git a/Transit/TransitTests/MCPTestHelpers.swift b/Transit/TransitTests/MCPTestHelpers.swiftindex 1938007..66d32f0 100644--- a/Transit/TransitTests/MCPTestHelpers.swift+++ b/Transit/TransitTests/MCPTestHelpers.swift@@ -18,37 +18,42 @@ struct MCPTestEnv { @MainActor enum MCPTestHelpers {      static func makeEnv(         taskCreateSave: @escaping (ModelContext) throws -> Void = { try $0.save() },         taskStatusSave: @escaping (ModelContext) throws -> Void = { try $0.save() },         projectFetcher: (any ModelFetching)? = nil,         taskFetcher: (any TaskFetching)? = nil,         commentFetcher: (any CommentFetching)? = nil,         milestoneFetcher: (any MilestoneFetching)? = nil,-        milestoneDisplayIDFinder: (any MilestoneDisplayIDFinding)? = nil+        milestoneDisplayIDFinder: (any MilestoneDisplayIDFinding)? = nil,+        milestoneServiceFetcher: (any ModelFetching)? = nil     ) 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,             createSave: taskCreateSave,             statusSave: taskStatusSave         )         let projectService = ProjectService(modelContext: context, fetcher: projectFetcher)         let commentService = CommentService(modelContext: context)         let milestoneStore = InMemoryCounterStore()         let milestoneAllocator = DisplayIDAllocator(store: milestoneStore)-        let milestoneService = MilestoneService(modelContext: context, displayIDAllocator: milestoneAllocator)+        let milestoneService = MilestoneService(+            modelContext: context,+            displayIDAllocator: milestoneAllocator,+            fetcher: milestoneServiceFetcher+        )         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(

diff --git a/Transit/TransitTests/MilestoneNameLookupStorageFailureSurfaceTests.swift b/Transit/TransitTests/MilestoneNameLookupStorageFailureSurfaceTests.swiftnew file mode 100644index 0000000..26bc4e0--- /dev/null+++ b/Transit/TransitTests/MilestoneNameLookupStorageFailureSurfaceTests.swift@@ -0,0 +1,151 @@+#if os(macOS)+import Foundation+import SwiftData+import Testing+@testable import Transit++/// T-1675: a failed project-scoped milestone name lookup must stay distinct from+/// a genuine no-match. JSON surfaces return the exact INTERNAL_ERROR envelope;+/// MCP returns the exact tool error; no create or update may mutate data.+@MainActor @Suite(.serialized)+struct MilestoneNameLookupFailureTests {++    private struct FetchFailure: Swift.Error, CustomStringConvertible {+        var description: String { "simulated milestone name lookup fetch failure" }+    }++    private struct FailingMilestoneFetcher: ModelFetching {+        func fetch<T: PersistentModel>(_ descriptor: FetchDescriptor<T>) throws -> [T] {+            throw FetchFailure()+        }+    }++    private func allocator() -> DisplayIDAllocator {+        DisplayIDAllocator(store: InMemoryCounterStore())+    }++    private func makeProject(in context: ModelContext) -> Project {+        let project = Project(name: "Transit", description: "", gitRepo: nil, colorHex: "#000000")+        context.insert(project)+        return project+    }++    private func makeTask(in context: ModelContext, project: Project) -> TransitTask {+        let task = TransitTask(name: "Existing", type: .feature, project: project, displayID: .permanent(1))+        StatusEngine.initializeNewTask(task)+        context.insert(task)+        return task+    }++    private func makeMilestone(in context: ModelContext, project: Project) -> Milestone {+        let milestone = Milestone(name: "Sprint", description: nil, project: project, displayID: .permanent(1))+        context.insert(milestone)+        return milestone+    }++    private func expectInternalError(_ result: String, hint: String) throws {+        let data = try #require(result.data(using: .utf8))+        let payload = try #require(try JSONSerialization.jsonObject(with: data) as? [String: Any])+        #expect(Set(payload.keys) == Set(["error", "hint"]))+        #expect(payload["error"] as? String == "INTERNAL_ERROR")+        #expect(payload["hint"] as? String == hint)+    }++    @Test func findByNamePropagatesInjectedFetchFailureInsteadOfReturningNoMatch() throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let project = makeProject(in: context)+        let service = MilestoneService(+            modelContext: context,+            displayIDAllocator: allocator(),+            fetcher: FailingMilestoneFetcher()+        )++        do {+            _ = try service.findByName("Sprint", in: project)+            Issue.record("Expected the injected milestone fetch to fail")+        } catch {+            #expect(String(describing: error) == "simulated milestone name lookup fetch failure")+        }+    }++    @Test func jsonCreateAndUpdatePathsReturnExactLookupErrorWithoutMutation() async throws {+        let testContainer = try TestModelContainer()+        let context = testContainer.context+        let project = makeProject(in: context)+        let task = makeTask(in: context, project: project)+        let milestone = makeMilestone(in: context, project: project)+        try context.save()++        let taskService = TaskService(modelContext: context, displayIDAllocator: allocator())+        let projectService = ProjectService(modelContext: context)+        let milestoneService = MilestoneService(+            modelContext: context,+            displayIDAllocator: allocator(),+            fetcher: FailingMilestoneFetcher()+        )+        let expectedHint = "Failed to look up milestone: simulated milestone name lookup fetch failure"++        let createResult = await CreateTaskIntent.execute(+            input: "{\"projectId\":\"\(project.id.uuidString)\",\"name\":\"New\","+                + "\"type\":\"feature\",\"milestone\":\"Sprint\"}",+            taskService: taskService,+            projectService: projectService,+            milestoneService: milestoneService+        )+        let updateTaskResult = UpdateTaskIntent.execute(+            input: "{\"displayId\":1,\"milestone\":\"Sprint\"}",+            taskService: taskService,+            milestoneService: milestoneService+        )+        let updateMilestoneResult = UpdateMilestoneIntent.execute(+            input: "{\"name\":\"Sprint\",\"projectId\":\"\(project.id.uuidString)\",\"description\":\"Changed\"}",+            milestoneService: milestoneService,+            projectService: projectService+        )+        let assignmentResult = IntentHelpers.assignMilestone(+            from: ["milestone": "Sprint"], to: task, milestoneService: milestoneService+        )++        try expectInternalError(createResult, hint: expectedHint)+        try expectInternalError(updateTaskResult, hint: expectedHint)+        try expectInternalError(updateMilestoneResult, hint: expectedHint)+        try expectInternalError(try #require(assignmentResult), hint: expectedHint)+        #expect(try context.fetch(FetchDescriptor<TransitTask>()).count == 1)+        #expect(task.name == "Existing")+        #expect(task.milestone == nil)+        #expect(milestone.milestoneDescription == nil)+    }++    @Test func mcpCreateUpdateAndScopedQueryReturnExactLookupErrorWithoutMutation() async throws {+        let env = try MCPTestHelpers.makeEnv(milestoneServiceFetcher: FailingMilestoneFetcher())+        let project = MCPTestHelpers.makeProject(in: env.context, name: "Transit")+        let task = try await env.taskService.createTask(+            name: "Existing", description: nil, type: .feature, project: project+        )+        let taskDisplayId = try #require(task.permanentDisplayId)+        let expectedHint = "Failed to look up milestone: simulated milestone name lookup fetch failure"++        let createResponse = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "create_task",+            arguments: ["projectId": project.id.uuidString, "name": "New", "type": "feature", "milestone": "Sprint"]+        ))+        let updateResponse = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "update_task",+            arguments: ["displayId": taskDisplayId, "milestone": "Sprint"]+        ))+        let queryResponse = await env.handler.handle(MCPTestHelpers.toolCallRequest(+            tool: "query_tasks",+            arguments: ["projectId": project.id.uuidString, "milestone": "Sprint"]+        ))++        for response in [createResponse, updateResponse, queryResponse] {+            #expect(try MCPTestHelpers.isError(response))+            #expect(try MCPTestHelpers.errorText(response) == expectedHint)+        }+        #expect(try env.context.fetch(FetchDescriptor<TransitTask>()).count == 1)+        #expect(task.name == "Existing")+        #expect(task.milestone == nil)+    }+}+#endif

diff --git a/Transit/TransitTests/ProjectLookupStorageFailureSurfaceTests.swift b/Transit/TransitTests/ProjectLookupStorageFailureSurfaceTests.swiftindex 7b34e7b..0f25a4d 100644--- a/Transit/TransitTests/ProjectLookupStorageFailureSurfaceTests.swift+++ b/Transit/TransitTests/ProjectLookupStorageFailureSurfaceTests.swift@@ -70,20 +70,21 @@ struct ProjectLookupStorageFailureSurfaceTests {             milestoneService: MilestoneService(modelContext: context, displayIDAllocator: allocator()),             projectService: projectService         )          try expectInternalError(taskResult, hint: "Failed to fetch project: simulated project fetch failure")         try expectInternalError(milestoneResult, hint: "Failed to fetch projects: simulated project fetch failure")         #expect(try context.fetch(FetchDescriptor<TransitTask>()).isEmpty)         #expect(try context.fetch(FetchDescriptor<Milestone>()).isEmpty)     } +#if os(macOS)     @Test func mcpCreateAndQueryPathsReturnStorageErrorsWithoutInsertionWhenProjectLookupFails() async throws {         let env = try MCPTestHelpers.makeEnv(projectFetcher: FailingProjectFetcher())         let projectID = UUID().uuidString          let createTask = await env.handler.handle(MCPTestHelpers.toolCallRequest(             tool: "create_task",             arguments: ["projectId": projectID, "name": "Task", "type": "feature"]         ))         let queryTasks = await env.handler.handle(MCPTestHelpers.toolCallRequest(             tool: "query_tasks",@@ -103,20 +104,21 @@ struct ProjectLookupStorageFailureSurfaceTests {         }         let idLookupFailure = "Failed to fetch project: simulated project fetch failure"         let nameLookupFailure = "Failed to fetch projects: simulated project fetch failure"         #expect(try MCPTestHelpers.errorText(createTask) == idLookupFailure)         #expect(try MCPTestHelpers.errorText(queryTasks) == idLookupFailure)         #expect(try MCPTestHelpers.errorText(createMilestone) == idLookupFailure)         #expect(try MCPTestHelpers.errorText(queryMilestones) == nameLookupFailure)         #expect(try env.context.fetch(FetchDescriptor<TransitTask>()).isEmpty)         #expect(try env.context.fetch(FetchDescriptor<Milestone>()).isEmpty)     }+#endif      @Test func visualAddTaskReportsInternalStorageErrorWithoutInsertionWhenProjectLookupFails() async throws {         let testContainer = try TestModelContainer()         let context = testContainer.context         let taskService = TaskService(modelContext: context, displayIDAllocator: allocator())         let projectService = ProjectService(modelContext: context, fetcher: FailingProjectFetcher())         let project = ProjectEntity(id: UUID().uuidString, projectId: UUID(), name: "Transit")          do {             _ = try await AddTaskIntent.execute(

diff --git a/specs/bugfixes/scoped-milestone-name-lookup-hides-fetch-failures/report.md b/specs/bugfixes/scoped-milestone-name-lookup-hides-fetch-failures/report.mdnew file mode 100644index 0000000..e293c95--- /dev/null+++ b/specs/bugfixes/scoped-milestone-name-lookup-hides-fetch-failures/report.md@@ -0,0 +1,103 @@+# Bugfix Report: Scoped Milestone Name Lookup Hides Fetch Failures++**Date:** 2026-08-05+**Status:** Fixed+**Ticket:** T-1675++## Description of the Issue++A project-scoped milestone name lookup must distinguish three outcomes: a valid no-match, a CloudKit-induced duplicate name, and an unreadable SwiftData store. The service already exposes that distinction through its throwing lookup and injected fetcher, but one shared assignment adapter collapsed an untyped storage error to the generic `"Failed to assign milestone"` hint. That prevented automation from receiving the exact retryable storage failure carried by the corresponding JSON and MCP create, update, and query paths.++**Reproduction steps:**+1. Construct `MilestoneService` with a deterministic `ModelFetching` implementation that throws.+2. Submit a project-scoped milestone-name lookup through the shared assignment path.+3. Observe a generic internal error instead of the underlying lookup failure.++**Impact:** Automation could not reliably identify the specific failed read or correlate retries across surfaces. A failure must not be mistaken for a normal missing milestone, ambiguity, validation error, or a completed mutation.++## Investigation Summary++### Phase 1 — Initial overview++The expected contract is `INTERNAL_ERROR` with the exact `Failed to look up milestone: <storage error>` hint for JSON intents and the same text in an MCP tool error. Valid no-match, ambiguity, project mismatch, identifier/field validation precedence, and unscoped cross-project query behavior must remain unchanged.++The focused macOS regression suite was red before the correction:++```text+MilestoneNameLookupFailureTests.jsonCreateAndUpdatePathsReturnExactLookupErrorWithoutMutation()+```++### Phase 2 — Systematic inspection++- `MilestoneService.findByName(_:in:)` already uses its injected `ModelFetching` seam. It returns `nil` only for a genuine no-match, throws `.ambiguousName` for duplicate project-scoped names, and propagates an infrastructure fetch error.+- `CreateTaskIntent`, `TaskUpdateValidator`, `IntentHelpers.resolveMilestone` (used by `UpdateMilestoneIntent`), MCP `create_task`, and MCP project-scoped `query_tasks` already translate an unexpected lookup failure as `Failed to look up milestone: <error>`.+- `IntentHelpers.assignMilestone(named:to:using:)` instead converted its generic catch to `INTERNAL_ERROR` with only `Failed to assign milestone`, losing the original failure text.+- T-1657 supplies the applicable surface-adapter pattern; T-1770 supplies the generic persistence classification and injected `ModelFetching` seam. Both commits are ancestors of this branch.++### Phase 3 — Root cause analysis++1. Why was the exact failure lost? The shared assignment helper used a generic fallback hint.+2. Why did other paths remain correct? Their generic catches retained the lookup-specific error text.+3. Why is that distinction required? A failing fetch is infrastructure state, not a missing milestone or invalid request.+4. Why was it deterministic to reproduce? T-1770 had already routed milestone name reads through an injectable fetcher, but MCP test setup did not expose that seam.++**Root cause:** one shared error adapter did not preserve the storage-error payload already carried by `MilestoneService.findByName(_:in:)`.++## Resolution for the Issue++**Changes made:**+- `Transit/Transit/Intents/IntentHelpers.swift` — maps an untyped name-lookup failure in shared assignment to `INTERNAL_ERROR` with the exact `Failed to look up milestone: <error>` hint.+- `Transit/TransitTests/MCPTestHelpers.swift` — exposes the existing `MilestoneService` fetcher injection through MCP test setup.+- `Transit/TransitTests/MilestoneNameLookupStorageFailureSurfaceTests.swift` — adds deterministic service, JSON create/update, shared assignment, and MCP create/update/project-scoped-query regressions with no-mutation assertions.++**Approach rationale:** The smallest safe change keeps the stable service contract and all existing typed error mappings. It aligns the one inconsistent adapter with T-1657/T-1770 without changing missing-name, ambiguity, project mismatch, validation precedence, or unscoped cross-project filtering.++**Alternatives considered:**+- Add a new service error case for storage failures — rejected because the raw fetch error already provides the required distinction and existing callers classify generic infrastructure errors as `INTERNAL_ERROR`.+- Change unscoped task queries to resolve a scoped milestone first — rejected because it would break the established cross-project same-name aggregation behavior.++## Regression Test++**Test file:** `Transit/TransitTests/MilestoneNameLookupStorageFailureSurfaceTests.swift`++**Tests:**+- `findByNamePropagatesInjectedFetchFailureInsteadOfReturningNoMatch`+- `jsonCreateAndUpdatePathsReturnExactLookupErrorWithoutMutation`+- `mcpCreateUpdateAndScopedQueryReturnExactLookupErrorWithoutMutation`++**What it verifies:** The deterministic fetch seam stays distinct from a no-match and yields the exact source error in every scoped JSON/MCP create, update, and query path while task and milestone state remain unchanged.++**Run command:** `make test-quick`++## Affected Files++| File | Change |+|---|---|+| `Transit/Transit/Intents/IntentHelpers.swift` | Preserve exact storage error text for shared name-based assignment. |+| `Transit/TransitTests/MCPTestHelpers.swift` | Allow MCP tests to inject the milestone service fetch seam. |+| `Transit/TransitTests/MilestoneNameLookupStorageFailureSurfaceTests.swift` | Add deterministic cross-surface error and no-mutation coverage. |+| `CHANGELOG.md` | Record the corrected automation failure contract. |++## Verification++**Automated:**+- [x] New focused regression suite failed before the fix.+- [x] Focused milestone lookup, task create/update, and MCP project-query suites pass after the fix.+- [x] Full macOS unit suite passes (`make test-quick`).+- [x] Linter and ownership guard pass (`make lint`).+- [x] iOS Simulator build passes (`make build-ios`).++**Manual verification:** Not required; deterministic in-memory SwiftData tests exercise the exact service and automation boundaries.++## Prevention++- Keep `nil` reserved for a valid scoped no-match; let `findByName(_:in:)` propagate storage failures and `.ambiguousName`.+- Catch typed domain errors before generic infrastructure errors, and preserve a source-specific lookup hint for the latter.+- Expose existing service seams through cross-surface test fixtures so an MCP adapter cannot silently diverge.++## Related++- T-1675+- T-1657 — project lookup storage-failure surface adapters+- T-1770 — persistence error classification and injected lookup seams+- T-1608 — unscoped MCP milestone-name fetch failures