Add DenoChildProcessSpawner - #6688
Merged
tim-smart merged 4 commits intoJul 28, 2026
Merged
Conversation
🦋 Changeset detectedLatest commit: 20ba4df The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA native Deno ChangesChild process spawning
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DenoTest as DenoChildProcessSpawner.test
participant Suite as ChildProcessSpawnerTest.suite
participant Spawner as DenoChildProcessSpawner
participant Deno as Deno.Command
participant Streams as Process streams
DenoTest->>Suite: configure shared suite with Deno layer
Suite->>Spawner: spawn command or pipeline
Spawner->>Deno: execute command with stdio options
Deno-->>Streams: provide stdin, stdout, and stderr
Streams-->>Suite: return output and exit status
Suite->>Suite: assert process behavior
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
Contributor
Bundle Size Analysis
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
packages/effect/test/unstable/process/ChildProcessSpawnerTest.ts (3)
219-221: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAwait
exitCodeafter draining stdout, not before.Every other test reads stdout first. Here
exitCodeis awaited while stdout is still buffered; it only passes becauselsoutput is smaller than the pipe buffer. Swap the two lines to match the rest of the suite and stay deadlock-free if a fixture dir grows.♻️ Proposed reorder
const handle = yield* ChildProcess.make`ls ${args} ${dir}` - const exitCode = yield* handle.exitCode const output = yield* decodeByteStream(handle.stdout) + const exitCode = yield* handle.exitCode🤖 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/effect/test/unstable/process/ChildProcessSpawnerTest.ts` around lines 219 - 221, In the ChildProcess test flow, reorder the awaits so decodeByteStream(handle.stdout) drains stdout before awaiting handle.exitCode. Keep the existing ChildProcess.make invocation and assertions unchanged.
262-306: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSerial draining of multiple child pipes. Both sites read one stream to completion before starting the next, which only works while every stream's payload fits in the OS pipe buffer — the exact hazard the comments on lines 311 and 326 call out and solve with concurrent reads.
packages/effect/test/unstable/process/ChildProcessSpawnerTest.ts#L262-L306: wrap thestdout/stderrreads inEffect.all([...], { concurrency: "unbounded" }).packages/effect/test/unstable/process/ChildProcessSpawnerTest.ts#L865-L883: wrap thestdout,stderr, andgetOutputFd(3)reads in a singleEffect.all([...], { concurrency: "unbounded" }).🤖 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/effect/test/unstable/process/ChildProcessSpawnerTest.ts` around lines 262 - 306, The stdout and stderr pipes are drained serially, risking blockage when one exceeds the OS pipe buffer. In ChildProcessSpawnerTest.ts lines 262-306, update both affected tests to read stdout and stderr through a single Effect.all with unbounded concurrency; in ChildProcessSpawnerTest.ts lines 865-883, similarly combine stdout, stderr, and getOutputFd(3) reads into one concurrent Effect.all.
887-902: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
killMatchingProcessesruns an unescapedpkill -f— worth hardening.
patternis interpolated straight into a single-quoted shell string, andpattern[0]isundefinedfor an empty string (yielding[undefined]). Callers currently pass safe literals, so this is not exploitable, but a stray pattern here issues a broadpkill -fagainst the whole machine. Consider asserting the pattern is a non-empty[A-Za-z0-9_-]+before building the command.🤖 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/effect/test/unstable/process/ChildProcessSpawnerTest.ts` around lines 887 - 902, Validate pattern in killMatchingProcesses before constructing the pkill command, requiring a non-empty value containing only ASCII letters, digits, underscores, or hyphens; reject invalid input rather than interpolating it, and retain the existing escaped-pattern behavior for valid values.packages/effect/test/unstable/process/fixtures/bash/parent-exits-early.sh (1)
7-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGive the descendant sleeps a unique marker.
These subshells run a bare
sleep 30, which is what forces the test to countsleep 30globally viaps aux. Passing a distinctive$0makes the process identifiable and the assertions precise.🔧 Proposed fix
( grandchild_pid=$BASHPID echo "Grandchild of child $i started with PID $grandchild_pid" - # Keep running for 30 seconds - sleep 30 + # Keep running for 30 seconds, tagged so tests can identify it + sh -c 'sleep 30' parent-exits-early-descendant ) & - # Keep the child running - sleep 30 + # Keep the child running, tagged so tests can identify it + sh -c 'sleep 30' parent-exits-early-descendant ) &🤖 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/effect/test/unstable/process/fixtures/bash/parent-exits-early.sh` around lines 7 - 24, Update the descendant sleep commands in the parent-exits-early fixture to pass a distinctive process marker via $0, using different identifiable markers for the child and grandchild sleeps. Preserve the 30-second duration while enabling process assertions to target only these descendants instead of counting all global “sleep 30” processes.
🤖 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.
Inline comments:
In `@packages/effect/test/unstable/process/ChildProcessSpawnerTest.ts`:
- Around line 712-730: Update the assertion in the “should throw permission
denied as a typed error” test to validate only the runtime-independent
PlatformError fields: _tag, module, and method. Remove the exact syscall and
pathOrDescriptor expectations so the shared test remains compatible across
adapters, leaving runtime-specific payload shape checks to adapter-specific
tests.
- Around line 340-401: Make the .all interleaving tests deterministic by
removing exact cross-stream ordering assumptions from “should read interspersed
stdout and stderr via .all” and “should handle many lines of interspersed output
via .all”. Assert that all expected stdout and stderr lines are present while
preserving each stream’s internal order, or use a sufficiently reliable
synchronization strategy instead of 10 ms sleeps.
- Around line 988-999: Replace the global `ps aux` search for the generic `sleep
30` pattern in the affected cleanup assertions with a unique marker identifying
descendants launched by `parent-exits-early.sh`. Update that script’s `sleep`
subshell invocations to receive the marker via `$0`, following the existing
pattern near the pipeline unref test, and have both assertions count only that
marker while preserving their expected zero-process checks.
- Around line 1003-1021: Ensure the process created in the “should not kill an
unrefed process when scope closes” test is always killed by attaching cleanup
through Effect.ensuring or Effect.addFinalizer, rather than leaving handle.kill
as the final statement after assert.isTrue. Hoist handle or use a scoped
finalizer so cleanup runs when assertions fail, and apply the same
finalizer-based cleanup to the pipeline test around its cleanup logic.
- Around line 142-153: Update the shell-expansion test around ChildProcess.make
to use a test-controlled environment variable with a known non-empty value
instead of HOME, passing that variable into the spawned shell command and
asserting the expanded output excludes the literal variable expression.
- Around line 19-33: Update decodeByteStream to handle ChildProcess.Encoding
values other than text encodings before constructing TextDecoder: either
restrict its accepted encoding type to supported text encodings or add explicit
decoding branches for base64, base64url, binary, and hex. Ensure unsupported
values no longer reach new TextDecoder(encoding) and preserve the existing
text-decoding behavior.
In `@packages/effect/test/unstable/process/fixtures/bash/spawn-children.sh`:
- Around line 18-26: Update both unused-count loops in spawn-children.sh to
discard the loop variable by using _ instead of j, preserving the existing
60-iteration sleep behavior and eliminating SC2034.
---
Nitpick comments:
In `@packages/effect/test/unstable/process/ChildProcessSpawnerTest.ts`:
- Around line 219-221: In the ChildProcess test flow, reorder the awaits so
decodeByteStream(handle.stdout) drains stdout before awaiting handle.exitCode.
Keep the existing ChildProcess.make invocation and assertions unchanged.
- Around line 262-306: The stdout and stderr pipes are drained serially, risking
blockage when one exceeds the OS pipe buffer. In ChildProcessSpawnerTest.ts
lines 262-306, update both affected tests to read stdout and stderr through a
single Effect.all with unbounded concurrency; in ChildProcessSpawnerTest.ts
lines 865-883, similarly combine stdout, stderr, and getOutputFd(3) reads into
one concurrent Effect.all.
- Around line 887-902: Validate pattern in killMatchingProcesses before
constructing the pkill command, requiring a non-empty value containing only
ASCII letters, digits, underscores, or hyphens; reject invalid input rather than
interpolating it, and retain the existing escaped-pattern behavior for valid
values.
In `@packages/effect/test/unstable/process/fixtures/bash/parent-exits-early.sh`:
- Around line 7-24: Update the descendant sleep commands in the
parent-exits-early fixture to pass a distinctive process marker via $0, using
different identifiable markers for the child and grandchild sleeps. Preserve the
30-second duration while enabling process assertions to target only these
descendants instead of counting all global “sleep 30” processes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 88ca1ff7-98e0-401f-ba70-9f66f6badc32
📒 Files selected for processing (9)
packages/effect/test/unstable/process/ChildProcessSpawnerTest.tspackages/effect/test/unstable/process/fixtures/bash/no-permissions.shpackages/effect/test/unstable/process/fixtures/bash/parent-exits-early.shpackages/effect/test/unstable/process/fixtures/bash/spawn-children.shpackages/effect/test/unstable/process/fixtures/config/SHOUTINGpackages/effect/test/unstable/process/fixtures/config/integerpackages/effect/test/unstable/process/fixtures/config/nested/configpackages/effect/test/unstable/process/fixtures/config/secretpackages/platform-node-shared/test/NodeChildProcessSpawner.test.ts
github-project-automation
Bot
moved this from Discussion Ongoing
to Waiting on Author
in PR Backlog
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🧹 Nitpick comments (4)
packages/effect/test/unstable/process/ChildProcessSpawnerTest.ts (3)
219-221: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAwait
exitCodeafter draining stdout, not before.Every other test reads stdout first. Here
exitCodeis awaited while stdout is still buffered; it only passes becauselsoutput is smaller than the pipe buffer. Swap the two lines to match the rest of the suite and stay deadlock-free if a fixture dir grows.♻️ Proposed reorder
const handle = yield* ChildProcess.make`ls ${args} ${dir}` - const exitCode = yield* handle.exitCode const output = yield* decodeByteStream(handle.stdout) + const exitCode = yield* handle.exitCode🤖 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/effect/test/unstable/process/ChildProcessSpawnerTest.ts` around lines 219 - 221, In the ChildProcess test flow, reorder the awaits so decodeByteStream(handle.stdout) drains stdout before awaiting handle.exitCode. Keep the existing ChildProcess.make invocation and assertions unchanged.
262-306: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSerial draining of multiple child pipes. Both sites read one stream to completion before starting the next, which only works while every stream's payload fits in the OS pipe buffer — the exact hazard the comments on lines 311 and 326 call out and solve with concurrent reads.
packages/effect/test/unstable/process/ChildProcessSpawnerTest.ts#L262-L306: wrap thestdout/stderrreads inEffect.all([...], { concurrency: "unbounded" }).packages/effect/test/unstable/process/ChildProcessSpawnerTest.ts#L865-L883: wrap thestdout,stderr, andgetOutputFd(3)reads in a singleEffect.all([...], { concurrency: "unbounded" }).🤖 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/effect/test/unstable/process/ChildProcessSpawnerTest.ts` around lines 262 - 306, The stdout and stderr pipes are drained serially, risking blockage when one exceeds the OS pipe buffer. In ChildProcessSpawnerTest.ts lines 262-306, update both affected tests to read stdout and stderr through a single Effect.all with unbounded concurrency; in ChildProcessSpawnerTest.ts lines 865-883, similarly combine stdout, stderr, and getOutputFd(3) reads into one concurrent Effect.all.
887-902: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
killMatchingProcessesruns an unescapedpkill -f— worth hardening.
patternis interpolated straight into a single-quoted shell string, andpattern[0]isundefinedfor an empty string (yielding[undefined]). Callers currently pass safe literals, so this is not exploitable, but a stray pattern here issues a broadpkill -fagainst the whole machine. Consider asserting the pattern is a non-empty[A-Za-z0-9_-]+before building the command.🤖 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/effect/test/unstable/process/ChildProcessSpawnerTest.ts` around lines 887 - 902, Validate pattern in killMatchingProcesses before constructing the pkill command, requiring a non-empty value containing only ASCII letters, digits, underscores, or hyphens; reject invalid input rather than interpolating it, and retain the existing escaped-pattern behavior for valid values.packages/effect/test/unstable/process/fixtures/bash/parent-exits-early.sh (1)
7-24: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGive the descendant sleeps a unique marker.
These subshells run a bare
sleep 30, which is what forces the test to countsleep 30globally viaps aux. Passing a distinctive$0makes the process identifiable and the assertions precise.🔧 Proposed fix
( grandchild_pid=$BASHPID echo "Grandchild of child $i started with PID $grandchild_pid" - # Keep running for 30 seconds - sleep 30 + # Keep running for 30 seconds, tagged so tests can identify it + sh -c 'sleep 30' parent-exits-early-descendant ) & - # Keep the child running - sleep 30 + # Keep the child running, tagged so tests can identify it + sh -c 'sleep 30' parent-exits-early-descendant ) &🤖 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/effect/test/unstable/process/fixtures/bash/parent-exits-early.sh` around lines 7 - 24, Update the descendant sleep commands in the parent-exits-early fixture to pass a distinctive process marker via $0, using different identifiable markers for the child and grandchild sleeps. Preserve the 30-second duration while enabling process assertions to target only these descendants instead of counting all global “sleep 30” processes.
🤖 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.
Inline comments:
In `@packages/effect/test/unstable/process/ChildProcessSpawnerTest.ts`:
- Around line 712-730: Update the assertion in the “should throw permission
denied as a typed error” test to validate only the runtime-independent
PlatformError fields: _tag, module, and method. Remove the exact syscall and
pathOrDescriptor expectations so the shared test remains compatible across
adapters, leaving runtime-specific payload shape checks to adapter-specific
tests.
- Around line 340-401: Make the .all interleaving tests deterministic by
removing exact cross-stream ordering assumptions from “should read interspersed
stdout and stderr via .all” and “should handle many lines of interspersed output
via .all”. Assert that all expected stdout and stderr lines are present while
preserving each stream’s internal order, or use a sufficiently reliable
synchronization strategy instead of 10 ms sleeps.
- Around line 988-999: Replace the global `ps aux` search for the generic `sleep
30` pattern in the affected cleanup assertions with a unique marker identifying
descendants launched by `parent-exits-early.sh`. Update that script’s `sleep`
subshell invocations to receive the marker via `$0`, following the existing
pattern near the pipeline unref test, and have both assertions count only that
marker while preserving their expected zero-process checks.
- Around line 1003-1021: Ensure the process created in the “should not kill an
unrefed process when scope closes” test is always killed by attaching cleanup
through Effect.ensuring or Effect.addFinalizer, rather than leaving handle.kill
as the final statement after assert.isTrue. Hoist handle or use a scoped
finalizer so cleanup runs when assertions fail, and apply the same
finalizer-based cleanup to the pipeline test around its cleanup logic.
- Around line 142-153: Update the shell-expansion test around ChildProcess.make
to use a test-controlled environment variable with a known non-empty value
instead of HOME, passing that variable into the spawned shell command and
asserting the expanded output excludes the literal variable expression.
- Around line 19-33: Update decodeByteStream to handle ChildProcess.Encoding
values other than text encodings before constructing TextDecoder: either
restrict its accepted encoding type to supported text encodings or add explicit
decoding branches for base64, base64url, binary, and hex. Ensure unsupported
values no longer reach new TextDecoder(encoding) and preserve the existing
text-decoding behavior.
In `@packages/effect/test/unstable/process/fixtures/bash/spawn-children.sh`:
- Around line 18-26: Update both unused-count loops in spawn-children.sh to
discard the loop variable by using _ instead of j, preserving the existing
60-iteration sleep behavior and eliminating SC2034.
---
Nitpick comments:
In `@packages/effect/test/unstable/process/ChildProcessSpawnerTest.ts`:
- Around line 219-221: In the ChildProcess test flow, reorder the awaits so
decodeByteStream(handle.stdout) drains stdout before awaiting handle.exitCode.
Keep the existing ChildProcess.make invocation and assertions unchanged.
- Around line 262-306: The stdout and stderr pipes are drained serially, risking
blockage when one exceeds the OS pipe buffer. In ChildProcessSpawnerTest.ts
lines 262-306, update both affected tests to read stdout and stderr through a
single Effect.all with unbounded concurrency; in ChildProcessSpawnerTest.ts
lines 865-883, similarly combine stdout, stderr, and getOutputFd(3) reads into
one concurrent Effect.all.
- Around line 887-902: Validate pattern in killMatchingProcesses before
constructing the pkill command, requiring a non-empty value containing only
ASCII letters, digits, underscores, or hyphens; reject invalid input rather than
interpolating it, and retain the existing escaped-pattern behavior for valid
values.
In `@packages/effect/test/unstable/process/fixtures/bash/parent-exits-early.sh`:
- Around line 7-24: Update the descendant sleep commands in the
parent-exits-early fixture to pass a distinctive process marker via $0, using
different identifiable markers for the child and grandchild sleeps. Preserve the
30-second duration while enabling process assertions to target only these
descendants instead of counting all global “sleep 30” processes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 88ca1ff7-98e0-401f-ba70-9f66f6badc32
📒 Files selected for processing (9)
packages/effect/test/unstable/process/ChildProcessSpawnerTest.tspackages/effect/test/unstable/process/fixtures/bash/no-permissions.shpackages/effect/test/unstable/process/fixtures/bash/parent-exits-early.shpackages/effect/test/unstable/process/fixtures/bash/spawn-children.shpackages/effect/test/unstable/process/fixtures/config/SHOUTINGpackages/effect/test/unstable/process/fixtures/config/integerpackages/effect/test/unstable/process/fixtures/config/nested/configpackages/effect/test/unstable/process/fixtures/config/secretpackages/platform-node-shared/test/NodeChildProcessSpawner.test.ts
🛑 Comments failed to post (1)
packages/effect/test/unstable/process/fixtures/bash/spawn-children.sh (1)
18-26: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Silence SC2034 by discarding the unused loop variable.
jis never read in either loop. Use_(or theseq-freefor ((j=0; j<60; j++))form) so ShellCheck stops warning.🔧 Proposed fix
# Keep running for 60 seconds - for j in {1..60}; do + for _ in {1..60}; do sleep 1 done ) & # Keep the child running - for j in {1..60}; do + for _ in {1..60}; do sleep 1 done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for _ in {1..60}; do sleep 1 done ) & # Keep the child running for _ in {1..60}; do sleep 1 done🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 24-24: j appears unused. Verify use (or export if used externally).
(SC2034)
🤖 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/effect/test/unstable/process/fixtures/bash/spawn-children.sh` around lines 18 - 26, Update both unused-count loops in spawn-children.sh to discard the loop variable by using _ instead of j, preserving the existing 60-iteration sleep behavior and eliminating SC2034.Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/platform-deno/src/DenoChildProcessSpawner.ts (4)
351-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
Stream.unwrap(Effect.succeed(...)).
getSourceStreamalready returns aStream; the wrapper is a no-op.♻️ Proposed simplification
- const sourceStream = Stream.unwrap( - Effect.succeed(getSourceStream(handles[handles.length - 1], options.from)) - ) + const sourceStream = getSourceStream(handles[handles.length - 1], options.from)🤖 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/platform-deno/src/DenoChildProcessSpawner.ts` around lines 351 - 353, In the source stream setup, replace the redundant Stream.unwrap(Effect.succeed(...)) wrapper with the Stream returned directly by getSourceStream, preserving the same handles and options.from arguments.
48-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider always attaching
cause.For
NotFound/PermissionDenied/TimedOutthe originatingDeno.errors.*instance (and its stack) is discarded, leaving only the derived tag. Keepingcausein all branches costs nothing and preserves diagnostics.♻️ Proposed tweak
return PlatformError.systemError({ _tag: tag, module: "ChildProcess", method, pathOrDescriptor: commandString(command), syscall: `${method} ${commandString(command).trim()}`, - ...(tag === "Unknown" ? { cause } : undefined) + cause })🤖 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/platform-deno/src/DenoChildProcessSpawner.ts` around lines 48 - 55, Update the PlatformError.systemError construction in DenoChildProcessSpawner so the original cause is always attached, regardless of the error tag. Remove the conditional restriction tied to the "Unknown" tag while preserving the existing _tag, module, method, pathOrDescriptor, and syscall fields.
247-249: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
detached: falseis rejected even though it is satisfiable.Deno's behavior is non-detached, so
detached: falsecan be honored as a no-op. Rejecting on mere presence forces portable callers to strip an explicitly-default-valued option. Rejecting onlydetached: truewould widen cross-platform compatibility (the corresponding assertion inpackages/platform-deno/test/DenoChildProcessSpawner.test.tswould need updating to usedetached: true).♻️ Proposed change
- if (Predicate.isNotUndefined(cmd.options.detached)) { + if (cmd.options.detached === true) { return yield* Effect.fail(unsupported("detached")) }🤖 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/platform-deno/src/DenoChildProcessSpawner.ts` around lines 247 - 249, Update the detached-option handling in DenoChildProcessSpawner so it rejects only when cmd.options.detached is true, while treating detached: false as a no-op. Update the corresponding test assertion in DenoChildProcessSpawner.test.ts to verify rejection for detached: true.
407-439: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract
flattenCommandintopackages/effect/src/unstable/process/ChildProcess.ts. It only depends onChildProcess.Command, andpackages/platform-node-shared/src/NodeChildProcessSpawner.tsalready carries the same helper, so centralizing it would remove duplication and keep both spawners on the shared API.🤖 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/platform-deno/src/DenoChildProcessSpawner.ts` around lines 407 - 439, Move the FlattenedPipeline interface and flattenCommand helper from DenoChildProcessSpawner into the shared ChildProcess module in packages/effect/src/unstable/process/ChildProcess.ts. Export them there, then update both DenoChildProcessSpawner and NodeChildProcessSpawner to import and reuse the shared symbols, removing their local duplicate implementations.
🤖 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.
Inline comments:
In `@packages/platform-deno/README.md`:
- Line 13: Update the README statement about killing a ChildProcess to say that
descendant processes remain running, rather than saying they are not reaped;
preserve the existing explanation about Deno lacking portable process-group
isolation.
In `@packages/platform-deno/src/DenoChildProcessSpawner.ts`:
- Around line 366-385: Update the pipeline handle construction around makeHandle
so kill terminates every handle in handles, invoking them in reverse order so
the tail is stopped first. Keep the existing tail-handle forwarding for pid,
exitCode, isRunning, and stream fields, and preserve the current unref behavior.
- Around line 232-235: Update the PipedCommand handling around the default
`fromOption` branch to reject additional file-descriptor sources with the same
`unsupported("additionalFds")` behavior used by the existing `to` validation. Do
not resolve valid fd names through `handle.getOutputFd`, and ensure unparsable
fd-style `from` values no longer silently fall back to stdout.
In `@packages/platform-deno/test/DenoChildProcessSpawner.test.ts`:
- Around line 33-38: Update makeTempDirectoryScoped so the incoming
options.directory value is translated to Deno.makeTempDir’s dir option before
invoking it, while preserving the remaining options and existing platformError
handling.
---
Nitpick comments:
In `@packages/platform-deno/src/DenoChildProcessSpawner.ts`:
- Around line 351-353: In the source stream setup, replace the redundant
Stream.unwrap(Effect.succeed(...)) wrapper with the Stream returned directly by
getSourceStream, preserving the same handles and options.from arguments.
- Around line 48-55: Update the PlatformError.systemError construction in
DenoChildProcessSpawner so the original cause is always attached, regardless of
the error tag. Remove the conditional restriction tied to the "Unknown" tag
while preserving the existing _tag, module, method, pathOrDescriptor, and
syscall fields.
- Around line 247-249: Update the detached-option handling in
DenoChildProcessSpawner so it rejects only when cmd.options.detached is true,
while treating detached: false as a no-op. Update the corresponding test
assertion in DenoChildProcessSpawner.test.ts to verify rejection for detached:
true.
- Around line 407-439: Move the FlattenedPipeline interface and flattenCommand
helper from DenoChildProcessSpawner into the shared ChildProcess module in
packages/effect/src/unstable/process/ChildProcess.ts. Export them there, then
update both DenoChildProcessSpawner and NodeChildProcessSpawner to import and
reuse the shared symbols, removing their local duplicate implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 515e8047-8fa2-4741-9ecb-d5620dae8986
📒 Files selected for processing (6)
.changeset/eff-141-deno-child-process.mdpackages/effect/test/unstable/process/ChildProcessSpawnerTest.tspackages/platform-deno/README.mdpackages/platform-deno/src/DenoChildProcessSpawner.tspackages/platform-deno/src/index.tspackages/platform-deno/test/DenoChildProcessSpawner.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/effect/test/unstable/process/ChildProcessSpawnerTest.ts
tim-smart
commented
Jul 28, 2026
|
|
||
| const resolveStdinOption = (options: ChildProcess.CommandOptions): ChildProcess.StdinConfig => { | ||
| const defaultConfig: ChildProcess.StdinConfig = { stream: "pipe", encoding: "utf-8", endOnDone: true } | ||
| if (Predicate.isUndefined(options.stdin)) { |
Contributor
Author
There was a problem hiding this comment.
Instead of Predicate.isUndefined just use normal undefined checks
Contributor
Author
There was a problem hiding this comment.
Replaced the Predicate.isUndefined checks with direct undefined checks throughout the adapter.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/platform-deno/test/DenoChildProcessSpawner.test.ts`:
- Around line 110-130: Update the “kills every process in a pipeline” test
around ChildProcess.pipeTo so the downstream sleep command independently updates
a second heartbeat file. Capture both heartbeat sizes after handle.kill({
killSignal: "SIGKILL" }), wait with TestClock.withLive, then assert each final
size remains unchanged, proving both the root and pipeline child processes
stopped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e9bd472-3c95-472e-a1f5-d1911d77bab8
📒 Files selected for processing (6)
packages/effect/test/unstable/process/ChildProcessSpawnerTest.tspackages/effect/test/unstable/process/fixtures/bash/parent-exits-early.shpackages/effect/test/unstable/process/fixtures/bash/spawn-children.shpackages/platform-deno/README.mdpackages/platform-deno/src/DenoChildProcessSpawner.tspackages/platform-deno/test/DenoChildProcessSpawner.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/platform-deno/README.md
- packages/effect/test/unstable/process/fixtures/bash/parent-exits-early.sh
- packages/platform-deno/src/DenoChildProcessSpawner.ts
- packages/effect/test/unstable/process/ChildProcessSpawnerTest.ts
tim-smart
changed the title
tim-smart
enabled auto-merge (squash)
July 28, 2026 09:33
tim-smart
deleted the
agent/codex-engineer/eff-141-deno-child-process-spawner
branch
July 28, 2026 09:34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
deno task --filter @effect/platform-deno test --runpnpm --filter @effect/platform-node-shared test --run test/NodeChildProcessSpawner.test.tsdeno check .pnpm checkpnpm lintpnpm exec docgeninpackages/platform-denoCloses EFF-141
Closes EFF-163
Closes EFF-164
Summary by CodeRabbit
ChildProcessSpawnerimplementation with shared cross-platform process conformance.ChildProcesslimitations and divergences.