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

2. browse_code — apps/server/src/mcp/toolkits/project/tools.ts

💭 Let me check the sibling toolkits, like the environment toolkit, to see what conventions they follow.

3. browse_code — apps/server/src/mcp/toolkits/**/*.ts

4. browse_code — apps/server/src/mcp/threadAccess.ts

5. browse_code — apps/server/src/mcp/toolkits/environment/handlers.ts

6. browse_code — apps/server/src/mcp/toolkits/environment/tools.ts

💭 I'm comparing the new project toolkit against the environment toolkit, checking ProjectService and SourceControlRepositoryService for consistent create/update/delete signatures and error handling. I notice the import style for ProjectService uses a named import rather than a namespace import, which seems to conflict with the service-boundary convention, but I see the sibling environment tools file does the same thing, so I'm weighing whether this is actually an inconsistency worth flagging. Now checking handlers.ts, where ProjectService is imported as a namespace correctly, but I'm wondering if that Project name might collide with the Project schema from contracts. The bigger issue I'm spotting is the error handling pattern: mapping typed domain errors into a generic unavailable failure collapses distinct failure modes like "workspace already registered" or "nonempty project needs force=true" into an indistinguishable message, which contradicts the convention that failures should be mapped where their context is known rather than wrapped wholesale. I want to verify this against what ProjectService actually defines for its error types.

7. browse_code — In apps/server/src/project/ProjectService.ts, show the ProjectService Context.Service tag interface: the signatures and error types of snapshot, getById, create, update, and delete, including the tagged error classes they can fail with (e.g. workspace already registered, project not empty). Also show how SourceControlRepositoryService.cloneRepository is defined and its error types.

💭 I'm noticing that mapping errors generically on create/update/delete collapses specific domain errors like ProjectNotFoundError, ProjectConflictError, and ProjectNotEmptyError into one vague "operation could not be completed" message—this violates the principle of preserving structured domain errors at translation boundaries rather than wrapping entire pipelines in a single generic failure. I want to check how other handlers (thread, worktree, preview) handle this same mapping to see if they preserve specific error tags. I'm also scanning for smaller issues: whether mutation handlers duplicate archivedAt liveness checks unnecessarily, whether Schema.optional vs optionalKey usage is inconsistent, and whether constructing a ProjectId directly from a commandId string (which has a different format like "mcp:") is a correctness bug worth flagging separately from the Effect service conventions review.

8. browse_code — apps/server/src/mcp/toolkits/thread/handlers.ts

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

💭 Since this mapError pattern is used throughout the repo for MCP handlers, flagging it broadly would conflict with the rule against demanding repo-wide cleanup. But the convention is more nuanced than a blanket rule—it says to pass through structured domain errors at translation boundaries and only wrap unknown failures, mapping errors where context is actually known rather than wrapping whole pipelines generically. Here in the new create/update/delete code, distinct domain errors like ProjectNotFoundError, ProjectConflictError, and ProjectNotEmptyError all collapse into a generic "operation could not be completed" message, even though the tool descriptions explicitly promise specific behaviors to callers. There's precedent elsewhere (threadAccess.readThread mapping ThreadManagementThreadNotFoundError to thread_not_found) for discriminating known tags into specific codes, so this looks like a legitimate finding—the fix would use catchTags or a discriminating mapError. I also want to check whether the current mapError call drops the cause chain entirely, which would violate the convention that wrapped failures should preserve the underlying error as cause. Checking tools.ts, the shared dependencies array and handler-specific dependencies (like SourceControlRepositoryService for clone) line up correctly, and ThreadManagementService is properly included where needed. I'm also noticing an inconsistency in import style—handlers.ts uses namespace imports for service tags while tools.ts uses named imports, which seems to diverge from the boundary convention about namespace imports preserving the service's public shape. Since tools.ts follows the same named-import pattern as its sibling environment/tools.ts, I'll treat that as acceptable precedent rather than flagging it, since the check explicitly says not to demand universal namespace imports. I also notice the mutation logic duplicates an archived/runtimeMode check already present in thread handlers, and that ProjectId.make is being seeded from the commandId, which feels odd but isn't a convention violation, so it's out of scope for this review. Now I'm weighing whether the new backend behavior—six new MCP tools—triggers the test-coverage convention, since there don't appear to be new handler-level tests accompanying this significant change. I'll be selective and post the error-mapping finding as the strong one, with the missing-tests point as a secondary comment. Before finalizing, I should verify the exact shape of OrchestratorMcpFailure to make sure my suggested fix is precise.

