1. git_tools — 5e63aea..f9bbc80

💭 The diff is huge and doesn't seem to respect the stat-only flag, so I should narrow my focus. Since this PR is about queue MCP controls, I'll check the actual changed files through the GitHub API instead.

2. github_api_read_only — repos/{owner}/{repo}/pulls/{pull_number}/files

3. github_api_read_only — repos/{owner}/{repo}/pulls/{pull_number}

💭 The PR's diff is muddied by an unrelated merge base, so I'm narrowing focus to the actual queue MCP changes—prior review already covered QueueMcpService.ts, so now I'm zeroing in on the newly added contract files like queueMcp.ts. ceilings, orchestrator service, adapter, and thread management changes to check whether the earlier comment was addressed and review the remaining new files at head.

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

5. git_tools — apps/server/src/mcp/**

💭 I'm mapping out the PR's commit history to identify the actual diff range, then plan to diff those two commits to see the real changes introduced.

6. git_tools — e0d7200..f9bbc80

💭 That base commit isn't right either, since it pulls in unrelated changes from a rebase. I should focus on the PR's actual four commits and figure out the real parent of the first one instead of relying on the log's path-filtered result. Rather than chase the exact diff base, I'll just review the queue MCP files directly at head — the contracts file, the service files, and the mode ceilings file. Since the author already addressed feedback in one commit by following Effect service conventions, I want to check the current state of QueueMcpService.ts and McpModeCeilings.ts to confirm.

7. browse_code — apps/server/src/mcp/QueueMcpService.ts

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

💭 That prior comment's now resolved with the inline interface. Checking conventions next: imports look right, using namespace imports for effect modules and a named import for the contracts package, plus a local service import for ThreadManagementService at the boundary, which matches the pattern of importing local service modules directly. I want to verify against how sibling files like WorktreeMcpService.ts import ThreadManagementService before flagging this as inconsistent, since conventions favor deferring to existing patterns unless clearly wrong. Then I'm checking the QueueMcpFailure error shape, which uses a code and message field, against the convention that errors should derive their message from structured attributes rather than storing it directly. I'm also examining a failure helper function that just wraps arguments into a new error object—this matches a stated rule against trivial error-constructor wrappers, though it's used widely across the codebase, so I want to check whether sibling services follow the same pattern before treating it as a real violation. I'm noticing that errorMessage(error) stringifies the cause into the message field, which conflicts with the rule that a wrapper's message should derive only from its own structural attributes, never from the cause—so dispatchFailure and projectionFailure may be losing the original cause while deriving their message from it. I'm also checking whether a private Schema.is predicate is redundantly wrapped by another function with the same signature, per the exported-predicate convention. Also dispatchFailure's ad-hoc _tag in error string sniffing instead of typed catchTags looks like another violation, since it erases a structured upstream error into a string rather than pattern-matching on the typed union. I want to check the contracts file next to confirm.

9. browse_code — packages/contracts/src/queueMcp.ts

💭 I'm noticing QueueMcpFailure only carries a free-form message and code with no cause, which violates the convention of deriving messages from structured attributes and preserving the underlying error as cause—dispatchFailure and projectionFailure both stringify the cause into the message instead of retaining it. Since QueueMcpFailure is newly introduced in this PR, this looks like a genuine violation rather than pre-existing debt, but I want to check WorktreeMcpFailure to see if sibling MCP services follow the same pattern before treating this as a clear-cut issue.

10. git_tools — TaggedErrorClass

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

💭 I'm noting that QueueMcpService mirrors the WorktreeMcpService pattern precisely, using the same failure helper and error class structure. Since the instructions call for treating stated conventions as authoritative over older code while avoiding unrelated cleanup demands, I want to weigh whether the error-model concerns here are legitimate convention violations worth flagging. I'm now checking the import style more closely -- both QueueMcpService and WorktreeMcpService use named imports for ThreadManagementService, so I need to see if the module-shape convention is actually being followed consistently. Now I want to look at the other changed files in this PR -- the OrchestratorMcpService refactor, the new getCommandReceipt additions, and the ClaudeAdapterV2 allowlist -- to check them against the same service definition and error handling conventions. I'm going to pull the diff scoped to just this PR's commits to do that properly.

12. git_tools — 0cf6661

💭 I realize those files were added in later commits, so I need to diff across the whole PR range instead of just the single commit, checking if I can use a parent-reference syntax for the base commit.

13. git_tools — 0cf6661~1..f9bbc80

💭 I'm reviewing the QueueMcpService diff against Effect service conventions, checking that imports, the Context.Service tag, helper functions, make, and layer follow the canonical ordering, and confirming that layer wraps make correctly through Layer.effect. I notice make isn't exported, but since a sibling service (WorktreeMcpService) follows the same pattern, I'll treat that as an accepted deviation rather than a flag. More concerning is the error handling in dispatchFailure, which relies on stringly-typed tag sniffing and manual stringification instead of using typed catchTags against the known OrchestratorV2Error union — that's a real deviation worth calling out. But then I notice the same pattern (message copying arbitrary defect text into cause, failure() as a bare error-constructor alias, untyped tag sniffing) exists identically in the adjacent WorktreeMcpService and OrchestratorMcpService files, which suggests this is the established convention for the MCP failure boundary in this codebase rather than a one-off violation. That weakens flagging concern (a) significantly. Looking at concern (b), the failure() helper marginally matches its sibling's pattern closely enough that it's not worth flagging on its own. Concern (c) is more solid though: dispatchFailure accepts an unknown parameter and does runtime _tag checks even though the actual error type is a known union, which genuinely erases type safety and could be fixed by typing the parameter and switching on _tag directly — that seems like a legitimate, self-contained issue worth raising. Given the repo's guidance that a reused static mapper is allowed to perform this classification when it keeps policy near the error type, and the fact that the prior review already caught and fixed the one clear violation (commit ebc47f0 specifically addressing Effect service conventions), I'm leaning toward the current state being largely conformant. The remaining candidates look like mirrored sibling patterns or optional style choices, which the instructions explicitly say not to fail on. I still want to check whether Orchestrator.ts duplicates runtimeModeRank/interactionModeRank logic. That duplication with McpModeCeilings.ts is real but justified — the orchestrator sits in the domain layer and can't depend on the MCP layer, and the ceiling semantics actually differ since it compares against both ceiling and caller projection, so it's not a convention violation. I'm also noting that getCommandReceipt uses a pre-existing CommandReceiptStoreV2Shape type, which conflicts with the convention against retaining standalone FooShape types in favor of Foo["Service"]. While the ThreadManagementService change correctly used OrchestratorV2["Service"]["getCommandReceipt"], this PR's Orchestrator.ts change instead imports and uses CommandReceiptStoreV2Shape["getByCommandId"] — the wrong pattern, since it should be CommandReceiptStoreV2["Service"]["getByCommandId"] per the convention. I want to check CommandReceiptStore.ts to confirm CommandReceiptStoreV2 is a proper Context.Service with a Service member, and I'm noting that ThreadManagementServiceShape is a separate, pre-existing standalone interface that predates this PR, so flagging it would be an unrelated refactor ask rather than a real issue.

14. git_tools — CommandReceiptStoreV2Shape|class CommandReceiptStoreV2

💭 I notice the new line in Orchestrator.ts uses CommandReceiptStoreV2Shape["getByCommandId"] instead of following the established convention of referencing the inferred service interface as Foo["Service"]["method"] — exactly the pattern the sibling change in ThreadManagementService.ts already uses with OrchestratorV2["Service"]["getCommandReceipt"]. This looks like a small, concrete, in-scope inconsistency with an easy fix. Actually, checking further, Orchestrator.ts itself exports OrchestratorV2Shape and other files import it that way, so Shape types are clearly an established pattern throughout this codebase — which weakens the case for flagging this as a violation, even though the instructions technically take precedence over existing conventions. I'm weighing this against the other candidate finding around how QueueMcpFailure and errorMessage get stringified, trying to decide which is the more defensible issue to surface. Confirmed—that pattern's used elsewhere too. Now let me pin down the exact line numbers in Orchestrator.ts at head for reference.

15. git_tools — CommandReceiptStoreV2Shape

💭 I'm planning an inline comment on line 180 of Orchestrator.ts suggesting the use of CommandReceiptStoreV2["Service"]["getByCommandId"]. Since dropping the now-unused type import on line 44 would require a second change and leaving it would break lint, I'll present this as a unified diff spanning both lines rather than a self-contained inline suggestion. Actually, since these two edits fall in separate, distant hunks, a single unified diff can't cleanly cover them either -- so I'll skip the diff format entirely and describe the fix in prose, giving the exact replacement text inline. This is a minor typing-style issue rather than a severe violation, so I'm weighing whether it should actually cause the check to fail or just be flagged as an optional note. I'll settle on reporting it as neutral rather than failing, since it's a small, single-line convention deviation. That said, I want to double check whether to even surface this: since the PR itself violates the same rule inconsistently across files (correct in a sibling file, incorrect here), it's a legitimate concrete finding tied to changed lines in this PR, so it's worth posting as a suggestion. I'm now looking at where exactly to attach the comment -- the added line in the orchestrator file that needs the fix -- but I need to verify it's actually part of the diff before posting. Given the merge base is quite different, the diff for that file is probably large enough to cover this region, so I'll just try posting the inline comment and see if it goes through.

16. modify_pr — Reviewed the queued-work MCP service, contracts, toolkit, and the orchestrator/thread-management additions against the Effect service conventions. The service definition itself now follows the canonical shape (inline Context.Service interface, real make, exported layer). One small deviation remains in the new orchestrator surface.