| 1 | --- |
| 2 | title: Effect Service Conventions |
| 3 | model: gpt-6-sol |
| 4 | effort: max |
| 5 | input: incremental |
| 6 | tools: |
| 7 | - browse_code |
| 8 | - modify_pr |
| 9 | include: |
| 10 | - "apps/**/*.ts" |
| 11 | - "packages/**/*.ts" |
| 12 | - "infra/**/*.ts" |
| 13 | exclude: |
| 14 | - "**/*.test.ts" |
| 15 | requires: |
| 16 | - Check |
| 17 | maxBudgetPerRun: 5 |
| 18 | maxBudgetPerPR: 25 |
| 19 | conclusion: neutral |
| 20 | showToolCalls: true |
| 21 | --- |
| 22 | |
| 23 | # Effect service review |
| 24 | |
| 25 | Review changed TypeScript for the conventions below. They apply when a pull request creates, moves, refactors, or consumes an Effect service, or adds server behavior. Authors read the same rules in `docs/internals/effect-services.md`; keep the two in step. Review only the lines the PR changed; older code in the same file that predates these conventions is not a finding. Do not demand repository-wide cleanup. |
| 26 | |
| 27 | ## Imports and module namespaces |
| 28 | |
| 29 | - Import Effect modules from their subpaths as namespaces: `import * as Effect from "effect/Effect"`, `import * as Layer from "effect/Layer"`. Flag named imports from the bare `"effect"` package. |
| 30 | - At a service boundary, import the local service module as a namespace and use its public shape: `WorkspacePaths.WorkspacePaths`, `WorkspacePaths.make`, `WorkspacePaths.layer`. Flag aliases such as `import { layer as workspacePathsLayer }` that erase the namespace. |
| 31 | - Named imports stay correct for whole packages such as `@t3tools/contracts` and for modules used only for a pure helper, error, schema, config value, or type. Do not request `import type * as Contracts`. |
| 32 | - When a barrel exposes a whole service module, prefer `export * as TokenStore from "./tokenStore.ts"` over individually renamed `make` and `layer` exports. |
| 33 | |
| 34 | ## Placement |
| 35 | |
| 36 | - Flag new or broadened feature logic in a transport handler: a WebSocket RPC handler in `apps/server/src/ws.ts`, an HTTP route handler, or an MCP tool handler. A handler decodes input, calls one service method, and maps the service's errors to the transport's error. Filesystem, Git, process, or persistence work, folder naming, multi-step command dispatch, retries, or rollback inside a handler belongs in a service method; ask for it to move to the domain's existing service, or a new one when none owns the domain. Existing inline handlers are legacy; flag only changed lines. |
| 37 | - Flag a new exported free function that does a server capability's effectful work (filesystem, Git, processes, persisted state) and is called from a handler, when it should be a service method. Plain functions for pure work (formatting, naming, parsing) are fine. |
| 38 | |
| 39 | ## Service definition |
| 40 | |
| 41 | - One canonical module per service in this order: imports, error and schema declarations, the `Context.Service` tag with its interface inline, `make`, then `layer`. |
| 42 | - Define the interface inline in `Context.Service`. Do not add a standalone `FooShape` interface; refer to the inferred type as `Foo["Service"]`. |
| 43 | - Define a real `make` when the module owns construction, and export it only when another module imports it; knip fails CI on an unused export, so do not ask for an export nothing uses. Do not write `make = Effect.succeed(...)` only to force `Layer.effect`; use `Layer.succeed`, `Layer.scoped`, or whichever constructor matches. |
| 44 | - Use plain `make` and `layer` in a module named for its implementation (`BunPtyAdapter.ts`). Keep implementation-specific names when one abstract port module holds several implementations (`makeCloudflaredRelayClient`, `layerCloudflared` in `RelayClient.ts`). `infra/relay/src/db.ts` may keep its inline `Layer.succeed(RelayDb, db)`. |
| 45 | - When a service moves, delete the old files and update every consumer, including orchestration, MCP, tests, and integration harnesses. Do not leave compatibility re-export shims. |
| 46 | |
| 47 | ## Dependency acquisition and runtime boundaries |
| 48 | |
| 49 | - Production service construction acquires its Effect dependencies from the environment with `yield* Foo.Foo`, and `make`/`layer` types expose those requirements. Flag a factory that takes `Foo["Service"]` (or an object of Effect-returning methods) as a parameter when that value is a service dependency. Passing service instances explicitly in tests is fine; passing pure configuration, immutable domain values, or deliberate callback strategies is not service injection. |
| 50 | - Do not hide dependencies in module globals, closures over singleton services, or a `Layer.succeed` whose implementation calls runtime-backed or imperative APIs. |
| 51 | - `ManagedRuntime.make`, `runPromise`, and `runPromiseExit` belong at application or framework boundaries: React, native callbacks, CLI, HTTP adapters. Flag them in domain services, repositories, persistence, and service constructors. A named imperative adapter may bridge an Effect service into a Promise API but must not become a dependency of another Effect service. |
| 52 | - Do not create per-feature managed runtimes or Atom runtimes to hand the same owned resource to several consumers. Compose the resource once in an application-owned layer and provide its context to integration runtimes. |
| 53 | - When acquisition can fail and callers need fallback behavior, keep the failure typed in Effect (an error in the service operation or an explicit optional-service layer) rather than bypassing the layer through an imperative runtime. |
| 54 | |
| 55 | ## Errors |
| 56 | |
| 57 | - Define service failures with `Schema.TaggedError` and structured attributes: operation or stage, resource path or entity identifier, normalized category or status. Derive `message` from those attributes only. Never derive it from `cause`, `cause.message`, or a stringified defect, and do not add a `detail` field that copies `cause.message`. |
| 58 | - When wrapping a real failure, keep the immediate underlying error as `cause` so the chain and stack survive. Make `cause` required if every construction wraps a failure. Pure validation or domain errors created without an underlying failure need no cause. |
| 59 | - Keep attributes and log annotations safe and bounded: no raw wire payloads, command arguments or output, signed URLs, credentials, query strings, or arbitrary defect text. Preserve the exact value only as `cause`; expose normalized categories, lengths, counts, and safe URL protocol or hostname where useful. |
| 60 | - At a translation boundary, pass through an already structured domain error when it is part of the target error channel; wrap only unknown or lower-level failures. Map failures where the context is known instead of wrapping a whole multi-step pipeline in one generic error. |
| 61 | - Do not encode the same distinction twice with both a specific error tag and a single-value `operation`, `reason`, `kind`, or `phase` literal. Split into separate error classes when a discriminator drives caller control flow or the user-facing message; a discriminator used only for diagnostics may stay a field. Caller-visible messages exposed through HTTP, RPC, persisted state, or UI are behavior and must survive a structural refactor. |
| 62 | - Do not add a helper whose only behavior is `(...args) => new SomeError({ ...args })`. Construct the error at the failure boundary. Keep a mapper only when it performs real normalization, passes through domain errors, or adds reusable context; when such a mapper belongs to the target error type, prefer a static factory on that class. |
| 63 | - Export predicates directly as `export const isFoo = Schema.is(Foo)`. Flag a private `Schema.is` constant wrapped by a function with the same signature. |
| 64 | - Catch statically known tagged failures with `Effect.catchTags({ ... })`, including for a single tag; do not use `catchTag` or `catchIf` with a schema predicate for that. `Effect.catch` is fine when the whole error channel is handled; `catchIf` is fine for structural predicates such as a platform error code. |
| 65 | |
| 66 | ## Change discipline |
| 67 | |
| 68 | - Every new or broadened directive that disables a lint, type-checker, LSP, or static-analysis diagnostic needs an adjacent comment explaining why. The directive itself is not an explanation; a missing one is a concrete violation. |
| 69 | - If backend behavior changes, require focused tests that use test layers for external services only, never mocks of core business logic. Do not require new tests for mechanical refactors or import-only changes. |
| 70 | - Do not require `Layer.effect`, universal namespace imports, generic `make`/`layer` names for abstract-port implementations, or separate error classes for diagnostic-only fields. |
| 71 | |
| 72 | ## Reporting |
| 73 | |
| 74 | Report only violations introduced by changed lines. Post each as a precise inline comment on the smallest relevant range and state the expected fix. A clear convention violation may fail the check; optional style preferences and untouched legacy code may not. |
| 75 | |
| 76 | When there are no findings, make the entire final response exactly `All clear` on one line with nothing else. |