10. browse_code — Show the definition of OrchestratorMcpFailure in packages/contracts (fields, code literals, whether it has a cause field). Also show any existing example in apps/server/src/mcp where a domain tagged error is discriminated (e.g. via error._tag or Effect.catchTags) and mapped to specific OrchestratorMcpFailure codes like invalid_request or conflict.

💭 There's clear precedent in the repo for keeping errors distinct rather than collapsing them. This new code merges ProjectNotFoundError, ProjectConflictError, and ProjectNotEmptyError into one generic orchestration_error, which breaks the convention of passing through structured domain errors at translation boundaries and only wrapping truly unknown failures. The clone operation shows the same pattern—collapsing SourceControlRepositoryError cases into a generic message loses information that should surface to callers. I'm also noting a second issue: the PR adds tests for registration and name uniqueness, but nothing exercises the handler behavior itself—pagination, capability gating, or the error mapping I just flagged. Since backend behavior changed, focused tests using test layers for external services should cover these paths too. I'm deciding this belongs in the review body rather than as an inline comment since it's not tied to specific lines, and it's a legitimate convention violation worth raising once, even if I want to avoid piling on general observations. For the inline comments, I'm settling on flagging the create/update/delete block in handlers.ts where ProjectService errors get mapped to unavailable, likely as one comment spanning that range rather than splitting across each operation. I'm sketching a projectFailure helper that maps ProjectNotFoundError, ProjectConflictError, and ProjectNotEmptyError to invalid_request and everything else to a generic orchestration_error, which fits since it does real normalization work rather than being a pass-through wrapper. I confirm ProjectServiceError is exported as a type from ProjectService.ts, so referencing it via the namespace import should work fine, though I'm still weighing whether Effect.catchTags applies here or if this mapping approach is the better fit. I confirm that within catchTags handlers, returning Effect.fail is the safe pattern since errors are yieldable but the handler itself needs to produce an Effect. Since the diff would touch three separate handlers, I'll likely skip providing a full diff and just describe the change, or offer just a single representative hunk. For the clone handler's similar issue with SourceControlRepositoryError collapsing distinct destination-exists/empty-path messages, I'm weighing whether to fold this into the same comment as the earlier finding since it's the same class of problem, or split it out since it's a different file and error type — leaning toward covering both in one broader comment. I'm settling on posting a single comment spanning the mutation handlers (create through clone, roughly lines 47-73) rather than splitting into two, since it's one convention violation repeated across those handlers, even though the range is a bit wide. Now I'm moving on to check other conventions in tools.ts, like the shared success type and named imports from the effect/unstable/ai module. McpInvocationContext and ThreadManagementService look properly declared, and the layer provisioning at McpHttpServer checks out fine too. The read-only allowlist change and shared presentation additions both seem like reasonable, non-convention-breaking updates, so I'm moving on to check service definition conventions next. Since OrchestratorMcpFailure has no cause field, there's nothing lost there. On the missing handler test coverage, I'll note it briefly in the review body rather than treat it as a blocker, but I'm now weighing whether the error-mapping issue — wrapping a whole multi-step pipeline instead of mapping failures where context is known — is significant enough to mark the review as failing rather than neutral. Actually, since this matches the established sibling pattern used across all the toolkit handlers uniformly, flagging it as a hard failure feels too harsh; I'll treat it as a non-blocking inline finding and set the review state to neutral instead. Now I'm drafting the actual inline comment text covering the create, update, and delete operations. Writing catchTags mapping... I should note missing test coverage in the review body but keep it to one sentence. I also want to double-check whether the ProjectConflictError/NotEmpty messages exposing workspaceRoot or projectId are safe to surface — since it's a local workspace path, it's probably fine. Let me pin down the exact line range for the inline comment on the handler file before posting.

11. modify_pr — Non-blocking: the new tools add real backend behavior (list pagination, the mutation capability gate, delete/clone gating) but the added tests only cover registration and tool-name uniqueness. A focused project/handlers.test.ts — in the style of preview/handlers.test.ts, with test layers for ProjectService/SourceControlRepositoryService only — would cover the gate and paging edges.