|
| 1 | +--- |
| 2 | +title: Effect Service Conventions |
| 3 | +model: claude-opus-4-8 |
| 4 | +effort: high |
| 5 | +input: full_diff |
| 6 | +tools: |
| 7 | + - browse_code |
| 8 | + - git_tools |
| 9 | + - github_api_read_only |
| 10 | + - modify_pr |
| 11 | +include: |
| 12 | + - "apps/**/*.ts" |
| 13 | + - "apps/**/*.tsx" |
| 14 | + - "packages/**/*.ts" |
| 15 | + - "packages/**/*.tsx" |
| 16 | + - "infra/**/*.ts" |
| 17 | + - "infra/**/*.tsx" |
| 18 | +conclusion: failure |
| 19 | +showToolCalls: true |
| 20 | +--- |
| 21 | + |
| 22 | +# Effect service review |
| 23 | + |
| 24 | +Review changed TypeScript and directly affected call sites for the conventions below. Apply them when a pull request creates, moves, refactors, or consumes an Effect service. Do not demand unrelated repository-wide cleanup. Treat these instructions as authoritative when older code differs. |
| 25 | + |
| 26 | +## Imports and module namespaces |
| 27 | + |
| 28 | +- Import Effect library modules from their subpaths as namespaces, for example `import * as Effect from "effect/Effect"` and `import * as Layer from "effect/Layer"`. Flag consolidated named imports from `"effect"` in touched Effect service code. |
| 29 | +- At a service boundary, import the local service module as a namespace and use its public module shape: `WorkspacePaths.WorkspacePaths`, `WorkspacePaths.make`, and `WorkspacePaths.layer`. Flag aliases such as `import { layer as workspacePathsLayer }` that erase the module namespace. |
| 30 | +- Namespace imports are not a blanket rule. Keep named imports for whole packages such as `@t3tools/contracts`, and for modules used only for a pure helper, error, schema, config value, or standalone type. Do not request `import type * as Contracts`. |
| 31 | +- A package subpath that is itself a service module may use a namespace import when callers access its service/tag, `make`, or `layer` members. |
| 32 | +- When a barrel exposes an entire service module, prefer `export * as TokenStore from "./tokenStore.ts"` so consumers can use `TokenStore.TokenStore` and `TokenStore.layer`. Do not individually rename `make` and `layer` exports to simulate a namespace. |
| 33 | + |
| 34 | +## Service definition |
| 35 | + |
| 36 | +- Use the canonical single-file order: imports, error/schema declarations, the `Context.Service` tag with its inline interface, `make`, then `layer`. |
| 37 | +- Keep a service's schemas/errors, `Context.Service` tag, construction, and layer in one canonical module when they form one implementation. |
| 38 | +- Define the service interface inline in the `Context.Service` declaration. Do not retain a standalone `FooShape` or `FooServiceShape` interface/type. |
| 39 | +- Refer to the inferred service interface as `Foo["Service"]`, including in mechanically updated orchestration, MCP, tests, and integration harnesses. |
| 40 | +- Export a real `make` when the module owns construction. Do not create `make = Effect.succeed(...)` solely to force `Layer.effect`. |
| 41 | +- Export the canonical layer as `export const layer = Layer...`. `Layer.effect` is not required: use `Layer.succeed`, `Layer.scoped`, or another appropriate constructor when that matches the implementation. |
| 42 | +- In a concrete implementation module already named for the implementation, use plain `make` and `layer` (for example `BunPtyAdapter.ts` and `NodePtyAdapter.ts`). |
| 43 | +- Keep implementation-specific names when an abstract port module contains one of several possible implementations, for example `makeCloudflaredRelayClient` and `layerCloudflared` in `RelayClient.ts`. |
| 44 | +- `infra/relay/src/db.ts` is an intentional exception: an inline `Layer.succeed(RelayDb, db)` is acceptable without generic `make`/`layer` exports. |
| 45 | + |
| 46 | +## Errors and predicates |
| 47 | + |
| 48 | +- Define service failures with `Schema.TaggedErrorClass` and structured attributes. Derive `message` from those attributes rather than storing an unstructured message as the only data. |
| 49 | +- `Schema.Defect()` is not a substitute for modeling a generic error: its tag, fields, or both must identify the failure structurally, and its `message` must not merely stringify an opaque cause. A semantically precise error tag may preserve a real `cause` without inventing a redundant singleton field when no additional variable context exists; still retain any real path, resource, request, or entity context available at the wrapping site. |
| 50 | +- Capture stable, serializable domain context such as the operation or stage, resource/path or entity identifier, and normalized category/status. Map failures where that context is known instead of wrapping an entire multi-step pipeline in one generic error. Do not add a `detail` field that merely copies `cause.message` and then use it to construct the wrapper message. |
| 51 | +- Keep direct error attributes and log annotations safe and bounded. Do not copy raw wire payloads, command arguments or output, signed URLs, credentials, query strings, fragments, selectors, or arbitrary defect text into `detail`, `reason`, `message`, or a parallel log payload. Preserve the exact underlying value only as `cause`; expose normalized categories plus lengths/counts and safe URL protocol/hostname diagnostics where useful. Logging a sanitized error must not reintroduce a removed legacy `detail` or serialized `cause` field beside it. |
| 52 | +- When translating or wrapping a real failure, preserve the immediate underlying error itself as `cause` alongside the structural fields so the complete error chain and stack remain available. If every construction wraps a failure, `cause` should be required; make it optional only when the same error can legitimately originate without an underlying failure. |
| 53 | +- At a translation boundary, pass through an already structured domain error when it is part of the declared target error channel. Wrap only unknown or genuinely lower-level failures. A static factory or mapper may perform this classification when it is reused and keeps the policy next to the target error type. |
| 54 | +- Derive the wrapper's `message` exclusively from its stable structural attributes, never from `cause`, `cause.message`, or a stringified defect. Do not replace the immediate error with only `error.cause`, erase a structured upstream error into a string, or manufacture an `Error` merely to populate `cause`. Pure validation/domain errors created without an underlying failure do not need a cause. |
| 55 | +- Do not encode the same distinction twice with both a specific error tag and a single-value `operation`, `reason`, `kind`, or `phase` literal. Choose one coherent model: use distinct error classes and omit the redundant discriminator when callers or messages treat the failures as genuinely different, or use one service-level error with a multi-value operation discriminator and a generic message derived from that operation when the failures share the same semantics. |
| 56 | +- Treat an error message exposed through an HTTP/RPC response, persisted state, UI, or another caller-visible boundary as behavior. Preserve those messages during a structural refactor. Existing distinct caller-visible messages are evidence that the failures should normally remain distinct error tags without redundant singleton discriminators, rather than being collapsed into a generic operation error. |
| 57 | +- Split semantically distinct failures into separate error classes when a `reason`, `kind`, `phase`, or similar discriminator is used to choose the user-facing message or drive caller control flow. A discriminator used only for internal diagnostics may remain a field. |
| 58 | +- Use `Schema.Union` of error classes when a shared schema, predicate, or helper type is useful. |
| 59 | +- Export direct schema predicates such as `export const isFoo = Schema.is(Foo)`. Flag a private `Schema.is` constant wrapped by a redundant function with the same signature. |
| 60 | +- Do not introduce a large `switch` or lookup table in an error's `message` getter to model failures that deserve separate error classes. |
| 61 | +- Catch statically known tagged failures with `Effect.catchTags({ ... })`, including when handling only one tag. Do not use `catchIf` with a schema predicate merely to recover one or more known `_tag` variants, and do not use `catchTag`. `Effect.catch` is appropriate when the entire error channel is intentionally handled; `catchIf` remains appropriate for genuinely structural predicates such as inspecting an underlying platform error code. |
| 62 | +- Do not add a helper whose only behavior is `(...args) => new SomeError({ ...args })`, including curried aliases used once with `mapError`. Construct the error at the failure boundary so its attributes and cause remain visible. Keep a mapper only when it performs real normalization, passes through existing domain errors, or adds reusable context/control flow. |
| 63 | +- When a reusable error-to-error translation clearly belongs to the target error type, prefer a descriptive static factory on that error class over a detached production-side switch. Do not force a static method for one-off inline mappings. |
| 64 | + |
| 65 | +## File layout and migrations |
| 66 | + |
| 67 | +- When combining `domain/Services/Foo.ts` and `domain/Layers/Foo.ts`, hoist the result to `domain/Foo.ts`. |
| 68 | +- Delete the old service/layer files. Do not leave compatibility re-export shims. Mechanically update every consumer, including orchestration, MCP, tests, and integration harnesses, to the canonical path. |
| 69 | +- Do not flag genuinely separate implementation/adapter modules merely because they remain in an implementation-oriented directory. |
| 70 | +- Avoid substantive orchestration or MCP redesign in service-cleanup PRs. Mechanical import, layer, and `Service["Service"]` updates are expected when required to remove obsolete paths or shapes. |
| 71 | + |
| 72 | +## Change discipline |
| 73 | + |
| 74 | +- Preserve useful comments, invariants, and specification documentation while moving code. |
| 75 | +- Do not add large tests solely to prove a mechanical refactor. Update existing tests and imports as needed. |
| 76 | +- If backend behavior changes, require focused tests. Use test implementations/layers for external services only; do not mock out core business logic. |
| 77 | +- Do not require `Layer.effect`, universal namespace imports, generic `make`/`layer` names for abstract-port implementations, separate error classes for diagnostic-only fields, or new tests for import-only changes. |
| 78 | + |
| 79 | +## Reporting |
| 80 | + |
| 81 | +Report only concrete violations introduced or retained in the pull request's changed scope. Prefer precise inline comments on the smallest relevant line range and state the expected fix. A clear convention violation may fail the check. Do not fail for optional style preferences or unrelated legacy code. |
| 82 | + |
| 83 | +This check defaults to failure. When there are no findings, stop immediately and make the entire final response exactly `All clear` on one line. Do not add a title, explanation, punctuation, Markdown, JSON, or trailing analysis, and do not continue reasoning after deciding the review is clean. |
0 commit comments