💭 I see that the previously flagged standalone ProjectMcpServiceShape interface has now been moved inline into Context.Service, resolving that issue. I'm now checking the imports in ProjectMcpService.ts for convention compliance, noting the subpath namespace imports and named contract imports look correct so far. I'm noticing that ThreadManagementService is imported as a named import rather than as a namespace, unlike sibling files that consistently use namespace imports for local service modules like ProjectService and ServerSettings. I want to verify whether this inconsistency is actually a convention violation by checking how the module exports its layer and how it's used elsewhere, like in the test file. I'm also considering the ProjectMcpFailure error class, which stores a free-form message string alongside a code field rather than deriving the message from structured attributes as the convention seems to prefer. The failure helper deriving its message from errorMessage(error) (the underlying cause) directly violates the rule against deriving messages from cause or cause.message, and the helper itself looks like the flagged trivial wrapper pattern that just constructs an error from its arguments. I should check whether sibling MCP services like WorktreeMcpService use this same pattern — if it's an established convention repo-wide, that changes how I'd flag it versus treating it as isolated new-code debt. Let me look at those sibling files to compare.

1. browse_code — apps/server/src/mcp/**/*.ts

2. browse_code — apps/server/src/mcp/ProjectMcpService.ts

💭 I notice make isn't exported here, which seems worth checking against convention—modules that own construction should export a real make. I want to compare this against WorktreeMcpService.ts to see if there's an existing failure type following the same pattern, which would confirm this code is consistent with established repo conventions.

3. browse_code — apps/server/src/mcp/WorktreeMcpService.ts

4. browse_code — apps/server/src/mcp/toolkits/project/handlers.ts

💭 I notice the new ProjectMcpService closely mirrors WorktreeMcpService's existing patterns, so flagging these as issues would mean flagging conventions already established elsewhere in the repo. I'm weighing whether the instructions to prioritize authoritative guidance over legacy code should override the guidance to avoid flagging unrelated style preferences. For the failure() helper, it does match the exact anti-pattern the guidelines call out—a trivial wrapper constructing an error object—even though it mirrors the sibling file precisely. For the error message concern, I'm noting that ProjectMcpFailure has no cause field at all, so the rule about not deriving messages from cause.message doesn't actually apply here since the message is the entire payload for the MCP tool contract.

5. git_tools — WorktreeMcpFailure|OrchestrationMcpFailure|McpFailure extends

💭 The MCP failure contract with code and message pairs is already an established pattern across three surfaces, so I won't flag it. Now I'm scanning the PR for real issues against conventions—checking the ThreadManagementService import in ProjectMcpService.ts, which mirrors the sibling WorktreeMcpService, and the corresponding test file imports, which seem to follow the same established pattern too. The other candidates I'm weighing: the cross-domain import of makeKeyedSerialExecutor from orchestration-v2 into SourceControlRepositoryService feels architecturally odd but isn't on the convention list, and the new withProjectMutationLock method along with the ProjectService changes both look fine. Let me grep for aliased layer imports in the test files to confirm whether this is a wider pattern.

6. git_tools — import {[^}]*layer as

7. browse_code — apps/server/src/orchestration-v2/KeyedSerialExecutor.ts

💭 The layer as X alias pattern is widespread throughout the legacy codebase, and the new test file just follows existing convention. Since I shouldn't demand unrelated repo-wide cleanup, and this is a test-only import alias, I think flagging it would just be noise rather than something worth raising. Moving to other changed code, I'm checking the ThreadManagementService interface changes, but the standalone shape interface was pre-existing so the PR only added a method—not something introduced here. I'm also looking at a cross-layer import of a helper function in SourceControlRepositoryService, but since it's a pure utility rather than a service, it doesn't clearly violate the layering rules, so I'll skip it. The error handling usage with a pre-existing error class also looks fine as-is. I'm also examining whether tag-based dispatch inside mapError calls in the MCP services violates the tagged-failure catching convention, but since it's a translation rather than recovery logic, and the sibling service follows the same pattern, I'll let it go. Now I'm checking whether these MCP service modules properly export a make function for construction, comparing WorktreeMcpService's pattern to see if it exports one accordingly.

8. git_tools — ^export const (make|layer)

9. git_tools — const make = Effect.gen

💭 The non-exported make pattern is the local convention here, so that's marginal and I'd skip flagging it. ProjectMcpService closely mirrors its sibling, so I'll move on to check other changed files, starting with how ThreadLaunchService now wraps the claim logic under the project mutation lock. Checking through the remaining files — dispatch logic, websocket changes, and contract schemas for ProjectMcp — everything looks consistent with existing patterns, just field spreads and optional schema fields that aren't convention violations. The more interesting question is in the test file: whether ProjectMcpService's tests are mocking out core domain logic like ProjectService rather than just external services, which would violate the testing guideline about not mocking core business logic. I notice the test file imports ThreadManagementService using the exact aliasing pattern I'd flagged before — import { layer as threadManagementLayer, ThreadManagementService } — which erases the module namespace at a service boundary. Since this is newly added code rather than pre-existing repo-wide convention, I'm weighing whether to flag it despite the guidance not to demand unrelated cleanup, since this instance is directly part of what's being reviewed.

