💭 Both previously flagged issues look resolved now, so I'm going back through the current diff to check for any new violations, starting with the imports and structure in QueueMcpService.ts. I'm looking at whether the local service modules are imported as namespaces consistently — some files use named imports (like QueueMcpService and ThreadManagementService), others use namespace imports for context modules. I'm also noticing the test file imports layer as a bare name from QueueMcpService, which could count as erasing the module namespace, so I want to check whether that convention actually applies to test files. Turning to real issues, I'm checking how QueueMcpService.ts handles error returns using yield* failure(...), and what the failure type actually resolves to. I'm also examining whether make should be exported per convention, since the file currently defines it unexported while other sibling service files may export it -- worth checking WorktreeMcpService/OrchestratorMcpService for consistency. On the error side, I'm looking at how QueueMcpFailure uses a code discriminator with a free-text message, and weighing whether that violates the convention of deriving the message from structured attributes rather than storing it as unstructured free text. Actually QueueMcpFailure builds messages by stringifying the cause, which seems to conflict with that same convention, though it may be justified as MCP boundary text — worth checking precedent in WorktreeMcpService or OrchestratorMcpService. As for the unexported thread-not-found predicate, it's used inline rather than wrapped redundantly, so it's likely fine, though it'd be cleaner placed next to the error class itself. I want to check if ThreadManagementService already exports similar predicates, since duplicating one locally would be worth flagging. I'm also weighing whether using a schema predicate to check tags inside a mapError call (rather than a catch) fits the convention about catchIf-based recovery, which feels borderline since this isn't actually a catch. Before deciding, I'll compare this against the actual patterns in WorktreeMcpService.ts and OrchestratorMcpService.ts, and then look at the Orchestrator.ts changes too.

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

2. browse_code — packages/contracts/src/worktreeMcp.ts

💭 QueueMcpFailure follows the same pattern as WorktreeMcpFailure, so that's fine, not a finding. Next I want to look at WorktreeMcpService.ts to check its structure, particularly whether things are properly exported or use namespace imports.

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

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

💭 I'm checking that QueueMcpService follows the same conventions as WorktreeMcpService, particularly around whether make is exported and how the layer is structured at the end of the file. I also want to verify if there's an exported predicate for ThreadManagementThreadNotFoundError and whether the queue service's local use of Schema.is duplicates an existing exported predicate. The new pModeCeilings.ts module looks fine as pure helper functions. I'm also examining whether dispatchFailure's approach of comparing string _tag values on an unknown type violates the convention of catching statically known tagged failures with Effect.catchTags — WorktreeMcpService does similar tag comparison but on a typed error rather than unknown, so there's precedent but with better type safety there. Looking at whether errorMessage deriving from cause/detail violates the rule against sourcing wrapper messages from cause — but WorktreeMcpService does the same thing, and these MCP failures intentionally carry human-readable messages for the agent, so this seems like established repo convention I wouldn't flag. There's also a possible message-leak concern but that's likely outside scope for this review. Now I'm comparing import patterns between toolkits/queue/handlers.ts and tools.ts against the worktree toolkit files, and checking the Orchestrator.ts changes — specifically the new getCommandReceipt field and whether the earlier comment about dropping a type alias import at line 44 was addressed. I want to grep through Orchestrator.ts to verify. I'm also noticing Orchestrator.ts duplicates runtimeModeRank/interactionModeRank functions that already exist in McpModeCeilings.ts, likely intentional since Orchestrator is core domain and shouldn't import from mcp/ due to layering rules — probably not a convention violation worth flagging. Let me pull up the worktree toolkit files and the Orchestrator import section to confirm.

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

6. git_tools — CommandReceiptStoreV2

💭 With both prior findings resolved, I want to check the import style in QueueMcpService.test.ts against WorktreeMcpService.test.ts for consistency, verify the integration test's mock layer usage is just a test seam, and confirm the barrel export convention for the new queueMcp module matches existing patterns. Since queueMcp is schemas/contracts, export * fits the sibling convention even though it breaks alphabetical order after worktreeMcp — that's a minor style nit, not a real convention violation, so I'll skip it and move on to checking WorktreeMcpService.test.ts's import style.

7. git_tools — from "./WorktreeMcpService.ts"

8. git_tools — ThreadManagementThreadNotFoundError|export const is[A-Z]

💭 I'm noticing that the test files use inconsistent import patterns for the layer—some alias it in a way that erases the module namespace, others import it bare, which is the exact anti-pattern the convention calls out at service boundaries. It's borderline whether this is worth flagging given it's already the established precedent across multiple test files. I should move on to checking ThreadManagementService around lines 150-170 and 240-280 to see how other consumers discriminate the ThreadNotFoundError case, whether they use a tag check or a schema predicate.

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

