💭 I'm reviewing the diff for Effect service convention violations, starting with the new files that were added. Looking at serviceLauncherClient.ts, I can see it's properly importing from the Effect library and defining an error class for the service launcher client. The error class uses Schema.TaggedErrorClass with a reason field as an unstructured string, which seems to violate the convention about capturing stable, serializable domain context instead of free-form messages. However, I notice there's already a ServerSelfUpdateError in the repo using the same reason pattern, so this appears to be following an existing convention in the codebase. Still, the ServiceLauncherClientError is using that reason field to represent many different failure scenarios with distinct messages like "The service launcher supplied invalid startup context" and others, which suggests the error handling could be more structured. Now I'm identifying the strongest findings: the serviceLauncherClient.ts module acquires dependencies directly from the global process object rather than through the Effect environment, and Sqlite.ts calls isServiceLauncherTrial() inside a layer, which is another hidden global read instead of using the Effect environment properly. The repo already has HostProcessEnvironment, HostProcessPlatform, and related utilities from @t3tools/shared/hostProcess specifically for this purpose, so the regression is clear — selfUpdate.ts previously used these environment-based references but now reads process.env directly through the client's default process. I'm also looking at serverShutdown.ts, which has a module-level mutable listeners set acting as a process-global channel with request and awaitRequest as module functions rather than a proper service, which is another violation of the convention against hiding dependencies in module globals. The ServiceLauncherClient layer in serviceLauncherClient.ts looks fine with its standard Layer.effect pattern, and serverRuntimeStartup.ts with its options-based make and layerWithOptions is acceptable. Now I'm checking bootService.ts, which is adding direct imports of node:fs/promises and node:crypto with a writeDurably function using Effect. tryPromise and process.pid — it has a diagnostics suppression comment. There's an existing writeFileStringAtomically utility in atomicWrite.ts that was previously used but is now removed. The issue here is that writeDurably inside make is calling Node fs directly while also acquiring the fs FileSystem service, mixing runtime-backed APIs with imperative ones. Using process.pid directly also bypasses proper dependency acquisition, which violates the convention against hiding dependencies in module globals or Layer.succeed implementations that call runtime-backed APIs. The file has an explicit diagnostics-off comment, suggesting the launcher needs to be standalone, though bootService isn't actually standalone. They need fsync which the Effect FileSystem service might not provide, so I probably shouldn't fail on this but could flag it as a concern. Looking at the error handling, PinnedRuntimeInstallError is being reused with step strings where the mapping checks error.step.startsWith("This version includes") — essentially using the step field to carry a full user-facing reason and then string-sniffing to decide the message. This violates the convention that semantically distinct failures should have separate error classes rather than using a discriminator field like step to drive control flow and message selection. MigrationManifestMismatchError looks fine with its structured fields and derived message. The servicePreflight.ts module uses pure functions and calls node:sqlite synchronously outside Effect at the CLI boundary, which is probably acceptable even though it's technically not a service. But serviceLauncherClient.ts has manual JSON.parse with hand-rolled validation instead of using Schema, and while the PR has some @effect-diagnostics-next-line preferSchemaOverJson:off annotations, the decoder still needs attention. The decoders for parsing the service state are duplicated across three files when they should live in a single canonical location like serviceProtocol.ts, which already defines the types — that's a moderate finding worth flagging against the conventions around keeping schemas and their implementations together. The tricky part is that serviceLauncher.ts is a standalone Node bundle that can't pull in Effect runtime dependencies, so while it imports types from serviceProtocol.ts, a Schema-based decoder would bloat it unnecessarily. The real duplication I should focus on is between bootService's readState and serviceLauncherClient's decodeUpdate — those could be shared, though it's a lighter concern. In selfUpdate.ts, the make function no longer needs host options passed in; it now pulls ServiceLauncherClient, ProcessRunner, and other dependencies from the environment, which is cleaner. But I'm noticing resolveServ resolveServerSelfUpdateCapability is calling isServiceLauncherManaged() to read the global process, and ServerEnvironment.ts is where this gets used — it's an Effect function that succeeds but hides a global read inside. Same pattern I flagged before. Then in Sqlite.ts, the layerConfig is using isServiceLauncherTrial() — another global read baked into a layer definition, when the ServiceLauncherClient service already exposes trial as a proper dependency. This is a real violation: the persistence layer should pull the trial flag from the service or config instead of re-reading process.env directly. There's a potential layering cycle issue since ServiceLauncherClient is provided at the makeServerLayer level and persistence sits deeper, but it's still worth flagging because it duplicates the launcher context decoding and can diverge from the client's stricter validation — the client fails on invalid context while this just returns false. Looking at serverActivation.ts and ProviderCommandReactor, both accept an optional activation Effect parameter as a callback or gate, which is fine since it's a deliberate strategy pattern rather than a service dependency. But when I check serviceLauncherClient.make, it accepts test seams like process and currentVersion in its options, which is similar to the old host pattern. The repo already has HostProcessExecutablePath and other dedicated Context services for this, so ServiceLauncherProcess should probably be a service too instead of a plain object with imperative methods that defaults to the global process. The real issue is that production defaults are reading globals directly, hiding the runtime dependency in a module default rather than acquiring it from the environment. I need to check the hostProcess package to see what conventions are already established, and verify the packageJson import for the currentVersion default. I'm noticing the new serviceLauncherClient.ts is accessing process.env directly through options?.process ?? process instead of going through the HostProcessEnvironment pattern that the rest of the codebase uses via Context.Reference. This creates a hidden dependency in the module defaults rather than properly exposing it through the Effect environment. The helper functions like isServiceLauncherManaged() and isServiceLauncherTrial() are also plain synchronous functions that read from globals directly. I'm looking at the ServiceLauncherClientError class, which uses Schema.TaggedErrorClass with just a reason attribute and relies on the base class's default message implementation rather than overriding it. The pattern here aligns with the convention of deriving the message from structured attributes instead of storing raw unstructured text. However, I notice this error class is handling about six distinct failure modes—invalid context, version mismatch, IPC unavailable, not managed, send failure, disconnect, and launcher rejection—all mapped through a single reason field that becomes caller-visible. The convention suggests that distinct caller-visible messages should map to distinct error classes or at least have structural discriminators. That said, there's precedent in the codebase with ServerSelfUpdateError, which also uses a reason field for caller-visible RPC messages, so this might be a deliberate stylistic choice. It's technically a violation of the convention's guidance on distinct failures, but it would require significant refactoring to address properly. I'll flag it as a moderate-confidence finding rather than a high-confidence one. Now let me check the exact lines in selfUpdate.ts where the step.startsWith logic appears. I'm looking at a specific code pattern in selfUpdate.ts where an error class is being misused to pass user-facing messages through prefix-matching logic, which violates the principle of keeping semantically distinct failures in separate error classes rather than using a single class with discriminators to drive different control flow paths. The fix would be to create a dedicated error type like ServicePreflightBlockedError with explicit reason and version fields, then catch it using Effect.catchTags for cleaner separation of concerns. I'm also noticing a hidden global process dependency issue in serviceLauncherClient.ts where isServiceLauncherManaged() and isServiceLauncherTrial() are called from production Effect code without proper dependency injection — the code uses options?.process ?? process as a fake-DI seam, and while the IPC handle itself genuinely needs the Node process, the environment variables should be sourced from HostProcessEnvironment Context instead. The real problem is that isServiceLauncherTrial() in the Sqlite layer duplicates what ServiceLauncherClient.trial already provides, but I need to verify whether the persistence layer can actually access the ServiceLauncherClient that's provided at the top level of makeServerLayer — if it's plumbed through the layer composition correctly, the persistence layer could receive the trial flag directly rather than making a separate global read. For serverShutdown.ts, there's a clear architectural issue: it uses a module-global mutable Set as a handoff channel between the CLI and a service, which violates the principle of not hiding dependencies in module globals — this should be a proper Context.Service with a layer instead, and it's also untestable because it leaks state across tests in the same process. In bootService.ts, writeDurably is doing filesystem operations directly via node:fs/promises and process.pid inside a service that already has FileSystem acquired, which means it's bypassing the acquired service dependency and making the service untestable — there's even an existing writeFileStringAtomically utility in atomicWrite.ts that should be used instead of hand-rolling durable writes with raw Node APIs. The ensurePinnedRuntimeInstalled function already takes fs, path, and runner as parameters from before this PR, so I won't flag that as a concern—the PR only adds the validate parameter to the existing signature. I'm also checking what the error message for PinnedRuntimeInstallError contains. The message template is Pinned runtime install failed while ${step}. When preflight.reason gets passed as the step parameter, it produces something like "Pinned runtime install failed while This version includes a database update and cannot be installed remotely. Run npx t3@x service update..." which is clearly nonsensical without string parsing. That's a definite violation. Now I need to get the accurate line numbers from the file heads for server.ts, Sqlite.ts, and serverShutdown.ts so I can provide the correct line positions for the GitHub PR review comments. I'm checking whether there's an existing pattern in the codebase for managing channels like this—maybe through a Context.Service or PubSub approach—and verifying that ServiceLauncherClient can be accessed from the CLI and server layers to handle the shutdown channel. The CLI races the shutdown request against the server runtime, and the self-update logic triggers the shutdown, so I need to structure this using Effect's patterns to make it all work together cleanly. The CLI can create the channel directly and pass it as a service alongside ServerConfig, which aligns with what the server.ts comment suggests. I'm identifying the key findings to document as inline comments, starting with the module-level mutable listener Set in serverShutdown.ts that acts as a hidden runtime dependency between the CLI and ServerSelfUpdate. For the second finding, I need to refactor how PinnedRuntimeInstallError.step handles user-facing blocked reasons—instead of using string prefix matching, I should create a dedicated error class like ServicePreflightBlockedError with explicit version and reason fields, then use Effect.catchTags for proper error handling. The third finding spans two locations: serviceLauncherClient.ts reads the global process and environment variables directly when HostProcessEnvironment already exists as a reference, and then Sqlite.ts line 80 calls isServiceLauncherTrial() as a global read inside a layer even though ServiceLauncherClient.trial is available—I'll post separate comments on each file to address these issues. For the fourth point, ServicePreflightBlockedError only carries a free-form reason string across six different failure modes, which violates the convention requiring structured error attributes with derived messages. I should flag this as a lower-priority inline comment, suggesting either a structural discriminator like operation or kind plus a derived message, or separate error classes entirely. The fifth issue is in bootService.ts where writeDurably uses node:fs/promises directly via Effect.tryPromise, bypassing the acquired FileSystem service—this hides a dependency and makes the service's writes untestable, so I'll flag it as violating the convention against hiding runtime-backed APIs in module globals or implementations. Actually, looking closer at Effect's FileSystem API, the File interface does expose a sync method for fsync operations, so the durability requirement could theoretically be met through the service. Directory fsync might be trickier, but I'll mention this as a non-blocking note rather than a hard violation. Scanning the dependency acquisition rules — the instructions emphasize reporting concrete violations to avoid false positives. writeDurably doesn't appear in the patterns they're looking for (service parameters, Layer.succeed, ManagedRuntime.make), so I'll skip flagging it inline. Now checking the server.ts area for other issues — I'm noticing ServiceLauncherClient.layer might be provided twice in the route layer setup. So the two instances of ServiceLauncherClient have separate activation deferreds and trial states — the routes instance's activation is never actually awaited by anything, which means ServerSelfUpdate operates independently from the server runtime startup that uses the other instance. The real issue is that makeRoutesLayer nests a Layer.provide that shadows the ServiceLauncherClient.layer already provided at the outer level in makeServerLayer, creating a duplicate stateful instance with its own independent Deferred and Ref state. This violates the principle of composing a resource once at the application level and providing it to all consumers rather than duplicating it across feature layers. I'm seeing that the Layer.provide call for ServiceLauncherClient.layer is nested inside makeRoutesLayer at line 419, while a separate provision happens at the makeServerLayer level around line 629. Since layer memoization is scoped to each Layer.provide composition, the nested provision creates its own isolated instance—meaning ServerSelfUpdate ends up with a different client than the one controlling activation, complete with separate Deferred/Ref state and its own IPC listener tracking. The duplication itself might not cause a hang since ServerSelfUpdate doesn't await activation, but it's still a layering violation—the same stateful service is being instantiated twice. I should also check if makeRoutesLayer is exported and used elsewhere, since if it's meant to be self-contained in tests or desktop code, the nested provide might be intentional. I'm noticing that makeRoutesLayer self-provides ServiceLauncherClient for standalone use in tests, but this creates a duplicate instance in production since makeServerLayer also builds routesLayer from makeRoutesLayer while the outer layer provides the same dependency. The question is whether this actually causes problems—the routes-side instance doesn't register listeners during construction since those are per-exchange. The real issue is that make performs validation which is wasteful to duplicate, and there's potential state divergence with latestOutcome and prepareTrial refs that ServerSelfUpdate doesn't actually read. It's a latent hazard rather than a current bug, but worth flagging as a comment since it violates the convention of not creating multiple managed runtimes for the same owned resource. The fix would be straightforward: remove the nested provide from makeRoutesLayer and handle it in the test instead. Now I'm settling on my final set of inline comments. I've identified six issues across the codebase: the module-global channel in serverShutdown.ts, step-prefix sniffing in selfUpdate.ts, using global process and env instead of proper dependency injection in serviceLauncherClient.ts, the related isServiceLauncherTrial() global read in Sqlite.ts, an error that only carries free-form reason text, and the double provide of the stateful ServiceLauncherClient. That's a lot, but they're all grounded concerns. I'll keep the first four as strong flags and phrase the remaining two as suggestions to maintain signal clarity. Now I need to verify that the line numbers I'm using actually appear in the diff so the comments will attach properly. For selfUpdate.ts, the mapError block is newly added around lines 160-167, so I'll use start_line 152 and line 166 to cover the relevant section. For serviceLauncherClient.ts, I'm looking at line 210 where the host variable is assigned, spanning lines 206-213. And I still need to check the Sqlite.ts change at line 80. For #2, the fix spans multiple files since it needs a new error class in either servicePreflight or pinnedRuntime with catchTags, so I'll describe the approach rather than show a diff. For #1, I'd suggest defining a Context.Service across multiple files without a diff. For #3, the fix requires acquiring HostProcessEnvironment in the make function—I can sketch that briefly. For #4, I could note threading the trial flag from ServiceLauncherClient. Now I'm drafting the comments, starting with #3: the issue is that make defaults to the global process for both the IPC handle and env lookup, reading the launcher context outside the Effect environment, but the repo already has HostProcessEnvironment modeled in hostProcess.ts that we could use instead. For the IPC handle override in tests, I'd suggest exposing it as a Context.Reference rather than an options.process default so the dependency is explicit in the layer's requirements. I should also note that isServiceLauncherManaged and isServiceLauncherTrial are sync globals called from selfUpdate.resolveServerSelfUpdateCapability and Sqlite.layerConfig, and resolveServerSelfUpdateCapability itself is an Effect.fn that reads these globals inside, so that's worth mentioning in the comment. Since findings exist, the check state should be set to failure per the convention violation rule.

1. modify_pr — Reviewed the new service modules and their call sites against the Effect service conventions. Six findings, mostly around dependency acquisition (module globals / global process instead of the Effect environment) and error modelling (a step string used to drive caller-visible messages). Details inline.