💭 I'm working through the scope of what needs reviewing here — the PR includes changes from a few commits, but there's some ambiguity about whether to focus on the specific range the author mentioned or the full merge-base comparison, and I need to figure out which diff actually represents the changes to evaluate for Effect service conventions. Looking at the follow-up commits, I'm spotting violations in GitHubPullRequestCli.ts where custom errors are being constructed with cause fields that just wrap plain Error objects with embedded messages, which goes against the convention of not manufacturing Errors just to populate the cause field. I'm noticing that GitHubPullRequestReadError is being misused here — it's meant specifically for decode failures with a real Schema.Defect() cause, but the new code is repurposing it for three unrelated conditions (missing refs, oversized files, binary files) by artificially creating Error objects as causes. This breaks both the documented constraint against manufacturing errors just to fill the cause field and the pattern the codebase already established elsewhere. The real issue is that the file path context is getting buried in a synthetic error message rather than being captured as a proper structural field on the error class itself, which means that useful domain information is only accessible through string parsing. I should verify the exact line numbers in the current code to add inline comments, then check whether GitLab has the same problem and whether GitLabMergeRequestReadError also requires a cause. Now Looking at the diff for line 527 in getMergeRequestDiff, this truncation is actually pre-existing from the base PR #4849, not something new in the follow-up. Since the whole GitLabPullRequestCli.ts file is new in that base PR, the pattern already exists in the aggregate diff against main, which means it's technically part of the changed scope. The merge base for this check is main, so everything from #4849 is included in the comparison. For the cause: new Error(...) violations, these do seem like genuine convention breaches — the code is manufacturing errors to satisfy the required cause field for decode-oriented error classes when there's no underlying failure (truncated output, binary content, missing refs). The convention explicitly says pure validation or domain errors created without an underlying failure shouldn't need a cause, and the path context is only embedded in the synthetic message rather than captured as stable, serializable domain context. Looking at the other findings: GitLab's getMergeRequestDiffFileContents uses return yield* Effect.fail(new GitLabDiffCommitError(...)) which is just a style choice, not a convention issue. For Schema.Defect(), GitHubPullRequestReadError's tag and operation field identify the failure structurally and the message doesn't stringify the cause, so that's acceptable. The PullRequestService.diffFileContents error I'm noticing a catchTag usage at line 1052 in GitHubPullRequestCli.ts that violates the convention of using catchTags with an object instead. I need to check if this line falls within the diff range by examining whether the file was modified relative to main. I'm looking at a catchTag usage that was introduced in PR #4849, which violates the project's convention of using catchTags with an object pattern instead. Since this code change falls within the PR's diff relative to the merge base, it's a clear violation that should be flagged by the check. The entire pullRequest directory is new to this PR, so I should do a thorough review of the Effect conventions across these ~10 new server modules. I'll focus on checking the import patterns—confirming no named imports from "effect"—and validating the Context.Service structure. Since I'm spotting two issues in the GitHubPullRequestCli file where error handling is inconsistent. Lines 760-765 are creating a generic Error as a cause for non-decode failures, but this should be its own dedicated error type with proper fields like the other GitHub-related errors. The same problem appears at lines 790-799, where additional context like path and limit information should be part of the error structure rather than buried in a synthetic cause. Looking at the error handling in that file section, I'm seeing how a non-decode failure gets wrapped in the decode error structure with a manufactured cause field—the refs read actually succeeded but didn't return any usable SHAs, so the code synthesizes an Error just to meet the required cause property of the GitHubPullRequestReadError type. The message ends up as "returned an unreadable getPullRequestDiffFileContents response", and I notice the sibling error types above it (GitHubDiffCursorError, GitHubDiffCommitError, GitHubViewerLoginUnavailableError) follow a cleaner pattern with dedicated tagged errors that have structural fields and no synthetic cause. I'm thinking a similar dedicated error like GitHubDiffRevisionsUnavailableError with command, cwd, and number fields would be more consistent, though that would require adding a new class rather than just a diff. Moving to the readFile failure section, I'm seeing two distinct failure modes—exceeding the 1 MB expansion limit and binary files—encoded only in the text of a manufactured Error, with the file path buried in that string instead of being a structural attribute. A dedicated error like GitHubDiffFileUnavailableError with a reason field that's either "too-large" or "binary", plus a path property, would make this much clearer and let the message derive from those fields rather than being synthetic.

1. modify_pr — ## Effect service conventions Reviewed the Effect service code in this PR's changed scope, focused on the follow-up commits (92f8ba5e1..9393ecc32) that add the pullRequests.diffFileContents path through the provider CLIs, provider port, service, RPC and client runtime. Service shape is fine: Context.Service tags with inline interfaces, namespace imports from effect/* subpaths, make + layer in canonical order, Foo["Service"] member types, dependencies acquired with yield* Foo.Foo, no ManagedRuntime/runPromise outside application boundaries, and the optional getDiffFileContents port member is guarded by the provider capability rather than faked. Four findings, all the same convention: the new non-decode failures in getPullRequestDiffFileContents / getMergeRequestDiffFileContents are funnelled into the decode-oriented *ReadError with a manufactured new Error(...) as cause, which both invents a cause where no failure occurred and leaves the real context (file path, why the read is refused) only inside a string. Both files already model this class of outcome as its own tagged error (GitHubDiffCursorError, GitHubDiffCommitError, GitLabDiffCommitError, GitLabViewerUnavailableError), so the fix follows the sibling pattern.