Repository navigation
Effect Service Conventions: 1 issue found
apps/desktop/src/shell/DesktopShellEnvironment.ts:ShellEnvironmentConfigaccepts aFileSystem.FileSystemservice instance as a plain field, hiding an owned Effect dependency frominstallShellEnvironment/installPosixEnvironmentrequirements instead of acquiring it viayield* FileSystem.FileSystemand providing it inmake(as is already done forChildProcessSpawner).
Previously flagged items are resolved: DesktopPreReadyPlatform.ts now owns the tag plus canonical make/layer with an inline Context.Service interface, and ElectronSafeStorage.ts exports a real make.
Details
Note
Your check run agent prompt is: .macroscope/check-run-agents/effect-service-conventions.md
More information about how Check Run Agents work can be found in our Help Center.
Files examined at head a88e1a3: apps/desktop/src/app/DesktopPreReadyPlatform.ts, DesktopEarlyElectronStartup.ts, DesktopStatePaths.ts, DesktopApp.ts, DesktopEnvironment.ts, apps/desktop/src/electron/ElectronSafeStorage.ts, ElectronProtocol.ts, ElectronApp.ts, apps/desktop/src/shell/DesktopShellEnvironment.ts, apps/desktop/src/settings/DesktopAppSettings.ts, apps/desktop/src/main.ts, plus touched test files.
Checks performed: namespace subpath imports for Effect modules; canonical module order (imports, errors/schemas, Context.Service tag with inline interface, make, layer); exported make/layer naming; Foo["Service"] usage; no new ManagedRuntime.make, runPromise, or runPromiseExit; Layer.succeed occurrences; error classes using Schema.TaggedErrorClass with structural attributes, derived messages, and preserved cause; no compatibility re-export shims left behind.
Notes (not flagged): ElectronProtocol.layerSchemePrivileges is an additional descriptive layer alongside the canonical layer and is acceptable; ElectronSafeStorage.selectedStorageBackend intentionally models unavailability as Option.none() for advisory logging.
🔍 Agent activity — 1 tool call
💭 I'm checking the current state of the PR files to see if the previous issues were resolved. DesktopPreReadyPlatform.ts looks good now with the make/layer and inline Context.Service interface added, and ElectronSafeStorage's make is properly exported. Let me scan through the files to identify any remaining violations. The prior issues look resolved. Moving through the other files now—DesktopEarlyElectronStartup and DesktopStatePaths are straightforward helpers, so those are fine. ElectronProtocol added layerSchemePrivileges, so I need to verify the naming conventions align with other layers in that module. DesktopShellEnvironment added fileSystem to the config, which is properly acquired through the effect system in the make function, though I'm checking how FileSystem is imported. There's a
process.getuid?.()call in installPosixEnvironment that's a module global, but it's not an Effect service so that's acceptable. Now I need to look at the ElectronProtocol export changes, the main.ts modifications, and DesktopApp.ts. There's a potential issue whereresolveEarlyLinuxElectronOptionsFromProcessis called synchronously inside an Effect generator in DesktopApp, reading process env and filesystem directly rather than through the environment services—which could violate the dependency hiding rule, though it's at a documented boundary and arguably acceptable for a pure synchronous read. I should check the DesktopApp context to see if this warrants flagging. I'm checking the main.ts file for layer convention violations, particularly around howresolveEarlyLinuxElectronOptionsFromProcess()is being called within the startup Effect—it's reading from the filesystem and process environment synchronously, which might be circumventing the proper FileSystem and environment service abstractions that the conventions are meant to enforce. Looking at ElectronProtocol.ts next, I noticelayerSchemePrivilegesas an implementation-specific layer name in a service module that already exports alayer, which could be redundant or a naming convention issue. I should verify whether ElectronProtocol properly exports bothmakeandlayer, and check how ElectronSafeStorage.make uses its dependencies. ElectronProtocol has bothlayerandlayerSchemePrivilegesas named layers, which is fine—implementation-specific naming is allowed. But I'm noticingregisterDesktopSchemePrivilegesSyncis exported without any apparent usage, so I need to verify whether it's actually being called internally or if it's dead code. TheconfigureElectronBeforeReadycall toregisterSchemesAsPrivilegedwas removed from main.ts, which is acceptable. In DesktopShellEnvironment, there's a test-only export ofresolveDefaultLinuxDbusSessionBusAddressthat duplicates logic already inlined in the production path—the helper exists only for tests, which is a minor convention issue but not a violation. I'm checking for any misuse ofLayer.succeed,ManagedRuntime, orrunPromisein the changed files, and there's nothing new there. DesktopEarlyElectronStartup imports pure helpers likefromLenientJsonand namespace imports forOptionandSchema, which are fine. DesktopStatePaths is a pure helper module with no service layer concerns. Now I'm looking at the HostProcessPlatform import to verify it follows the same conventions. The try/catch inselectedStorageBackendswallows errors intoOption.nonefor advisory logging, which is borderline against the convention of modeling failures with typed errors in Effect. But since the function deliberately returns an optional type, treating unavailability asnoneis a reasonable design choice, so I won't flag it. Moving on to checkDesktopShellEnvironment.tsfor make and config threading. I'm refactoring howShellEnvironmentConfighandles theFileSystemservice—instead of having it acquired from the environment insideinstallPosixEnvironment, I'm now passing it directly as a config field to helper functions, which aligns with the convention that factories should accept service implementations as explicit dependencies rather than letting them surface implicitly in the Effect type. The issue is that this violates the module's own pattern:spawneris acquired viayield*and provided throughEffect.provideService, sofileSystemshould follow the same approach rather than being hidden in the config struct. This creates a type signature mismatch whereinstallShellEnvironmentonly declaresChildProcessSpawneras a requirement while theFileSystemdependency is concealed, which the check should flag as a violation. I should target line 17 in the diff where thefileSystemfield is added toShellEnvironmentConfig. There's also a minor concern aboutresolveDefaultLinuxDbusSessionBusAddressbeing exported only for tests, though that's acceptable given the behavior change is covered by tests. The boundary layer code likeDesktopPreReadyPlatform.makeusing Electron's imperative APIs is fine, but I need to examine whetherresolveEarlyLinuxElectronOptionsFromProcessreading the filesystem synchronously during startup inDesktopApp.tscrosses into service construction territory where it shouldn't. Looking at the other changes:DesktopEarlyElectronStartup.tsis pure and uses Schema/Option correctly, andElectronSafeStoragenow requiresHostProcessPlatformwhich surfaces as a layer requirement that gets provided in main.ts. The real question is whetherFileSystemcould be acquired directly insideinstallPosixEnvironmentas an Effect, which would make the requirement flow throughinstallShellEnvironmentnaturally without needing the make function to provide it explicitly. Actually, sinceinstallIntoProcessis typed asEffect.Effect<void>in the service shape, the make function needs to provide both the spawner and fileSystem viaEffect.provideService. I'll write up the review comment now and mark this as a failure since there's a clear convention violation. The comment goes on line 17. Since this spans multiple changes—removing the field, adding a yield in the helper, and using provideService in make—I'll describe it rather than provide a diff. I should also verify this wasn't already flagged in prior runs; the earlier comments were on different components, so this is a new finding worth posting. Let me confirm the exact line numbers and submit.
1. modify_pr — One finding: a service instance is threaded through a config record instead of being acquired from the Effect environment.
Agent Credits: 83 credits