Repository navigation
Effect Service Conventions: 2 issues found
The new Effect service added by this PR does not follow the canonical service-module conventions.
apps/server/src/persistence/Services/ProjectionQueuedTurns.ts— retains a standaloneProjectionQueuedTurnRepositoryShapeinterface instead of defining the service interface inline in theContext.Servicedeclaration, and splits the tag from its construction/layer instead of living in one canonical module (apps/server/src/persistence/ProjectionQueuedTurns.ts).apps/server/src/persistence/Layers/ProjectionQueuedTurns.ts— owns construction but does not exportmake, exports the layer asProjectionQueuedTurnRepositoryLiveinstead oflayer, and annotates members viaProjectionQueuedTurnRepositoryShape[...]rather thanProjectionQueuedTurnRepository["Service"][...].
Both findings were posted as inline review comments.
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.
Methodology: reviewed every changed TypeScript file in scope plus the directly affected call sites (ProviderCommandReactor.ts, ProjectionPipeline.ts, ProjectionThreadMessages service/layer, contracts, client-runtime, web/mobile UI).
Checks that passed:
- Effect library modules are imported as subpath namespaces; no consolidated named imports from
"effect".import { AsyncResult } from "effect/unstable/reactivity"matches established repo-wide usage. ProviderCommandReactorand the projection pipeline acquireProjectionQueuedTurnRepositoryfrom the environment withyield* ProjectionQueuedTurnRepository; no service instances are passed as constructor parameters in production code.- No new
ManagedRuntime.make,runPromise, orrunPromiseExitin domain/persistence code;Effect.runPromiseappears only in tests. - No new error classes, so no
Schema.TaggedErrorClass/ discriminator /causeissues.Effect.catchCausewithCause.hasInterruptsOnlyfor the queued-turn recovery is a structural predicate, which is acceptable. - Backend behavior changes are covered by focused tests (
decider.queuedTurns.test.ts,ProviderCommandReactor.test.ts, migration041test,threadReducer.test.ts).
Context for the findings: the repository already contains canonical single-file services (persistence/AuthSessions.ts, persistence/AuthPairingLinks.ts, persistence/ProviderSessionRuntime.ts, the latter leaving only a @deprecated alias behind in Layers/). The legacy Services/ + Layers/ split for other projection repositories is pre-existing code and was not flagged.
🔍 Agent activity — 1 tool call
💭 Looking at the diff, I'm noticing some Effect service convention violations in the new projection queue turns file. The code is defining a separate
ProjectionQueuedTurnRepositoryShapeinterface instead of keeping it inline with theContext.Servicedeclaration, which goes against the established pattern. Additionally, the file structure should be consolidated into a singlepersistence/ProjectionQueuedTurns.tsfile rather than split acrossServices/andLayers/directories. I'm also considering whether this new service should follow the existing repository pattern of splitting Services and Layers, or if the authoritative conventions should take precedence. Since this is a newly created service and the instructions are clear about applying the canonical convention when creating new Effect services, the split pattern here appears to be a violation that should be addressed. Additionally, I'm noticing thatProviderCommandReactorLiveusesLayer.provideMergeto inject theProjectionQueuedTurnRepositoryLivedependency directly, which bakes in a specific implementation rather than exposing the requirement in the layer's type signature — that's another potential violation of the production service construction pattern. I'm noticing the persistence directory still maintains the Services/Layers split for projections, while some files like AuthSessions.ts and AuthPairingLinks.ts have already been migrated to the new pattern. Since the instructions say to apply conventions when creating or consuming an Effect service and treat new patterns as authoritative, I need to decide whether to flag this inconsistency in the new ProjectionQ service. The new service splits into separate files and uses a standalone interface instead of inlining it in Context.Service, which diverges from the instructions. While the surrounding sibling repositories all follow the split pattern, the guidelines are explicit about treating new conventions as authoritative. This seems like a legitimate finding to flag, though I should verify by examining the migrated examples and the new files more closely to confirm what the canonical pattern actually is. I'm looking at the established pattern in this repo where migrated modules like AuthSessions.ts are structured as single files containing the interface, make function, and layer export all together. Let me check what the layer export is actually named in AuthSessions.ts. I found the canonical pattern in the codebase—the newProjectionQueuedTurnsservice follows the legacy split approach with a standaloneProjectionQueuedTurnRepositoryShapeinterface rather than defining it inline withinContext.Service. The service implementation should be consolidated into a single file. Looking at the actual implementation, the Layers file is missing the exportedmakefunction, and it's exporting the layer asProjectionQueuedTurnRepositoryLiveinstead of the standardlayername. There's also a type inconsistency where it's usingProjectionQueuedTurnRepositoryShape["upsert"]instead of the proper service type path. I'm also noticing thatProviderCommandReactoris directly baking in theProjectionQueuedTurnRepositoryLiveimplementation throughLayer.provideMerge, which violates the convention that production services should acquire their dependencies from the environment usingyield*rather than hardcoding concrete implementations. The error handling and schema validation patterns look consistent with the rest of the codebase, so those aren't issues. ThecatchCauseandSchema.ischecks follow existing patterns in other layer files. I'm checking whether importingAsyncResultas a named import from theeffect/unstable/reactivitysubpath violates the convention, since it's not a consolidated import from the root"effect"package—I should verify how the rest of the codebase handles similar imports. TheAsyncResultimport is standard across the codebase, so that's not an issue. The real problem is that this new Effect service was created using the legacy split-file pattern with a separate Shape interface and...Livelayer naming, but the established convention—already followed in migrated modules like the auth and provider session services—requires everything in a single canonical file with an inline interface, an exportedmakefunction, andexport const layer. The style guide explicitly mandates this approach, so I need to determine if this deviation warrants a failure. There's a counterargument: the instruction warns against demanding unrelated repository-wide cleanup, and this new service mirrors the pattern used in twelve sibling projection repositories that feed into the ProjectionPipeline. But the authoritative guidance is clear, so I'll flag this as one finding with two inline comments—one on the Services file for the split structure and Shape interface, and one on the Layers file for the incorrect layer naming and missing make export. Since the check defaults to failure when findings exist, I'll mark it as such. Before finalizing, though, I should verify whether the repository has documented conventions in something like AGENTS.md that explicitly require new persistence projections to follow the Services/Layers pattern. This looks good for flagging — the repo follows a single canonical module pattern, and the new service uses the legacy split approach. Now I'm checking if there are other convention issues in the diff, particularly around howProviderCommandReactorusesLayer.provideMerge(ProjectionQueuedTurnRepositoryLive)to hide the requirement, and whetherProviderCommandReactorLivealready does this with other repositories elsewhere in the codebase. TheprovideMergepattern is consistent with howProjectionPipelinealready handles things, so that's fine. BothProviderCommandReactorLiveandProjectionPipelineprovide their own instances ofProjectionQueuedTurnRepositoryLive, but since the repository layer is stateless and SQL-backed, they're accessing the same database anyway — no violation there. I'm also scanning for other potential issues: the schema definitions look correct, no new errors, no misuse ofLayer.succeed, andrunPromiseonly appears in tests. One thing to flag is that the new Layers file referencesProjectionQueuedTurnRepositoryShape["upsert"]directly, which ties back to the standalone Shape convention issue I already noted. I need to post two inline comments suggesting consolidation: first, the Shape interface inServices/ProjectionQueuedTurns.tsshould be inlined into theContext.Servicedeclaration with all schemas, tags, and layer definitions in one canonical module. Second, theLayers/ProjectionQueuedTurns.tsfile is missing explicitmakeandlayerexports that should be added to the canonical module. I'm determining the exact line ranges for these comments now — the Services file is 69 lines total, so I'll target lines 46-69 for the Shape interface, and the Layers file needs a comment around themakedefinition and the final exports. I'm composing a review with two inline comments—one on the Services file covering lines 46-69, and another on the Layers file for lines 194-197 addressing the unexportedmakefunction and the naming convention issue with the Shape property accessor. I'll mark this as a failure state with the title "Effect Service Conventions: 2 issues found" to highlight the convention violation in the newly created service.
1. modify_pr — Reviewed the new Effect service introduced by this PR (ProjectionQueuedTurnRepository) and the touched service consumers. Two convention violations in the new service module layout; the rest of the changes (namespace imports, yield* Foo.Foo dependency acquisition in ProviderCommandReactor/ProjectionPipeline, no new runtimes or errors) look consistent with the conventions.
Agent Credits: 189 credits