💭 The ThreadManagementServiceShape interface is pre-existing and out of scope for this PR since it only adds a member to it, so I'll skip flagging that. In QueueMcpService though, I notice the projectionFailure checks Schema.is(ThreadManagementThreadNotFoundError) against an unknown error even though errors should be statically known here, so I want to check if there's precedent elsewhere for handling this differently. I'm curious whether OrchestratorMcpService discriminates thread-management errors using _tag checks instead, and whether it also erases the typed error channel with error: unknown in a similar helper. Let me look at how it maps thread-management errors to compare approaches.

10. git_tools — ThreadManagementThreadNotFoundError|_tag === "ThreadManagement

11. git_tools — Schema.is(

💭 I'm checking whether QueueMcpService's structure genuinely deviates from WorktreeMcpService's pattern, specifically whether make is exported in the original convention or kept internal, so I need to verify the end of WorktreeMcpService.ts before flagging a violation. Looking closer at errorMessage, QueueMcpService seems to prioritize detail/cause.message over the error's own structured message getter — which contradicts the convention that wrapper messages should come from stable structural fields, not from stringifying an opaque cause. This looks like a real divergence from WorktreeMcpService, which apparently prefers error.message directly instead. This confirms the test only checks error codes, so the fix has minimal blast radius — I can suggest replacing errorMessage with a version that derives from the structured message property, mirroring WorktreeMcpService's approach, as a self-contained inline fix. Moving on, I'm now looking at dispatchFailure's handling of unknown errors, where classifying via a stringified _tag on an untyped value erases the static guarantees of the OrchestratorV2Error union — this seems to run against the convention of using Effect.catchTags for statically known tagged failures. There's precedent in OrchestratorMcpService which uses a typed switch over _tag for ThreadManagementError, so typing this parameter properly and switching exhaustively would be more consistent. It's a reasonable finding, but I'm not sure it's a clear convention violation versus just a style preference, so I'll treat it as borderline and decide whether to raise it alongside the stronger finding or skip it. I also notice projectionFailure uses unknown because it handles errors from two different channels — one produces OrchestratorV2Error and the other ThreadManagementError. That means loadProjection's check for ThreadManagementThreadNotFoundError can never actually trigger in that channel, which looks like dead/incorrect logic worth flagging separately from the typing style issue. dispatchFailure's string-sniffing on an unknown-typed error matches the same anti-pattern the guideline warns against, so I'll flag it as its own low-key inline comment, though I'm wary of being too nitpicky given the check wants concrete violations. I should double-check OrchestratorV2Error's exported tags before writing the suggestion. Separately, I'm noticing Orchestrator.ts now duplicates runtimeModeRank/interactionModeRank logic that was just extracted into McpModeCeilings.ts, which also raises a layering concern since Orchestrator shouldn't depend on mcp/ — worth suggesting the helpers move to a shared module instead.

12. git_tools — export const OrchestratorV2Error|export type OrchestratorV2Error

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

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

15. browse_code — apps/server/src/orchestration-v2/Orchestrator.ts

💭 All the orchestrator errors expose structural message getters, so preferring detail/cause over .message could bypass those structured messages and end up stringifying raw defects instead—like a DB error hiding inside OrchestratorProjectionError's cause. I want to check OrchestratorDispatchError too, so I'm looking at lines 60-95 to see how its cause field is typed.

16. browse_code — apps/server/src/orchestration-v2/Orchestrator.ts

💭 I see that the cause field is a string, and the queue service deliberately digs into it to surface the actual rejection reason rather than relying on the generic "failed to dispatch" message. This looks like an intentional tradeoff to expose more useful error context to the agent. But I'm now spotting a more concerning case: for OrchestratorProjectionError, the cause is a Schema.Defect, so when cause is an Error instance, its raw message (like a SQL error) leaks straight into the MCP failure output. That directly violates the convention that message should be derived exclusively from structural attributes, never from cause or a stringified defect -- so this looks worth flagging. However, precedent from WorktreeMcpService favors preferring the wrapped error's own structured message getter over digging into cause. But OrchestratorDispatchError's message doesn't include the actual reject reason, and that reason seems deliberately surfaced via cause for queue commands, so swapping to the structural message could weaken diagnostics without breaking any existing tests, since those check codes rather than message content. I'm weighing whether this ambiguity is enough to still flag as a convention violation. I'm now sketching a narrower fix: derive the message from the source error's structured field, only falling back to cause when it's a curated string rather than an arbitrary defect's message. Drafting revised code... Actually, since these Orchestrator errors all extend Error via the tagged error class, the final fallback already handles the structural message case—so the only truly redundant branch is the cause instanceof Error → cause.message check. I'm now leaning toward simply deleting that one line rather than the more elaborate rewrite. I'm weighing whether this is too minor to fail the check on, given two prior runs already found and fixed real issues, and if I don't find anything else solid I need to reconcile "All clear" with the required JSON output format. Let me keep scanning for other possible violations before settling on a verdict, moving on to check the queue toolkit's imports next.

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

18. browse_code — apps/server/src/mcp/McpInvocationContext.ts

💭 I'm confirming the McpHttpServer diff correctly imports QueueMcpService as a namespace and uses its layer. I'm also tracking a couple of remaining findings: how errorMessage derives its text from cause.message as a defect, and how dispatchFailure relies on stringly-typed _tag checks on an unknown value instead of a proper typed union. For that second point, I'm sketching a typed alternative using a switch over OrchestratorV2Error's _tag to classify failures like operation_rejected versus orchestration_error, mirroring the precedent set by OrchestratorMcpService's threadManagementFailure switch. Since fixing this properly would require adding an import for OrchestratorV2Error alongside the function change, I'm weighing whether this counts as a legitimate inline suggestion or needs to be flagged separately as requiring an additional import. I'm deciding to post both findings A and B, since B represents a genuine type-safety erasure rather than just a style preference — a renamed tag could silently misclassify errors. I'm skipping a third potential comment about the test file's import convention since it's low value, and now reconsidering finding A more carefully, specifically whether the cause instanceof Error path is actually reachable with sensitive or opaque text given how OrchestratorProjectionError.cause originates as a Defect from the projection store. Tracing through errorMessage's logic, I confirm the cause path is reachable and likely returns PersistenceSqlError's message, which does appear structural, so this may actually be fine already. But the stated convention is explicit that a wrapper's message should derive exclusively from stable structural attributes, never from cause or a stringified defect — and QueueMcpService's projectionFailure violates this by preferring cause through errorMessage. The minimal fix would be to prefer the error's own message field instead. If I reorder to prefer .message first though, OrchestratorDispatchError loses its ceiling-rejection detail that the MCP agent needs, falling back to a generic dispatch failure message. So the fix has to keep the detail/string-cause preference specifically for dispatch errors while only dropping the cause-instanceof-Error branch — which is what I already proposed, and it shouldn't break any tests since they check code fields, not messages. Now I want to verify the line numbers in the diff line up correctly for my inline comments on the new QueueMcpService.ts file. Now I'm weighing whether these findings are serious enough to fail the check, since the guidance says not to fail for mere style preferences — but the rule explicitly forbids deriving the wrapper's message from cause or cause.message, and QueueMcpService does exactly that, so this reads as a clear convention violation rather than a stylistic nitpick. For the second issue, since there's no explicit bullet covering it, I'm treating it as weaker: the mapper takes an untyped value and compares stringified tags rather than using a typed switch, which is more of a suggestion than a hard rule. I'm considering posting both but keeping the strong one (A) as the main driver for failing the check, possibly folding B into the same comment thread since they're adjacent lines rather than making it a separate flag. I'm now thinking about how to structure the actual suggestion diff for the cause block — replacing lines 80-84 to drop the redundant Error-message branch while keeping the rest of the cause-handling logic intact. For comment 2 on the dispatchFailure typing, the fix would require importing OrchestratorV2Error and changing the signature, which spans multiple non-contiguous edits — too messy for a single diff, so I'll just describe it rather than provide a suggestion block. Given that complexity and lower payoff, I'm leaning toward dropping that second comment and focusing the review solely on the clear, well-grounded violation in the cause-handling code. I'll mark this as a failure since there's a genuine finding, then draft the comment explaining that deriving the failure message from cause.message leaks arbitrary internal error text at the MCP boundary, whereas convention dictates building wrapper messages only from structural attributes — noting the other branches already preserve curated messaging appropriately. I need to verify the line numbers (80-84) match the actual diff for QueueMcpService.ts before posting the suggestion via github_pr_review, checking indentation and confirming the RIGHT-side line 80 corresponds to the cause-check conditional. Everything checks out, so I'll proceed with posting.

19. modify_pr — One finding on the new QueueMcpService error translation. Everything else (inline Context.Service interface, make/layer shape, namespace import in McpHttpServer, Foo["Service"] typing in Orchestrator/ThreadManagementService, toolkit wiring) matches the existing worktree/orchestrator MCP conventions.