💭 I'm looking at PendingRequestMcpService.ts, checking that imports and structure follow Effect conventions—the cursor parsing helper, service tag definition, and error handling are organized in a reasonable order. Now I'm examining the failure schema, which uses a code discriminator plus a free-form message string built at the call site, rather than deriving the message from structured attributes as the conventions suggest. This looks like a possible finding, but since it's a wire contract shared across MCP tools, I want to check whether other existing failures in the repo, like orchestratorMcp.ts, follow the same pattern before flagging it. Also concerning is that errorMessage(error) stringifies the underlying cause directly into the wrapper's message rather than preserving the cause itself, which violates the guidance against deriving messages from stringified defects — though again I need to verify if this matches established repo convention for MCP failures before treating it as a real violation. I'm also noting duplication between Orchestrator.ts and the shared McpModeCeilings.ts module for rank definitions, though this may be acceptable given Orchestrator's role as core server logic. Checking stablePart's try/catch for encodeURIComponent throwing on lone surrogates — that's handled fine. I want to look at the repo's actual conventions for orchestratorMcp contracts before drawing more conclusions. The PendingRequestMcpService setup using Context.Service with exported make and layer looks correct. But I'm flagging an inconsistency in handlers.ts and tools.ts: some files use named imports like { PendingRequestMcpService } while others use namespace imports like * as McpInvocationContext at service boundaries, which the convention discourages in favor of consistent namespace-style imports — though I'm not certain this counts as a real violation versus just inconsistent style. I should compare against how existing orchestrator toolkit files handle these imports, so let me pull up a few of those files to check.

1. browse_code — apps/server/src/mcp/toolkits/orchestrator/tools.ts

2. browse_code — apps/server/src/mcp/toolkits/orchestrator/handlers.ts

💭 The new handlers look consistent using .use, that seems fine. Now I want to check OrchestratorMcpService.ts to compare its error and failure handling conventions with the service definition style.

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

4. git_tools — OrchestratorMcpFailure

💭 The new PendingRequestMcpFailure follows the same wire contract as OrchestratorMcpFailure, so that's consistent and not worth flagging. The service definition also correctly inlines its interface rather than keeping a standalone shape, which is actually better than the orchestrator's approach. Now I'm checking how dependencies are acquired, starting with the crypto service. The crypto import looks fine, and reading a "detail" property from an upstream error inside the error message helper seems acceptable since it's not duplicating cause.message directly. I'm also weighing whether the rank function duplication between Orchestrator.ts and McpModeCeilings.ts is a real convention violation or just an architectural choice outside the scope of listed conventions, and checking whether the type-only import from the contracts package follows the established pattern. Handler code confirms the same use-pattern combining PendingRequestMcpService and McpInvocationContext, and the tools.ts import matches the orchestrator's structure. I'm deciding that the ThreadManagementServiceShape addition and its usage in tests are pre-existing patterns outside this PR's refactoring scope, so they don't need flagging. Similarly, the Orchestrator.ts additions to OrchestratorV2Shape and the optional replayed field are minor API details rather than convention violations. I want to double-check the actual PendingRequestMcpService.ts file for anything the diff might have truncated, and verify the test file's use of a mock layer is just a normal test seam.

5. browse_code — apps/server/src/mcp/PendingRequestMcpService.ts

💭 I'm checking how errorMessage handles the wrapper's message versus deriving from stable structural attributes—OrchestratorMcpService already follows this pattern, but the new version adds extracting a detail field from upstream errors, which is worth flagging against the convention of never deriving messages from causes. The Schema.is predicate for isThreadNotFound is private and not exported, but since it's not wrapped by a redundant function with the same signature, and OrchestratorMcpService uses the same private pattern, I don't think this is worth flagging either. Looking at projectionFailure using isThreadNotFound(error) inside mapError rather than catchIf — this is mapping the error channel, not recovering it, so the convention against using catchIf with a schema predicate to recover known tags doesn't really apply here. What does look weaker is dispatchFailure(error: unknown), which inspects _tag via string comparison instead of using typed tags, even though the dispatch error channel is statically known — a typed mapper or Effect.catchTags would be more appropriate than stringly-typed sniffing. I need to weigh how strict to be here, since I should only report concrete violations rather than assuming stylistic preference, and it's not obvious this fully matches any listed rule about translation boundaries and structured domain errors. I want to check whether the existing OrchestratorMcpService has a similar pattern for comparison before deciding whether to flag this.

6. git_tools — OrchestratorDispatchError|dispatchFailure|_tag" in error

💭 There's no such mapping in OrchestratorMcpService, so I need to check how it maps dispatch errors coming from threadManagement.dispatch.

