💭 I'm reviewing this PR to check if it follows the Effect service conventions. I need to examine the new files for violations across imports, service definitions, dependency acquisition, and error handling patterns. Looking at MonitorRegistry.ts, I see it defines a standalone MonitorRegistryShape interface before passing it to Context.Service, which violates the convention requiring inline interface definitions. The file does correctly reference OrchestrationEngineService["Service"] for the inferred interface. There's also a module-global store with a Map and mutable variables that the code comments suggest mirrors McpProviderSession's design pattern, though this approach of hiding dependencies in module globals is flagged by the conventions. I'm spotting a standalone PullRequestSnapshotFetcherShape interface in PullRequestMonitor.ts — that's a direct violation since it's exported separately before being used in the Context.Service definition. The PullRequestMonitor service itself defines its interface inline which is correct, and the import style across the new files looks consistent with the namespace pattern. Now I'm checking the imports in tools.ts more carefully — there's a named import of Tool and Toolkit from effect/unstable/ai, which is a subpath rather than the main effect module. Since they're used as Tool.make and Toolkit.make, this is acceptable even though it's a named import rather than a namespace import, but it's worth noting it's from an unstable subpath. Moving on to error conventions, I see MonitorStartError defined as a TaggedErrorClass with only an unstructured message field. The convention says errors should have structured attributes and derive the message from those, not store an unstructured message as the sole data — this is a violation since the error only contains a generic string message. The tricky part is that these messages are highly varied, constructed at different call sites, so restructuring them into specific attributes would be cumbersome. It's borderline whether to flag this — the convention is explicit about avoiding unstructured messages, but this error genuinely serves as a message carrier for the LLM, so it might be a pragmatic exception rather than a real violation. Let me focus on the clearest violations instead. MonitorRegistryShape and PullRequestSnapshotFetcherShape are defined as standalone interfaces, which directly contradicts the convention that service interfaces should be declared inline within Context.Service and referenced via the inferred Foo["Service"] pattern rather than kept as separate types. Looking at handlers.ts, I'm seeing an error handler that manually checks the _tag property to distinguish MonitorStartError from other errors, but the convention calls for using Effect.catchTags with explicit tag patterns instead of a generic catch with conditional logic. The error mapping pattern here discards the cause and structural context when converting GitHub errors to MonitorStartError, which violates the convention about preserving cause information. Though MonitorStartError is a tool-level error meant for the LLM, this is still debatable. The strongest findings are the two standalone *Shape interfaces—those are directly named anti-patterns in the conventions and clearly introduced in this PR. Let me verify these are actually defined as standalone interfaces and not inlined. Both MonitorRegistry and PullRequestSnapshotFetcher define their Shape interfaces as standalone exports, which is the violation. Interestingly, PullRequestMonitor itself uses the inline pattern correctly, so the author clearly knows the convention—making these Shape definitions inconsistent. In the test harness, they're using inferred types and object literals rather than explicitly referencing the Shape interfaces, which is the right approach. The module-level activeEngine in MonitorRegistry is another concern—it's a hidden dependency where bindEngine stores the service globally, which directly violates the convention against module globals and implicit dependencies. However, there's a documented rationale explaining this is necessary because teardown happens outside the service context, and it mirrors the existing pattern in McpProviderSession, so this is a deliberate architectural choice rather than an oversight. The activeEngine binding is more of a concern but it's well-justified given the architecture. I'm focusing on the clear Shape violations now. Let me scan through the imports in the new files to make sure there aren't any other standalone shapes or named effect imports I've missed. Checking handlers.ts, tools.ts, and gitHubPullRequestMonitor.ts for their import patterns. The GitHubPullRequestMonitorDecodeError uses Schema.Defect for the cause with structured fields for command, cwd, detail, and cause — the detail field is a static description string rather than copying from cause.message, which is fine. I should verify whether MonitorRegistryShape is actually needed since make is a plain const object typed as that shape and gets reused in tests. Actually, the convention says not to keep standalone Shape types — I should inline the interface directly into Context.Service and type make as MonitorRegistry["Service"] instead. The test already uses MonitorRegistry.MonitorRegistry.of({...}) without needing the Shape name, so this refactoring should work. I'll apply the same approach to PullRequestSnapshotFetcher and add inline comments to explain the changes, though I need to figure out the line numbers for the new file first. I found the MonitorRegistryShape interface at line 21 in the new file. Defining the interface methods... Defining the registry interface methods... Adding more imports... Looking at the WakeClaim interface, it tracks the state of a wake request with generation number, command ID, phase status, and dispatch timing, plus a cursor to the previous state so failed wakes can rewind and avoid losing events. Now I'm seeing the start of PullRequestSnapshotFetcherShape interface... Defining the fetcher interface... I'm settling on the final verdict: there are two legitimate findings here. I'm also reconsidering the MonitorStartError issue around storing unstructured messages—it's a real convention violation, though perhaps not as critical as the other findings. So I'm settling on the two Shape interface violations as the core findings since they're textbook matches to the anti-pattern, and I'm dropping MonitorStartError to keep the review focused and avoid a weaker argument. Now I'm looking at the Effect.catch pattern with _tag inspection in handlers.ts to see if that's another violation worth flagging. The convention says not to use catchTag or catchIf just to recover specific _tag variants, but Effect.catch is fine when handling the entire error channel intentionally. Here the code is normalizing defects into MonitorStartError, which is a legitimate use of Effect.catch, so I'm not flagging it. Now I'm ready to write the review with two inline comments and the fail state. For the first comment on MonitorRegistry.ts line 21, the issue is that the standalone MonitorRegistryShape interface should be inlined into the Context.Service definition following convention, then referenced as MonitorRegistry["Service"]. Same pattern applies to PullRequestSnapshotFetcherShape in PullRequestMonitor.ts line 42. Since inlining spans multiple regions (the interface definition plus the class reference), I can't create a clean inline suggestion diff, so I'll just describe the guidance without a suggestion block. Now I'm working through the schema requirements for the comments array—it needs path, body, line, start_line, side, and start_side. The schema marks start_line and side as required integers, but for single-line comments I'm uncertain whether to set start_line equal to line, omit it, or pass null. GitHub's API might reject equal values or require start_line to be less than line for multiline spans. I'll use the github_pr_review tool and set start_line to match line for single-line comments, with side and start_side both set to "RIGHT". I'll set start_line to null and start_side to null for single-line comments, which should work fine. For the new files on the RIGHT side, I need to flag violations at line 21 and line 42. Now I'm deciding whether to post inline comments via the review tool or just return the JSON check result — the instructions emphasize precise inline comments, so I should post the review with those comments and then provide the final JSON response.

1. modify_pr — Effect Service Conventions: 2 findings. Two standalone *Shape service interfaces were introduced; the conventions require the service interface to be declared inline in the Context.Service tag and referenced as Service["Service"].