💭 Both previously flagged issues look resolved now — the free-form error messages in ScheduledTaskService.ts use structured attributes, and the test import uses the proper namespace alias. I'm now checking whether the standalone OrchestratorMcpServiceShape interface violates the convention of defining service interfaces inline within the Context.Service declaration. Since this shape interface predates the PR and the diff only adds a method to it, flagging it would count as unrelated cleanup rather than a concrete new violation, so I'll skip it. Instead I want to look more closely at whether the error message getters in ScheduledTaskService.ts use switch statements or reason discriminators that should really be split into separate error classes — starting with ScheduledTaskManualRunScopeError's reason literal. I count six distinct reasons in that scope error, each mapped to its own message via a switch, which matches the exact pattern flagged as needing separate error classes. The ScheduledTaskManualRunConflictError does the same with four reasons. But since all these scope/authorization variants collapse to the same MCP failure code while only the messages differ, I need to decide whether callers genuinely treat them as different cases or whether one class with a discriminator is the coherent choice. Actually the prior review comment apparently prompted exactly this shape, suggesting deriving the message from captured fields, so the author complied — flagging the switch now would contradict that guidance. Since all scope failures map to the same task_not_found outcome and the message is derived from a stable reason field, I'll lean toward not flagging this. Now I want to check other conventions, like whether makeKeyedSerialExecutor (imported from orchestration-v2/KeyedSerialExecutor.ts) counts as a service — it looks like a factory function. I'm checking through the test harness's layer setup and the ScheduledTaskService test file's imports and mocks, which all look fine as test seams. There's a minor import-ordering nit with CodexProviderCapabilitiesV2 being placed after other imports, but that's a lint concern, not something I need to address. Now I'm moving into looking at the MCP server code. In OrchestratorMcpService.ts, there's a named import of ScheduledTaskManualRunError and ScheduledTaskService from the local service module, which technically bends the "import as namespace at service boundaries" convention, but since this is a pre-existing pattern the PR is just extending, flagging it would be unrelated cleanup, so I'll skip it -- same reasoning applies to the integration test file. Next I'm looking at scheduledTaskManualRunFailure in the orchestrator service, which is a switch on the error's _tag mapping domain errors to an MCP failure type. This looks like a one-off inline mapping rather than a reusable translation, so per the convention it doesn't need a forced static factory, but I want to check the file for whether similar mapping helpers already exist there before deciding. I'm also checking that domain errors preserve cause where relevant — scope, ceiling, conflict, and not-found errors are pure validation errors so they don't need one. Then I'm noticing the pre-existing taskError helper being reused with free-form string messages across many new call sites, which conflicts with the convention to derive messages from structured attributes rather than storing an unstructured message as the primary data. I'm checking whether CommandReceiptStoreV2Shape is a pre-existing standalone interface versus following the Foo["Service"] convention seen elsewhere, like in ThreadManagementService.ts where OrchestratorV2["Service"]["getCommandReceipt"] is used instead. Now I want to verify a few things directly: whether CommandReceiptStoreV2Shape really is separate from Context.Service, whether ScheduledTaskService yields its dependencies the usual way, and how KeyedSerialExecutor and CommandReceiptStore are structured.

1. browse_code — apps/server/src/orchestration-v2/CommandReceiptStore.ts

2. browse_code — apps/server/src/orchestration-v2/KeyedSerialExecutor.ts

💭 That's a pre-existing pattern, so CommandReceiptStoreV2Shape is fine as is. Let me check ScheduledTaskService.ts's current state to confirm the error definitions and imports are correct.

3. browse_code — apps/server/src/scheduledTasks/ScheduledTaskService.ts

💭 I'm noticing both ScopeError and ConflictError use a reason discriminator that switches the message, which is exactly the pattern the guidance warns against—splitting these into separate error classes per reason instead of one class with a switch statement seems like the right call. But I'm questioning whether the ScopeError reasons are similar enough in semantics that a single generic message derived from the operation would suffice, versus ConflictError where "task-already-running" seems genuinely distinct from an idempotency-key mismatch and probably deserves its own class. Since this is also a direct reply to my earlier comment, I need to decide whether re-flagging this is worthwhile. Rather than push further on this ambiguous case, I want to look for other clearer violations instead — checking whether the MCP mapper function for scheduled task manual run failures stays consistent with existing helpers, and looking at what else ThreadManagementService might be adding.

4. git_tools — function .*Failure(|failure(",|const failure",

💭 The new mapper fits the existing naming convention in this file. Since these error messages get exposed to MCP clients, I'm weighing whether these are genuinely distinct caller-visible failures or if there's overlap, and I want to double-check how the test file handles dependency injection to see if there's a real inconsistency worth flagging. Everything checks out - the test seams, stub layers, and schema definitions all look consistent, and the import style matches the prior fix. Now I want to diff the two commits to see exactly what changed between them and whether any new issues were introduced.

5. git_tools — 9733891..1de76b9

💭 I'm considering whether the switch-based reason discriminator in the scheduled task error classes violates the convention against modeling separate failure types within a single error's message getter, versus splitting them into distinct error classes. The MCP mapper collapses caller-archived, target-archived, and not-in-project into the same task_not_found outcome, so the caller-side control flow doesn't actually branch even though the message getter does — which makes me second-guess whether this is worth flagging as strongly as the rule implies, or whether a structured-fields approach with a generic derived message would satisfy the intent without a full split. Weighing false-positive risk against missing a real violation, given that the guidance calls out this exact pattern twice and the new code adds two multi-branch switches in message getters, I'm leaning toward raising it once as a suggestion on the ScopeError lines rather than failing outright — especially since ScopeError's reasons all share the same "not authorized in scope" semantics, and ConflictError's reasons mostly share "idempotency key conflict" too, so keeping one class with a generic message derived from the discriminator could be the acceptable fix. I think the cleanest finding is that ScheduledTaskManualRunConflictError.reason actually mixes distinct semantics — task-already-running versus three idempotency-key variants — with a switch deciding the caller-visible message, so splitting off a separate error type would be cleaner, and the same issue applies to ScopeError's six-branch switch. I'm deciding whether to post one comment covering both switches or two separate ones, and I'll go with a single combined comment to avoid noise. I also need to verify the diff's line numbers so the comment anchors correctly on the head file, since ScheduledTaskService.ts lines 98-177 are the relevant added lines, then draft the actual comment text noting the duplication between the ScopeError getter and the conflict error's getter. That would mean splitting into separate classes per scope condition, which feels like a heavy refactor to push on someone who just changed this design at my request. I'm second-guessing whether it's worth flagging at all — maybe I should focus on whether the current design actually violates the stated convention rather than pushing for further decomposition. That evidence tips me toward splitting — distinct caller-visible messages support keeping distinct error tags. I'll decide to flag it with an inline comment on the ScopeError class (mentioning ConflictError too), without a diff since the fix touches multiple call sites and the MCP mapper, proposing distinct classes for the archived/not-in-project/task-not-in-project cases and splitting out task-already-running from ConflictError. I'm double-checking other spots — the accepted-run-unauthorized variants only affect the message, not control flow, and ScheduledTaskError's free-form messages are a pre-existing contract out of scope. The catchCause/interrupt handling in the poll loop and the Effect.catch usage both look fine, so now I'm pinning down exact line numbers for the comment on the ScopeError class. Confirming the multi-line comment spans lines 98-128, all of which are added in the diff, so I'll write that comment concisely.

6. modify_pr — One convention finding on the new scheduled-task manual-run errors; everything else (namespace imports, layer/service shape, dependency acquisition, test seams) looks consistent with the repo conventions.