feat(process): add processApi with middleware support#159
Open
feat(process): add processApi with middleware support#159
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a middleware-capable public API Changes
Sequence DiagramsequenceDiagram
participant Client
participant Exec
participant DoExec
participant API
participant Middleware
participant OS
Client->>Exec: exec(cmd, opts)
Exec->>DoExec: route via doExec
DoExec->>API: operations.exec(cmd, opts)
API->>Middleware: around(exec) invoked
Middleware->>Middleware: inspect/modify args
Middleware->>API: call next(...)
API->>OS: create/run process
OS-->>API: Process
API-->>DoExec: Process
DoExec-->>Exec: Process
Exec-->>Client: Process
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
commit: |
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@process/package.json`:
- Around line 14-16: Update the package.json version field to a minor bump to
reflect the new public API surface (e.g., change "version": "0.7.2" to "0.8.0");
ensure the package.json "version" value is updated consistently and include an
entry in the release notes/changelog if present so the new middleware-capable
API is recorded before publishing.
In `@process/README.md`:
- Around line 230-233: The heading "#### Capturing process executions for
testing" is jumping levels; change this heading to one level higher (use "###
Capturing process executions for testing") so the Markdown increments by one
level; update the heading text in the README.md where "Capturing process
executions for testing" appears to fix MD001 and keep surrounding headings
consistent with the rest of the document.
In `@process/test/exec.test.ts`:
- Around line 526-569: The tests use the shell builtin "echo" which fails on
Windows; update the processApi.around tests to invoke a real executable instead
of a shell builtin by calling exec with a binary and args (e.g., use "node" with
"-e" and a small console.log payload) so the created command tokens and
middleware behavior are preserved; modify both uses of exec("echo ...") in the
tests around the createProcess middleware (refer to processApi.around,
createProcess, exec, and spawn) to call exec with the executable and argument
array form to ensure cross-platform compatibility.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@process/package.json`:
- Around line 14-15: The peerDependencies entry currently pins effection to a
pre-release ("^4.1.0-alpha.3") which is overly restrictive; update the
package.json peerDependencies key for "effection" to the project-standard range
"^3 || ^4" so consumers of both Effection 3.x and 4.x stable releases are
supported; modify the "peerDependencies" block (the "effection" entry)
accordingly and run a quick install/test to verify no semver conflicts.
In `@process/README.md`:
- Around line 6-9: Update the version note text that currently reads "Note:
Starting with version 0.2.0, this package requires Effection v4.1 or greater..."
to reflect the actual release that introduced the Effection v4.1 requirement:
locate the markdown paragraph containing that exact sentence (the "Note:
Starting with version 0.2.0..." line) in README and change "0.2.0" to the
correct version number for this release so the note accurately states which
version requires Effection v4.1.
- Around line 236-249: The example's logged output is incorrect because the
middleware in processApi.around's createProcess receives parsed command and args
(args[0] is the command name like "git" or "npm"), so change the example to
either join the command and its arguments before pushing (e.g. push `${args[0]}
${args.slice(1).join(' ')}`) or update the expected console output to reflect
only the command names (e.g. ["git","npm"]); update the example using the
createProcess middleware and exec invocation to show consistent input and
output.
taras
commented
Feb 16, 2026
process/src/exec.ts
Outdated
| * Create a process operation. | ||
| * This is the core operation that middleware can intercept. | ||
| */ | ||
| createProcess(cmd: string, opts: ExecOptions): Operation<Process>; |
Member
Author
There was a problem hiding this comment.
I think the APi should be exec and daemon, not createProcess
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
process/src/exec.ts (1)
136-202: 🧹 Nitpick | 🔵 TrivialConsider potential double-execution edge case.
Each method (
[Symbol.iterator],join,expect) independently callsdoExec(), which creates a new process. While the typical usage patterns (exec().join()orlet p = yield* exec()) are safe, the following would unexpectedly spawn two processes:let e = exec("cmd"); yield* e; // process 1 yield* e.join(); // process 2 (Exec.join, not Process.join)This appears to be the existing design, not a regression. Consider adding a JSDoc note to clarify the intended usage patterns.
🤖 Fix all issues with AI agents
In `@process/mod.ts`:
- Around line 1-3: Remove the redundant explicit re-export of processApi: the
existing export * from "./src/exec.ts" already re-exports processApi, so delete
the line exporting processApi and, if you want to keep discoverability, add a
short doc comment above the barrel export (export * from "./src/exec.ts")
mentioning that processApi is available from this module; reference symbols:
processApi and the export * from "./src/exec.ts".
In `@process/src/exec.ts`:
- Line 6: The import of createApi in process/src/exec.ts is invalid for the
declared Effection version and will break at runtime; remove createApi from the
effection import list and refactor any code in exec.ts that calls createApi to
use a supported Effection primitive (e.g., replace with an equivalent using
available APIs such as createResource/spawn/Operation or a small local wrapper),
or stub the functionality with a clear TODO comment; once
thefrontside/effection#1116 lands and you bump the effection dependency, restore
createApi and remove the temporary workaround, and add/adjust tests to cover the
altered code path to prevent regressions.
taras
force-pushed
the
feat/process-api
branch
from
6b9bdcb to
b84bb58
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@process/README.md`:
- Line 231: Change the heading "Intercepting daemon calls" (and the similar
heading at line 251) to use level ### instead of #### so the heading level does
not jump after the `processApi` section; locate the occurrences of the text
"Intercepting daemon calls" in README.md and replace the leading four hashes
with three to restore proper heading hierarchy under `processApi`.
In `@process/src/exec.ts`:
- Around line 3-9: Change the import for the type-only symbol Api to a
TypeScript type-only import to improve tree-shaking and clarity: update the
import statement that currently brings in Api, Operation, createApi, resource,
spawn so that Api is imported with the type modifier (keep Operation as-is if
it’s used at runtime). Locate the import at the top of the file (references:
Api, Operation, createApi, resource, spawn) and modify only the Api import to be
type-only.
- Around line 137-146: doExec() is being invoked multiple times (in the
iterator, join(), and expect()), which spawns duplicate processes; change the
implementation to lazily create and cache the single process/operation result
returned by processApi.operations.exec(command, options) (e.g., store it in a
local variable like cachedOp or createdProcess) and have *[Symbol.iterator](),
*join(), and *expect() all yield from that cached value so the process is
created once and reused; if maintaining mutable cached state across yields is
concerning, use Effection's resource/useScope pattern to manage the single
process lifecycle instead.
---
Duplicate comments:
In `@process/mod.ts`:
- Around line 2-3: The explicit re-export of processApi is redundant because the
existing barrel export (export *) already exposes processApi; remove the
explicit line "export { processApi } from \"./src/exec.ts\";" or instead convert
the preceding comment into a JSDoc on the barrel export to preserve
discoverability, leaving only the single barrel export that exposes processApi
(symbol: processApi) and avoiding duplicate exports.
In `@process/package.json`:
- Around line 15-16: The peerDependencies entry currently pins effection to the
pre-release "^4.1.0-alpha.3" for the createApi feature; update the project docs
to clearly state this intentional constraint (add a note in the package README
or CHANGELOG referencing the peerDependencies key and the createApi feature and
why effection 4.1-alpha is required), and add a TODO comment in package.json
maintenance notes to change the version to "^4.1" (or broaden back to "^3 || ^4"
if compatible) once Effection 4.1.0 is stable; keep the existing "effection":
"^4.1.0-alpha.3" entry until that follow-up is done.
In `@process/src/exec.ts`:
- Line 6: The import of createApi in process/src/exec.ts depends on an unmerged
Effection PR (`#1116`); remove the direct import of createApi (and any usage of it
in this module) and replace it with a safe fallback that either (a) uses an
alternative local implementation or helper, or (b) throws a clear runtime error
explaining the upstream dependency until `#1116` is merged; update any functions
that call createApi to use the fallback or guard them behind a feature flag so
the package can be published safely without the upstream PR.
taras
commented
Mar 16, 2026
effect-ts/effect-runtime.ts
Outdated
| @@ -1,5 +1,5 @@ | |||
| import { type Effect, type Exit, Layer, ManagedRuntime } from "effect"; | |||
Member
Author
There was a problem hiding this comment.
Rebase on main this PR so this will disappear
taras
commented
Mar 16, 2026
process/README.md
Outdated
Comment on lines
+6
to
+9
| > **Note**: Starting with version 0.8.0, this package requires Effection v4.1 or greater | ||
| > for full functionality. The middleware/API features (`processApi`) require the new | ||
| > `createApi` function introduced in Effection v4.1. | ||
|
|
Member
Author
There was a problem hiding this comment.
This is no longer true, we should remove
taras
commented
Mar 16, 2026
process/README.md
Outdated
| expect(): Operation<ExitStatus>; | ||
| } | ||
|
|
||
| ### `processApi` |
Member
Author
There was a problem hiding this comment.
Needs to be updated to mach casing
taras
commented
Mar 16, 2026
taras
commented
Mar 16, 2026
cowboyd
reviewed
Mar 16, 2026
process/api.ts
Outdated
Comment on lines
+33
to
+38
| /** | ||
| * Core interface for the process API operations. | ||
| * Used internally by createApi to enable middleware support. | ||
| */ | ||
| export interface ProcessApiCore { | ||
| /** |
Member
There was a problem hiding this comment.
See FS PR, but Core in this context is a throw-away word
process/api.ts
Outdated
Comment on lines
+133
to
+143
| export function exec(command: string, options: ExecOptions = {}): Exec { | ||
| function* doExec(): Operation<Process> { | ||
| return yield* coreExec(command, options); | ||
| } | ||
|
|
||
| return { | ||
| *[Symbol.iterator]() { | ||
| return yield* doExec(); | ||
| }, | ||
| *join() { | ||
| const process = yield* doExec(); |
Member
There was a problem hiding this comment.
Agree with code rabbit here. Highly suspicious
Introduce @effectionx/context-api integration for @effectionx/process, allowing middleware interception of process creation. Rename from createProcess to exec/daemon per review feedback, use ProcessHandler naming convention, bump version to 0.8.0.
taras
commented
Mar 27, 2026
process/api.ts
Outdated
| }, | ||
|
|
||
| *daemon(command: string, options: ExecOptions): Operation<Daemon> { | ||
| let process = yield* ProcessApi.operations.exec(command, options); |
Member
Author
There was a problem hiding this comment.
why is this not inside the resource?
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.
Motivation
Add middleware support to
@effectionx/processusing Effection's newcreateApifeature, enabling logging, mocking, and instrumentation of process execution for testing and observability.This is part of a series of PRs converting effectionx packages to use the new
createApipattern:@effectionx/fetch- feat(fetch): add fetchApi with middleware support #158@effectionx/process- this PR@effectionx/fs- coming nextApproach
processApiwith acreateProcessoperation that middleware can interceptexec()to use the API internally while maintaining backward compatibilitypkg.pr.new/thefrontside/effection@1116preview forcreateApisupport