7. git_tools — .dispatch(

💭 I'm checking how the existing service handles dispatch error mapping around those lines.

8. browse_code — apps/server/src/mcp/OrchestratorMcpService.ts

💭 This new dispatchFailure with untyped _tag string sniffing seems off since the dispatch error channel should be statically typed. I want to check the actual error type of ThreadManagementService.dispatch.

9. git_tools — readonly dispatch:

10. git_tools — readonly dispatch:|dispatch: (|OrchestratorV2DispatchError|export type OrchestratorV2Error

💭 I'm noting that dispatchFailure sniffs _tag off an unknown-typed error, losing static type safety—this should instead use typed handling like Effect.catchTags or a switch on the known error union. I'm checking whether projectionFailure has the same issue with its untyped error parameter. Meanwhile the Schema.is narrowing pattern used elsewhere in getProjectThread's error mapping matches existing repo precedent (as seen in OrchestratorMcpService), so that usage seems acceptable rather than a genuine violation—unlike the string-based _tag comparison on unknown in dispatchFailure, which is strictly worse and sacrifices exhaustiveness checking. I'd recommend replacing that with Effect.catchTags or a typed switch over the known error tags, and flag it as a moderate, defensible finding. I'm also weighing whether the failure(code, message) helper violates the "no trivial error constructor wrapper" rule, since it does nothing but instantiate PendingRequestMcpFailure—but since it mirrors an existing helper already established in the sibling OrchestratorMcpService, this looks like a deliberate repo convention rather than a new anti-pattern worth flagging. ct violation since it directly mirrors the sibling service, so flagging it feels overly pedantic given the check's guidance against demanding unrelated cleanup or optional style fixes. I'm also checking whether the failure contract leaks any wire payload details—it only carries code and message, mirrored from the existing contract, so that's fine. But I want to look closer at whether errorMessage copies raw upstream error text into a caller-visible message, since the rule against exposing wire payloads, arguments, credentials, or defect text directly applies here. This does look like a genuine issue: the new logic pulls detail and stringified cause into the MCP message, which violates deriving the wrapper's message strictly from structural attributes rather than cause text. The right fix would be to use the typed error's own message field, derived from structural attributes, instead of digging into detail or cause. I'm settling on a small set of high-confidence findings: one about _tag sniffing on an unknown value where a typed union with catchTags should be used instead, and another about copying arbitrary upstream detail/cause text into the outward-facing message. I'm double-checking peripheral concerns — the mocked ThreadManagementService in tests looks fine, and McpModeCeilings.ts as a pure helper module checks out. I'm noticing though that Orchestrator.ts duplicates the same ranking logic that was just extracted into McpModeCeilings.ts, which is a real maintainability smell even if it doesn't map cleanly onto one of the specific conventions I'm checking against. I'm deciding to skip flagging the duplicated ranking logic and the optional replayed? field on OrchestratorV2DispatchResult, since neither maps to a listed convention violation — better to stay focused on concrete violations. I'm now looking at PendingRequestMcpService.ts, where helper functions and failure declarations are ordered after the Context.Service tag rather than following the canonical import/schema/tag/make/layer order. I'm checking whether dispatchFailure(error: unknown) silently defeats the typed error channel from threadManagement.dispatch, since using Effect.mapError with an untyped error compiles but would mask a new error tag as a generic "orchestration_error" — I want to trace whether errorMessage correctly handles the cause field on OrchestratorDispatchError before flagging this as a real bug risk.

11. git_tools — class OrchestratorDispatchError

💭 This confirms that OrchestratorDispatchError derives its message from structural attributes while cause is an arbitrary defect string, and the new errorMessage logic prefers that stringified cause over the structured message—exactly violating the rule that message must never be derived from an opaque cause. This surfaces directly into the MCP tool response, making it a strong finding. I'm now also checking enforceRuntimeRequest for similar issues.

12. git_tools — cause: `

💭 The string-cause pattern at line 905 is pre-existing, so it matches existing style and doesn't need flagging. I'm finalizing two inline comments — one on errorMessage and one on dispatchFailure in PendingRequestMcpService.ts — confirming both fall within the diffed lines since it's a new file, so line positions should map correctly for the tool. The errorMessage issue is real: it prefers an upstream cause's arbitrary defect text over the structured error's own derived message, violating the convention that wrapper messages must come only from stable structural attributes, never from cause or stringified defects. My fix strips out the detail/cause branches and just uses the error's own message directly, matching how the sibling OrchestratorMcpService already does it. For dispatchFailure, I'm suggesting the parameter be typed as the known error union and switched on its _tag instead of handling it generically. I'll keep it simple: prose explanation with a tiny illustrative snippet rather than a strict diff. I'm also weighing whether dispatchFailure's string-based tag comparison truly violates the tagged-failure handling convention—it's a mapError not a catch, but the underlying weakness (comparing against unknown rather than static typing) still seems worth flagging, likely as a secondary, lower-severity finding alongside the errorMessage issue. Reconsidering the errorMessage severity once more: since it directly shapes the caller-visible failure message, and the sibling implementation already derives from error.message for structured errors, my suggestion to revert to that simpler behavior lines up well with existing conventions. I'll go ahead and post the review now, marking both findings.

13. modify_pr — Reviewed the new PendingRequestMcpService and its call sites against the Effect service conventions. Service definition (inline Context.Service interface, make acquiring ThreadManagementService/Crypto from the environment, export const layer), namespace imports, toolkit wiring, and Foo["Service"] usage all look right. Two findings on the error-translation helpers.