💭 I'm reviewing the diff across the environment MCP service files to check they follow Effect service conventions, starting with how modules are imported in EnvironmentMcpService.ts and McpHttpServer. ts versus handlers.ts, which uses a named import of the service tag rather than a namespace import. I'm also checking whether make should be exported since the module owns construction of the service, noting that currently it's defined but not exported, which may deviate from the convention. For the error handling, I'm looking at how EnvironmentMcpFailure combines a code literal union with a free-form message string, and checking whether that violates the convention of deriving the message from structured attributes rather than storing it as unstructured text — even though using a single error class with a multi-value discriminator is otherwise allowed. I'm also flagging that the failure(code, message) helper is basically just wrapping arguments into a new error object, which the convention explicitly forbids as a trivial pass-through helper. Similarly, the unavailable(...) wrapper catches an error's cause but discards it when constructing the message-only failure, which conflicts with the rule that underlying causes should be preserved rather than dropped. I'm also noting some smaller things: the layer definition looks fine, but tools.ts mixes namespace and named imports for its dependencies in a way I want to compare against the orchestrator's tools.ts pattern. The test file's use of a mock provider registry is an acceptable test seam, and the new registration tests seem justified since this is genuinely new functionality rather than a mechanical refactor. I did notice an import ordering issue in McpHttpServer.ts, though that's more of a lint nit than a real convention violation, so I want to check how other files in the repo handle similar import patterns.

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

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

💭 Let me check the existing WorktreeMcpService.ts file to see how conventions like exports and error definitions are handled in this repo.

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

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

💭 I'm noting that the new code follows existing conventions in WorktreeMcpService, but the review instructions say to treat them as authoritative even when older code differs—so I shouldn't excuse issues just because they mirror legacy patterns, though I also shouldn't flag mere style preferences unrelated to the actual changes in scope. The failure helper is a clear match for the disallowed pattern of a trivial wrapper that just constructs an error object, and since it's newly introduced in this PR rather than merely reused, it counts as a genuine violation. Similarly, EnvironmentMcpFailure stores an unstructured message string instead of deriving it from structured attributes, and doesn't preserve the underlying cause when wrapping real failures—both flagged issues per the instructions, even though this mirrors the existing WorktreeMcpFailure design exactly. Looking closer at the multi-value discriminator allowance, I notice the two environment_unavailable call sites actually use distinct messages for different scenarios, which suggests these might really be two separate structural cases being conflated under one generic code rather than a single case with a derived message. The bigger issue is that the unavailable helper wraps a failure without preserving the original cause, so diagnostics get lost even though the test expectations don't check for it directly — I'd flag both the missing cause field on the contracts error class and the message-derivation gap together as one combined comment on the service helpers. I'm also noting the mixed import style in tools.ts, where a namespace import sits alongside a named import for the service, and want to check whether that's consistent with the convention used in the worktree handlers.

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

6. browse_code — packages/contracts/src/environmentMcp.ts

💭 I want to compare this against WorktreeMcpFailure in the contracts to see if they're identical. I'm also noting that the operation_failed code in EnvironmentMcpFailure appears to be declared but never actually used in the service — a minor dead literal, worth flagging but not a major convention violation. Let me check contracts/worktreeMcp.ts next.

7. git_tools — WorktreeMcpFailure extends

8. git_tools — Schema.TaggedErrorClass

💭 I'm noting the repo has mixed conventions, but the newer pattern with structured attributes and derived messages should take precedence, especially since EnvironmentMcpFailure mirrors the established WorktreeMcpFailure sibling. I'm flagging the "failure" helper in EnvironmentMcpService.ts as a strong finding since it violates the explicit prohibition against helpers with no real behavior beyond wrapping. Then for EnvironmentMcpFailure itself, the message is stored as an unstructured string rather than derived from structured fields, so I'm suggesting it derive the message from code and add an optional cause. I'm also catching that the unavailable helper drops the underlying error instead of preserving it as cause, which ties into the same structural fix—likely worth two separate comments, one for the contracts file and one for the service. Given the explicit prohibition on the failure helper pattern and the clear message-derivation violation, I think the check should fail overall. I'm also double-checking whether two distinct caller-visible messages sharing one environment_unavailable code should really be split into separate tags or a generic derived message, which supports the same finding. Everything else—unused codes, test-only Layer.succeed usage—looks fine to skip, so I'm moving on to check handlers.ts next. Since flagging non-export of make would be noise if it mirrors sibling MCP services, I should check a couple of those services to confirm the convention before deciding.

9. git_tools — ^export const make = Effect

10. git_tools — ^const make = Effect

💭 I notice the new file uses a non-exported const make, unlike the dominant export const make convention elsewhere, but since sibling MCP services follow the same pattern and the guidance seems focused on avoiding fake construction rather than export style, I'll skip flagging this as noise. Turning to actual findings, I'm looking at environmentMcp.ts where message: Schema.String is unstructured — it should probably derive its message from code and include an optional cause field. But fixing this properly requires touching the service call sites too, making it cross-file, so a clean diff isn't possible here; I'll describe the contract-side change and note the service updates needed rather than force a self-contained diff. I'm also flagging EnvironmentMcpService.ts — the failure helper is a bare error-constructor alias, which is discouraged, and the unavailable handler drops the underlying cause when it shouldn't. Since removing the helper or wiring in a cause field means updating call sites and possibly the contract schema, I don't think I can post a tidy diff for either — better to combine these into two focused inline comments on the service file instead of forcing separate patches. For comment B, I want to note that EnvironmentMcpFailure only stores a free-form message rather than deriving it from structural attributes, losing the underlying cause at wrap sites like unavailable(). I'll suggest dropping the message field, adding an optional cause of type Defect, deriving message from code via a getter, and splitting the two distinct environment_unavailable situations apart. Since deriving message from code would break existing constructions that pass message directly, I won't provide a concrete diff for this one. Given these clear convention violations, I'm settling on failure as the review state, and now I'll post the review.

11. modify_pr — Reviewed the new EnvironmentMcpService, its toolkit wiring, and the environmentMcp contract against the Effect service conventions. Module layout (namespace imports of Effect subpaths, inline Context.Service interface, make then layer, dependencies acquired via yield*) looks right; two findings on the error model.