Repository navigation
Effect Service Conventions: 1 issue found
Found one Effect service convention violation in the changed scope.
apps/server/src/orchestration-v2/ProviderTurnStartService.ts(lines 436-438): a known tagged failure (ProjectionStoreThreadNotFoundError) is recovered withEffect.catchIf(Schema.is(...)). The conventions require statically known tagged failures to be caught withEffect.catchTags({ ... }), even for a single tag;catchIfis reserved for genuinely structural predicates. This is also the onlycatchIf(Schema.is(...))occurrence in the repo, whereasEffect.catchTagsis used pervasively elsewhere.
An inline review comment with a suggested fix has been posted.
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.
Scope reviewed: changed TypeScript under the configured include patterns, focusing on new/refactored Effect service code.
Checked and cleared:
import { layer as ... }aliases in the newProviderTurnStartService.test.tsandCodexAdapterV2.test.ts— this alias style is an established, repository-wide pattern (used throughoutruntimeLayer.ts,applicationLayer.ts, harnesses, and existing tests), so it is out of scope for this PR and not flagged.Layer.mock(...)usage inProviderTurnStartService.test.ts— legitimate test seams; passing mocked service instances in tests is permitted.- New type
ProviderAdapterV2ExistingSubagentinProviderAdapter.ts— pure data interface, no service/error concerns. RunExecutionService.ts/CodexAdapterV2.tschanges — routing/logic and pure helpers; no new service definitions, error classes, module-global runtimes, orManagedRuntime/runPromiseboundaries introduced.
No violations found related to service definition ordering, error modeling, dependency acquisition, or runtime boundaries.
🔍 Agent activity — 1 tool call
💭 I'm examining the diff to check for Effect service convention violations, starting with the new test file and its imports from the Effect library. Looking at the imports in the test file, I notice several that use aliases to erase the module namespace—specifically
import { layer as idAllocatorLayer }andimport { layer as providerTurnStartLayer, ProviderTurnStartServiceV2 }. The convention flags these kinds of aliases as violations since they hide the original module namespace. I need to verify whether this is an existing pattern in the codebase or a new violation being introduced. I need to focus on the actual service convention violations rather than getting caught up in stylistic details. Let me examine the production code changes to see if there are real issues with how services are being defined or consumed, then check whether the test file's import patterns match what's already established in the codebase. I'm noticing thatProviderTurnStartServiceV2Shapeis pre-existing code that wasn't modified in this PR, so it's outside the scope of what I'm reviewing. Now I'm looking at thecatchIfusage that was added in ProviderTurnStartService.ts, where it's filtering projection store results based on a schema check. The convention is clear here: when catching statically known tagged errors likeProjectionStoreThreadNotFoundError, I should useEffect.catchTagswith an object mapping the tag to its handler, rather thancatchIfwith a schema predicate. This is a violation in the new code that needs to be fixed. I'm noticing thatProjectionStoreThreadNotFoundErroris a tagged error class, and the current implementation usesEffect.catchIfwith a schema check, but the convention would be to useEffect.catchTaginstead to match the error handling pattern. Actually, looking more carefully, the convention is to useEffect.catchTagswith an object mapping the error tag to its handler, so it should beEffect.catchTags({ ProjectionStoreThreadNotFoundError: () => Effect.succeed(null) }). This is a violation in the changed scope since it's a new line added in this PR within a file matching the include pattern. Let me check the exact line number in the file to confirm. I'm checking whethercatchIfwith a schema predicate appears elsewhere in the codebase, though the convention should take precedence regardless of existing patterns. Let me reconsider this approach. The convention explicitly prohibits usingcatchIfwith schema predicates to recover known_tagvariants, which is exactly what's happening here—so this needs to be flagged. Moving on to the other changed files, the test file usesLayer.mockwhich aligns with the convention allowing service instances in tests. However, the imports likeimport { layer as providerTurnStartLayer }andimport { layer as idAllocatorLayer }are erasing the module namespace at a service boundary, which the convention says to flag even in mechanically updated tests. But I should check whether this pattern is already established in the codebase—the existing test fileCodexAdapterV2.test.tsalready useslayer as idAllocatorLayer, so this might be a pre-existing convention in the repo. The strongest, most defensible finding remains thecatchIfwith schema predicate violation. Let me scan the changed files systematically for other instances ofcatchIf,catchTag,Layer.succeed, or similar error-handling patterns that might violate the conventions. I need to check how prevalent thelayer asaliasing pattern is across the codebase to determine if this is truly a violation or an established convention in this repo. Thelayer as Ximport alias pattern is standard throughout the codebase in both production and test files, so that's not worth flagging as an issue in this PR. Now I need to check whether the repo uses thecatchIfpattern with schema predicates elsewhere. I'm noticing this is a new violation introduced in the PR—it's usingcatchIfwith a schema predicate when it should be usingEffect.catchTagsinstead, which is the established convention. Since this is the only instance, it's worth flagging, and I should check how other parts of the codebase handle similar error recovery patterns. I'm noticing thatEffect.catchTagsis the standard pattern used throughout the codebase, so the newcatchIf(Schema.is(...))approach in ProviderTurnStartService.ts stands out as a deviation from both the established convention and the linting rule. This is a valid concern. Let me scan through the diff again to check for any other service definition, error handling, or dependency issues I might have missed, starting with the main production changes around CodexAdapterV. Preparing the inline suggestion with proper indentation to replace the catchIf block with catchTags, targeting lines 436-438.
1. modify_pr — Effect service conventions: one issue found. Known tagged failures should be recovered with Effect.catchTags rather than catchIf + a schema predicate.
💭 Now I'm generating the JSON output for the check run, marking it as a failure since a convention violation was detected.
Agent Credits: 67 credits