refactor(opencode): add Effect-native process wrapper - #1397
Conversation
Astro-Han
added
P2
|
Warning Review limit reached
More reviews will be available in 51 minutes and 36 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesProcess Effect Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/opencode/test/util/process.test.ts (1)
22-27: Use the Effect test harness instead ofEffect.runPromise(...)in the test body.This test exercises an Effect-native workflow (
Process.runEffect) with a spawned child process, so it should run viatestEffect(...)withit.live(...)rather than rawtest(...)+Effect.runPromise(...). ImporttestEffectfromtest/lib/effect.ts, defineconst it = testEffect(Layer.allSucceed(...) /* or minimal layer */)near the top of the file, then replace the test withit.live("captures stdout and stderr through the Effect path", () => Effect.gen(function* () { ... })).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/util/process.test.ts` around lines 22 - 27, The test for Process.runEffect should use the Effect test harness instead of raw test() with Effect.runPromise(). Import testEffect from test/lib/effect.ts and create an it constant at the top of the file using testEffect(Layer.allSucceed(...)). Replace the test() function declaration with it.live("captures stdout and stderr through the Effect path", ...) and convert the test body to use Effect.gen() syntax instead of await Effect.runPromise(), yielding the Process.runEffect call and asserting on the result within the generator function.Sources: Coding guidelines, Learnings
packages/opencode/src/util/process.ts (1)
356-357: 💤 Low valueConsider using
makeRuntimefromsrc/effect/run-service.ts.The coding guidelines recommend using
makeRuntimewhich provides a sharedmemoMapfor layer deduplication. The current implementation usesManagedRuntime.makedirectly. This may be intentional ifProcessis a foundational module that needs to avoid circular dependencies, but if not, consider aligning with the standard runtime pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/util/process.ts` around lines 356 - 357, The runtime is being created using ManagedRuntime.make(defaultLayer) directly instead of the recommended makeRuntime function from src/effect/run-service.ts, which provides a shared memoMap for layer deduplication. Unless this Process module is a foundational module that needs to avoid circular dependencies, replace the ManagedRuntime.make(defaultLayer) call with makeRuntime() from src/effect/run-service.ts, ensuring the runtime variable and runPromise function continue to work as expected.Source: Coding guidelines
packages/opencode/src/pty/index.ts (1)
172-176: ⚡ Quick winUse
Effect.fnoperator arguments instead of outer.pipe()here.Line 172-Line 176 can pass
Effect.orDiedirectly toProcess.terminateTreeEffect(...)to match the repo’s Effect composition convention.♻️ Proposed refactor
- yield* Process.terminateTreeEffect({ + yield* Process.terminateTreeEffect({ pid: session.process.pid, signalRoot: (signal) => session.process.kill(signal), waitForExit: exited, - }).pipe(Effect.orDie) + }, Effect.orDie)As per coding guidelines, “Use
Effect.fn("Domain.method")... these accept pipeable operators as extra arguments to avoid unnecessary outer.pipe()wrappers”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/pty/index.ts` around lines 172 - 176, The code is using an outer `.pipe(Effect.orDie)` wrapper after the `Process.terminateTreeEffect()` call, but according to the repo's Effect composition convention, this operator should be passed directly as an argument to `Process.terminateTreeEffect()` instead. Refactor the call to `Process.terminateTreeEffect()` to accept `Effect.orDie` as a direct argument within the function call, eliminating the need for the outer `.pipe()` wrapper while maintaining the same functionality.Source: Coding guidelines
packages/opencode/src/tool/shell.ts (1)
610-618: ⚡ Quick winApply the same
Effect.fncall style in both abort/timeout branches.Line 610-Line 618 should pass
Effect.orDieas an argument toProcess.terminateTreeEffect(...)instead of chaining.pipe(...)for consistency with the repo Effect style.♻️ Proposed refactor
- yield* Process.terminateTreeEffect({ pid: handle.pid, waitForExit: Effect.runPromise(handle.exitCode) }).pipe( - Effect.orDie, - ) + yield* Process.terminateTreeEffect( + { pid: handle.pid, waitForExit: Effect.runPromise(handle.exitCode) }, + Effect.orDie, + ) ... - yield* Process.terminateTreeEffect({ pid: handle.pid, waitForExit: Effect.runPromise(handle.exitCode) }).pipe( - Effect.orDie, - ) + yield* Process.terminateTreeEffect( + { pid: handle.pid, waitForExit: Effect.runPromise(handle.exitCode) }, + Effect.orDie, + )As per coding guidelines, “Use
Effect.fn("Domain.method")... these accept pipeable operators as extra arguments to avoid unnecessary outer.pipe()wrappers”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/tool/shell.ts` around lines 610 - 618, Both the abort branch (around line 610) and timeout branch (around line 615) currently use Process.terminateTreeEffect() with a chained .pipe(Effect.orDie) call. Refactor both instances to pass Effect.orDie directly as an argument to the Process.terminateTreeEffect() function call instead of chaining it with .pipe(), removing the outer .pipe() wrapper entirely. This applies the consistent Effect style used throughout the repository where pipeable operators are passed as extra arguments to effect functions.Source: Coding guidelines
packages/opencode/src/session/prompt.ts (1)
1498-1505: ⚡ Quick winInline operators into
Process.textEffect(...)instead of piping the result.Line 1498-Line 1505 should use
Effect.fn’s operator-argument form to avoid the extra.pipe(...)wrapper.♻️ Proposed refactor
- Process.textEffect([shellCmd], { shell: sh, nothrow: true }).pipe( - Effect.map((result) => result.text), - Effect.orDie, - ), + Process.textEffect( + [shellCmd], + { shell: sh, nothrow: true }, + Effect.map((result) => result.text), + Effect.orDie, + ),As per coding guidelines, “Use
Effect.fn("Domain.method")... these accept pipeable operators as extra arguments to avoid unnecessary outer.pipe()wrappers”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/session/prompt.ts` around lines 1498 - 1505, The code in the shellMatches.map callback is chaining Effect operators using `.pipe()` after calling Process.textEffect. Refactor this to pass the operators directly as arguments to Process.textEffect instead of piping them. Remove the `.pipe(Effect.map((result) => result.text), Effect.orDie,)` chain and instead provide these operators as additional arguments to the Process.textEffect call to follow the coding guideline of using Effect.fn operator-argument form to avoid unnecessary outer pipe wrappers.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/src/pty/index.ts`:
- Around line 172-176: The code is using an outer `.pipe(Effect.orDie)` wrapper
after the `Process.terminateTreeEffect()` call, but according to the repo's
Effect composition convention, this operator should be passed directly as an
argument to `Process.terminateTreeEffect()` instead. Refactor the call to
`Process.terminateTreeEffect()` to accept `Effect.orDie` as a direct argument
within the function call, eliminating the need for the outer `.pipe()` wrapper
while maintaining the same functionality.
In `@packages/opencode/src/session/prompt.ts`:
- Around line 1498-1505: The code in the shellMatches.map callback is chaining
Effect operators using `.pipe()` after calling Process.textEffect. Refactor this
to pass the operators directly as arguments to Process.textEffect instead of
piping them. Remove the `.pipe(Effect.map((result) => result.text),
Effect.orDie,)` chain and instead provide these operators as additional
arguments to the Process.textEffect call to follow the coding guideline of using
Effect.fn operator-argument form to avoid unnecessary outer pipe wrappers.
In `@packages/opencode/src/tool/shell.ts`:
- Around line 610-618: Both the abort branch (around line 610) and timeout
branch (around line 615) currently use Process.terminateTreeEffect() with a
chained .pipe(Effect.orDie) call. Refactor both instances to pass Effect.orDie
directly as an argument to the Process.terminateTreeEffect() function call
instead of chaining it with .pipe(), removing the outer .pipe() wrapper
entirely. This applies the consistent Effect style used throughout the
repository where pipeable operators are passed as extra arguments to effect
functions.
In `@packages/opencode/src/util/process.ts`:
- Around line 356-357: The runtime is being created using
ManagedRuntime.make(defaultLayer) directly instead of the recommended
makeRuntime function from src/effect/run-service.ts, which provides a shared
memoMap for layer deduplication. Unless this Process module is a foundational
module that needs to avoid circular dependencies, replace the
ManagedRuntime.make(defaultLayer) call with makeRuntime() from
src/effect/run-service.ts, ensuring the runtime variable and runPromise function
continue to work as expected.
In `@packages/opencode/test/util/process.test.ts`:
- Around line 22-27: The test for Process.runEffect should use the Effect test
harness instead of raw test() with Effect.runPromise(). Import testEffect from
test/lib/effect.ts and create an it constant at the top of the file using
testEffect(Layer.allSucceed(...)). Replace the test() function declaration with
it.live("captures stdout and stderr through the Effect path", ...) and convert
the test body to use Effect.gen() syntax instead of await Effect.runPromise(),
yielding the Process.runEffect call and asserting on the result within the
generator function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e473b1ca-c27a-4b1a-8d5f-eb470b342956
📒 Files selected for processing (6)
packages/opencode/specs/effect-migration.mdpackages/opencode/src/pty/index.tspackages/opencode/src/session/prompt.tspackages/opencode/src/tool/shell.tspackages/opencode/src/util/process.tspackages/opencode/test/util/process.test.ts
2 participants
Summary
Move
packages/opencode/src/util/process.tsto an Effect-native implementation while keeping the legacy async compatibility facade.Closed process boundaries:
Process.runEffect,textEffect,linesEffect,stopEffect,descendantsEffect, andterminateTreeEffectnow own execution and cleanup.run/text/lines/stop/descendants/terminateTreefacade now delegates through the Process service runtime.session/prompt.ts,pty/index.ts, andtool/shell.tsnow yield the Effect-native process APIs directly.Retained process boundaries:
Process.spawnstill returns the Node child facade withexited: Promise<number>because CLI pager/auth flows, long-lived LSP launch, Windows cmd script spawning, stream ownership, and process cleanup still depend on that shape.Why
Related to #936.
The remaining process utility primitive was still a Promise-first wrapper. This PR moves the reusable process execution and cleanup paths behind Effect APIs without forcing every compatibility caller through the Effect graph in one review.
Related Issue
Related to #936
Human Review Status
Pending
Review Focus
Please focus on whether the Effect API owns the real implementation, whether the retained
spawnfacade is still the right compatibility boundary, and whether abort/timeout/process-tree cleanup behavior is preserved.Risk Notes
Process and shell cleanup are platform-sensitive. The PR preserves the existing
cross-spawnchild facade, Windows cmd script behavior, stdout/stderr buffering,nothrow, missing-command handling, abort cleanup, timeout cleanup, and process-tree termination tests.Skipped checklist items:
How To Verify
Screenshots or Recordings
Not applicable; no visible UI changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
Refactor
Tests