10. browse_code — apps/server/src/mcp/ProjectMcpService.test.ts

💭 I've confirmed lines 38-39 double-import the same module using both a named import and a namespace import, violating the convention of using a single namespace import like ProjectMcp.ProjectMcpService. This is a small but clear newly-introduced issue worth flagging with a suggestion, and I'm also checking the aliased layer import around lines 28-31 for a similar pattern. Given that the layer as X alias appears 60+ times across the repo as a dominant convention, I'll skip flagging it here to avoid noise inconsistent with the codebase, and just flag the unambiguous duplicate import instead. Now I want to check the changed production files for deeper runtime concerns like dependency acquisition or boundary issues. I'm scanning through the full list of touched files — server, service, toolkit, orchestrator, projection, and client-runtime files — to see which ones need closer inspection. Let me look at how toolkits/project/tools.ts imports ProjectMcpService and the invocation context namespace, and compare it against worktree/tools.ts for consistency.

11. browse_code — apps/server/src/mcp/toolkits/worktree/tools.ts

12. browse_code — apps/server/src/mcp/toolkits/worktree/handlers.ts

💭 The project toolkit matches the worktree, so the real new violation is the duplicate import in the test file, once named and once namespaced, which conflicts with using the module's public namespace shape and is just redundant. I'll flag that, then check ProjectService.ts and the other file for similar convention issues.

13. browse_code — apps/server/src/sourceControl/SourceControlRepositoryService.ts

💭 This file looks consistent, aside from a trivial blank line removal in the imports. I'm checking whether there's a dependency-acquisition issue with the keyed serial executor, and confirming that ProjectMcpService's layer is properly registered with its Crypto dependency in McpHttpServer alongside the T3ProjectFileLoader layer. My main finding so far is a redundant duplicate import of the service module in the test file, though that feels like a minor issue rather than a clear convention violation worth failing over. The more significant convention concern is around ProjectMcpFailure's error design — it builds its message from the cause rather than preserving the cause itself, which conflicts with the stated convention that wrapper errors should derive messages from stable structural attributes and retain the underlying cause when translating a real failure. I'm weighing whether this schema-encoded wire error is an exception to that rule. I'm concluding this pattern mirrors WorktreeMcpFailure and OrchestratorMcpFailure exactly, both already established across the codebase for MCP contracts returned to an LLM, so flagging it would mean demanding a redesign of an existing convention rather than catching a new violation — which conflicts with guidance not to require repo-wide cleanup. Similarly, the failure() factory helper duplicates code already present in WorktreeMcpService, so flagging it as new noise doesn't seem right either. Given the prior review already caught and fixed the one substantial issue, and this new code closely follows the sibling service's established patterns, I'm leaning toward reporting no violations, or at most a minor note about the duplicate import. duplicate import at lines 38-39 and lean toward marking it neutral rather than failing, since it's a minor redundancy in a test file. Re-examining the actual convention text, it does specify importing the local service module as a namespace and using its public shape like WorkspacePaths.make — the test does import the namespace correctly as ProjectMcp and use ProjectMcp.layer, but redundantly also named-imports the tag separately, which is a mild but real deviation rather than a false positive. I want to double check whether there's a more substantive issue I'm overlooking, so I'm going to look at the dispatch change in ThreadManagementService and ThreadLaunchService, and confirm whether the standalone ThreadManagementServiceShape interface retaining withProjectMutationLock is pre-existing rather than something newly introduced that violates the shape convention.

14. browse_code — apps/server/src/orchestration-v2/ThreadManagementService.ts

💭 The shape itself is pre-existing, and this PR just adds a member to it, so I won't flag it as repository-wide cleanup. I'll go ahead and post the one inline comment about the test's duplicate import since it's a legitimate small fix matching the convention, and I'll mark the check as failure since I do have a finding to report. Looking closer, the test genuinely imports the same module twice without consistently using it as a namespace, which does match the stated convention against erasing module namespace — so I'm confident flagging it. But the fix touches multiple usages of ProjectMcpService throughout the file (roughly eight places), so I can't offer a clean self-contained inline suggestion; I'll describe the fix and provide a unified diff instead. I'll draft the review comment for line 39, pointing out the double import and recommending keeping only the namespace import while updating call sites to reference it consistently, then post it as a normal PR comment rather than an approval or change request.

15. modify_pr — Effect service conventions: one minor import-hygiene issue in the new test module. The service definition itself (inline Context.Service interface, ProjectMcpService["Service"][...] annotations, namespaced module imports, make + layer order) follows the sibling WorktreeMcpService.ts conventions.