feat: Add Pi Coding agent as a provider - #3947

💭 I'm checking the diff for violations of Effect service conventions, particularly the rule about defining service interfaces inline within Context.Service declarations rather than as separate standalone types. Looking at the new files like piRuntime.ts, I need to see if they're following this pattern correctly. The issue is that PiRuntimeShape is defined as a standalone interface and then used in the Context.Service declaration, which violates the convention. The convention also says to refer to the inferred service interface as Foo["Service"] rather than using external type references like PiRuntimeShape in test doubles and other places. Since PiRuntimeShape is newly introduced in this pull request, it's a violation that should be flagged. I need to verify whether PiRuntimeShape is purely the service interface or if it serves some other purpose that might justify keeping it separate. Looking at the make/layer conventions, the code exports PiRuntimeLive instead of the canonical layer name, which also appears to be a violation of the established pattern. The PiRuntimeShape interface is the real issue here—the check instructions explicitly say not to keep standalone shape interfaces, but the repo already has ProviderAdapterShape and similar patterns, so I need to decide whether this is an established convention I should respect or a violation I should flag. The instructions treat the check as authoritative when there's a conflict, so the shape interface probably needs to go. Looking at where PiRuntimeShape is actually used, it shows up across multiple test files as the type annotation for test doubles, which confirms it's being referenced externally and would need to be addressed if I'm flagging this as a violation. The convention states that service interfaces should be accessed via PiRuntime["Service"] rather than using a standalone shape type. I need to verify whether Context.Service actually supports that bracket notation for accessing the service type, since that's the crux of whether this is truly a violation or if the convention applies differently to this pattern. Looking at PiAdapterShape usage in PiAdapter.ts — it's consuming the existing ProviderAdapterShape rather than defining a new service, so that's out of scope. The makePiAdapter function returns an object satisfying PiAdapterShape and gets provided as a Context.Service in tests, but I need to determine if it's actually functioning as a service or just a factory. Now checking the error conventions against PiRuntimeError, which uses Data.TaggedError instead of the recommended Schema.TaggedErrorClass — that might be a convention deviation, though Data.TaggedError could be widely used in the codebase. The detail field also needs scrutiny to ensure it's not just duplicating cause.message. I'm noticing that the detail field is constructing an error message that includes the full command with its arguments, which could leak sensitive information like prompt text. That's a potential security issue worth flagging. There's also some redundancy in how PiRuntimeError uses both an operation field and a detail field to encode similar distinctions. Looking at the message derivation, PiRuntimeError relies on Data.TaggedError which doesn't auto-derive messages from attributes the way the convention expects. Using Schema.TaggedErrorClass would be more aligned with that pattern. The static is method using P.isTagged is a bit borderline—it's not redundantly wrapped, but exporting direct schema predicates would be cleaner. The clearest violation though is the standalone PiRuntimeShape interface for the new PiRuntime service. Let me verify that by checking the actual file to confirm it hasn't already been addressed and to get the exact line numbers. I also need to look at how errors are defined elsewhere in the repo—specifically whether Schema.TaggedErrorClass is the established pattern—to determine if PiRuntimeError using Data.TaggedError is a genuine deviation worth reporting. I need to restructure how the repository is passed—it should be an object containing the URL rather than just a string. I'll use the head commit SHA. I'm seeing the repo's error handling pattern now — errors use Schema.TaggedErrorClass with structured attributes and a message getter, plus an optional cause field for defects. The ProviderAdapterShape<TError> is a generic adapter contract rather than a Context.Service, which works fine since it's parametrized and meant as an interface for adapters rather than a service tag. The new PiRuntime service defines PiRuntimeShape as a standalone interface, which breaks the repo convention of defining service interfaces inline within the Context.Service declaration. I need to check how OpenCodeRuntime handles this — if it's an established local pattern, the violation becomes less clear, but since the instructions say to treat new code as in-scope, I should still flag it. Also, PiRuntimeError uses Data.TaggedError while the repo's error convention uses Schema.TaggedErrorClass, which is another deviation I need to verify against the existing runtime service definitions. I need to check how OpenCodeRuntime is structured as a Context.Service and verify it uses OpenCodeRuntimeShape, while also confirming that OpenCodeRuntimeError extends the tagged error pattern with its operation, cause, and detail fields. I'm setting up PiRuntime to follow the same pattern as OpenCodeRuntime, defining the service interface, creating a tagged error type with metadata fields and a type guard, establishing the live layer implementation, and starting to build the factory function. Now I'm weighing whether to flag this pattern as a violation. The guidelines say not to demand unrelated cleanup, but they're also explicit about avoiding standalone FooShape interfaces. Since PiRuntime is newly introduced and mirrors the established sibling pattern, flagging it would force a divergence from what's already in the codebase. The check allows for not failing on optional style preferences or legacy code, and this seems like a case where consistency with the existing module takes precedence over the stricter guideline. The conventions explicitly state "when older code differs," which suggests new code should follow the updated patterns regardless of what existing sibling services do. Inlining PiRuntimeShape into the Context.Service definition is a concrete, contained fix—I'd need to update the type references throughout the make internals and tests, but that's manageable. The real question is whether PiRuntimeShape serves a legitimate purpose beyond just following the old pattern, or if it's genuinely necessary for the implementation. ured about whether this is actually the intended violation to flag, or if the code is acceptable as-is. The PR introduces PiRuntimeShape as a new standalone type that mirrors the older OpenCodeRuntime pattern, and the conventions are explicit: define the service interface inline and don't keep a separate FooShape. This looks like a textbook case the conventions describe. But I'm weighing whether flagging it violates the principle of not demanding unrelated cleanup—except PiRuntimeShape isn't legacy code, it's newly written in this PR, so it's fair game to address. There's also PiRuntimeError using Data.TaggedError when the repo convention calls for Schema.TaggedErrorClass, which I see in the Errors.ts file. The conventions are authoritative over older patterns, so even though OpenCodeRuntimeError uses the older approach, that shouldn't stop me from flagging the new code. The real question is whether I should flag both issues or focus on the clearest one given how closely the new service mirrors the existing sibling. The error handling pattern in PiRuntimeError mirrors OpenCodeRuntimeError closely enough—both use structured attributes and preserve cause chains—so the real question is whether the deviation from Schema.TaggedErrorClass is significant enough to flag. Since neither implementation defines a custom message getter and the semantic requirements around structured attributes and cause preservation are met, I'm reconsidering whether to raise this as a finding at all. The convention is explicit: don't keep a standalone PiRuntimeShape when the service interface should be defined inline in Context.Service. The codebase already follows this pattern with TextGeneration["Service"], so this is a clear violation I should flag. I'm also considering whether to flag the consumers importing PiRuntimeShape, but that depends on whether they're part of the same violation or separate concerns. Let me locate the exact line numbers for PiRuntimeShape and the class declaration in piRuntime.ts so I can pinpoint the issue precisely. I'm checking the specific line range to see the exact structure of the code. I've identified that PiRuntime is a new Context.Service with a standalone PiRuntimeShape interface, mirroring the OpenCodeRuntime pattern. Now I'm weighing whether to flag this against the convention that requires service interfaces to be defined inline rather than as separate standalone types, especially since this is a newly introduced pattern in this PR. The authoritative-override clause in the conventions tips me toward flagging it—the "Effect Service Conventions" check specifically targets this exact scenario. However, I'm also considering whether to flag PiRuntimeError, which uses Data.TaggedError instead of Schema.TaggedErrorClass. This is more ambiguous since the codebase has two established patterns: Errors.ts uses Schema.TaggedErrorClass while the runtime modules use Data.TaggedError, so I'm less certain about flagging that one. I'm also noticing line 627 copies command arguments into the detail field, which mirrors what OpenCodeRuntime does and doesn't expose sensitive information—the args are just CLI flags like provider and model settings, not secrets. Given that this pattern is consistent with the sibling module and the arguments aren't sensitive, I'm inclined not to flag this either. So I'm settling on one clear finding: the standalone PiRuntimeShape interface on the new PiRuntime service, though I'm still weighing whether to flag it at all given how strongly the established pattern pulls toward accepting it. I'll flag the standalone PiRuntimeShape as a violation of the service-definition convention, targeting the interface declaration with guidance to inline it within the Context.Service declaration instead. The comment will be concise and reference the established pattern. Now I'm setting up the GitHub PR review with the inline comment positioned at lines 590-601 on the right side, along with a brief summary in the review body.

1. modify_pr — Effect service conventions review: one finding on the new PiRuntime service definition.

💭 The review is in, so I need to finalize the JSON output. I had a small formatting issue with the provider path that I'm correcting now, and I'll make sure the summary paths are properly structured.