chore: effect rpc and layer based startup#929
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
the
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Dev URL redirect drops request path and query
- Fixed the redirect to construct a new URL from the request's pathname and search params against the dev origin, and updated the test to assert the correct behavior.
Or push these changes by commenting:
@cursor push 089fb595df
Preview (089fb595df)
diff --git a/apps/server/src/http.ts b/apps/server/src/http.ts
--- a/apps/server/src/http.ts
+++ b/apps/server/src/http.ts
@@ -90,7 +90,8 @@
const config = yield* ServerConfig;
if (config.devUrl) {
- return HttpServerResponse.redirect(config.devUrl.href, { status: 302 });
+ const devTarget = new URL(`${url.pathname}${url.search}`, config.devUrl);
+ return HttpServerResponse.redirect(devTarget.href, { status: 302 });
}
if (!config.staticDir) {
diff --git a/apps/server/src/server.test.ts b/apps/server/src/server.test.ts
--- a/apps/server/src/server.test.ts
+++ b/apps/server/src/server.test.ts
@@ -266,7 +266,7 @@
const response = yield* Effect.promise(() => fetch(url, { redirect: "manual" }));
assert.equal(response.status, 302);
- assert.equal(response.headers.get("location"), "http://127.0.0.1:5173/");
+ assert.equal(response.headers.get("location"), "http://127.0.0.1:5173/foo/bar");
}).pipe(Effect.provide(NodeHttpServer.layerTest)),
);|
|
||
| const config = yield* ServerConfig; | ||
| if (config.devUrl) { | ||
| return HttpServerResponse.redirect(config.devUrl.href, { status: 302 }); |
There was a problem hiding this comment.
Dev URL redirect drops request path and query
High Severity
The dev URL redirect uses config.devUrl.href without appending the incoming request's pathname or search params. Every GET request (e.g. /settings?tab=general) redirects to the dev server root (http://127.0.0.1:5173/) instead of the matching route (http://127.0.0.1:5173/settings?tab=general). The reference implementation correctly constructs new URL(url.pathname + url.search, origin). The test at line 269 asserts the broken behavior, masking the regression.
Additional Locations (1)
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
github-actions
bot
added
the
size:XXL
juliusmarminge
force-pushed
the
effect-http-router
branch
from
ede8523 to
8ea82bc
Compare
| ? Deferred.succeed(deferred, exit.value) | ||
| : Deferred.failCause(deferred, exit.cause); | ||
|
|
||
| export const makeCommandGate = Effect.gen(function* () { |
There was a problem hiding this comment.
🟢 Low src/serverRuntimeStartup.ts:62
Once signalCommandReady sets commandReadinessState to "ready", enqueueCommand bypasses the queue and runs effects directly (line 86-87), while commands already in the queue are still being processed by commandWorker. This allows commands arriving after the state transition to execute concurrently with queued commands, breaking the sequential ordering guarantee that the queue was designed to provide.
Also found in 1 other location(s)
.reference/server/src/model-store.ts:218
Race condition in
subscribe: Events published toeventsPubSubduring thecatchupquery execution will be missed.Stream.concat(catchup, live)only subscribes to the PubSub aftercatchupcompletes. Ifappendpublishes events whilecatchupis querying the database, those events won't be in the catchup results (they occurred after the query started) and won't be captured bylive(subscription wasn't established yet). This causes event loss for concurrent subscriptions.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/serverRuntimeStartup.ts around line 62:
Once `signalCommandReady` sets `commandReadinessState` to `"ready"`, `enqueueCommand` bypasses the queue and runs effects directly (line 86-87), while commands already in the queue are still being processed by `commandWorker`. This allows commands arriving after the state transition to execute concurrently with queued commands, breaking the sequential ordering guarantee that the queue was designed to provide.
Evidence trail:
apps/server/src/serverRuntimeStartup.ts lines 64-67 (commandWorker forked in separate fiber), lines 69-72 (signalCommandReady sets state to ready first, then succeeds deferred), lines 79-97 (enqueueCommand checks state and runs directly if ready at lines 82-83, or queues with Deferred.await(commandReady) at lines 90-95)
Also found in 1 other location(s):
- .reference/server/src/model-store.ts:218 -- Race condition in `subscribe`: Events published to `eventsPubSub` during the `catchup` query execution will be missed. `Stream.concat(catchup, live)` only subscribes to the PubSub after `catchup` completes. If `append` publishes events while `catchup` is querying the database, those events won't be in the catchup results (they occurred after the query started) and won't be captured by `live` (subscription wasn't established yet). This causes event loss for concurrent subscriptions.
- Replace state-dir config with T3CODE_HOME/base-dir derivation - Route attachments and provider logs through derived paths - Lazily load Bun/Node HTTP, PTY, and platform services
Co-authored-by: codex <codex@users.noreply.github.com>
| [WS_METHODS.subscribeOrchestrationDomainEvents]: (_input) => | ||
| Stream.unwrap( | ||
| Effect.gen(function* () { | ||
| const snapshot = yield* orchestrationEngine.getReadModel(); | ||
| const fromSequenceExclusive = snapshot.snapshotSequence; | ||
| const replayEvents: Array<OrchestrationEvent> = yield* Stream.runCollect( | ||
| orchestrationEngine.readEvents(fromSequenceExclusive), | ||
| ).pipe( | ||
| Effect.map((events) => Array.from(events)), | ||
| Effect.catch(() => Effect.succeed([] as Array<OrchestrationEvent>)), | ||
| ); |
There was a problem hiding this comment.
🟡 Medium src/ws.ts:148
In subscribeOrchestrationDomainEvents, when orchestrationEngine.readEvents() fails, the error is silently caught and replaced with an empty array. The state machine then initializes nextSequence = fromSequenceExclusive + 1, but since the replay failed, events with that sequence number will never arrive. Live events accumulate in pendingBySequence waiting for the missing sequence, causing the subscription to hang indefinitely and leak memory. Consider propagating the replay error instead of silently succeeding so the client receives a failure indication.
const replayEvents: Array<OrchestrationEvent> = yield* Stream.runCollect(
orchestrationEngine.readEvents(fromSequenceExclusive),
).pipe(
Effect.map((events) => Array.from(events)),
- Effect.catch(() => Effect.succeed([] as Array<OrchestrationEvent>)),
+ Effect.mapError(
+ (cause) =>
+ new OrchestrationReplayEventsError({
+ message: "Failed to replay orchestration events for subscription",
+ cause,
+ }),
+ ),
);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/ws.ts around lines 148-158:
In `subscribeOrchestrationDomainEvents`, when `orchestrationEngine.readEvents()` fails, the error is silently caught and replaced with an empty array. The state machine then initializes `nextSequence = fromSequenceExclusive + 1`, but since the replay failed, events with that sequence number will never arrive. Live events accumulate in `pendingBySequence` waiting for the missing sequence, causing the subscription to hang indefinitely and leak memory. Consider propagating the replay error instead of silently succeeding so the client receives a failure indication.
Evidence trail:
apps/server/src/ws.ts lines 148-200 at REVIEWED_COMMIT:
- Line 157: `Effect.catch(() => Effect.succeed([] as Array<OrchestrationEvent>))` silently catches errors
- Lines 164-167: State initialization with `nextSequence: fromSequenceExclusive + 1`
- Line 160: `Stream.merge(replayStream, orchestrationEngine.streamDomainEvents)` merges replay with live
- Lines 170-198: State machine logic that buffers out-of-order events in `pendingBySequence` and only emits when `nextSequence` matches
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
- Replace the temporary PubSub bridge with `Stream.callback` - Wire terminal subscriptions through `Queue.offer` for simpler cleanup - Preserve unsubscribe handling with acquire/release
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
| connect(client: BrowserWsClient): void { | ||
| if (this.scope) { | ||
| void Effect.runPromise(Scope.close(this.scope, Exit.void)).catch(() => undefined); | ||
| } | ||
| if (this.streamPubSubs.size === 0) { | ||
| this.initializeStreamPubSubs(); | ||
| } | ||
| this.client = client; | ||
| this.scope = Effect.runSync(Scope.make()); |
There was a problem hiding this comment.
🟢 Low test/wsRpcHarness.ts:73
When connect() is called while a previous connection exists, the old scope is closed asynchronously without awaiting completion (void Effect.runPromise(...) on line 75). Immediately after, this.client is reassigned to the new client on line 80 and a new scope is created on line 81. If the old server has pending finalizers that emit responses via this.client.send(), those responses will be sent to the new client instead of the old one, since this.client is already updated. This race condition causes unexpected messages to be received by the new client.
connect(client: BrowserWsClient): void {
- if (this.scope) {
- void Effect.runPromise(Scope.close(this.scope, Exit.void)).catch(() => undefined);
- }
+ if (this.scope) {
+ Effect.runSync(Scope.close(this.scope, Exit.void));
+ }
if (this.streamPubSubs.size === 0) {
this.initializeStreamPubSubs();
}
this.client = client;
this.scope = Effect.runSync(Scope.make());🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/test/wsRpcHarness.ts around lines 73-81:
When `connect()` is called while a previous connection exists, the old scope is closed asynchronously without awaiting completion (`void Effect.runPromise(...)` on line 75). Immediately after, `this.client` is reassigned to the new client on line 80 and a new scope is created on line 81. If the old server has pending finalizers that emit responses via `this.client.send()`, those responses will be sent to the *new* client instead of the old one, since `this.client` is already updated. This race condition causes unexpected messages to be received by the new client.
Evidence trail:
apps/web/src/test/wsRpcHarness.ts lines 73-88 (connect method): Line 75 shows `void Effect.runPromise(Scope.close(this.scope, Exit.void))` - async close without await. Line 80: `this.client = client` - immediate reassignment.
apps/web/src/test/wsRpcHarness.ts lines 141-149 (makeServerOptions): The `onFromServer` callback captures `this` and accesses `this.client.send()` at execution time, not at callback creation time. This means old server finalizers would reference the newly-assigned client after line 80 executes.
- Import shared git error types directly from `@t3tools/contracts` - Remove the local git error re-export module
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Boolean flag
falsevalues silently ignored by filter- Replaced
Option.filter(flag, Boolean)withOption.getOrElse(flag, ...)so thatSome(false)is correctly preserved instead of being converted toNone.
- Replaced
- ✅ Fixed: Settings update error drops schema validation details
- Replaced the generic error message with
Cause.pretty(decoded.cause)and passed the cause through, consistent with how schema errors are reported elsewhere in the codebase.
- Replaced the generic error message with
Or push these changes by commenting:
@cursor push f7ab68c0c3
Preview (f7ab68c0c3)
diff --git a/apps/server/src/cli.ts b/apps/server/src/cli.ts
--- a/apps/server/src/cli.ts
+++ b/apps/server/src/cli.ts
@@ -104,7 +104,7 @@
}
const resolveBooleanFlag = (flag: Option.Option<boolean>, envValue: boolean) =>
- Option.getOrElse(Option.filter(flag, Boolean), () => envValue);
+ Option.getOrElse(flag, () => envValue);
export const resolveServerConfig = (
flags: CliServerFlags,
diff --git a/apps/server/src/serverSettings.ts b/apps/server/src/serverSettings.ts
--- a/apps/server/src/serverSettings.ts
+++ b/apps/server/src/serverSettings.ts
@@ -21,6 +21,7 @@
} from "@t3tools/contracts";
import {
Cache,
+ Cause,
Deferred,
Duration,
Effect,
@@ -317,7 +318,8 @@
if (decoded._tag === "Failure") {
return yield* new ServerSettingsError({
settingsPath: "<memory>",
- detail: "failed to normalize server settings",
+ detail: Cause.pretty(decoded.cause),
+ cause: decoded.cause,
});
}
const next = decoded.value;You can send follow-ups to this agent here.
| } | ||
|
|
||
| const resolveBooleanFlag = (flag: Option.Option<boolean>, envValue: boolean) => | ||
| Option.getOrElse(Option.filter(flag, Boolean), () => envValue); |
There was a problem hiding this comment.
Boolean flag false values silently ignored by filter
Medium Severity
resolveBooleanFlag uses Option.filter(flag, Boolean) which filters out false values — Boolean(false) is false, so Some(false) becomes None, falling through to the env/default value. This means explicitly passing a boolean CLI flag as false (e.g., negating --auto-bootstrap-project-from-cwd) has no effect; the environment or default value wins instead. The intent is clearly to let CLI flags override env values for both true and false, but Option.filter with Boolean only preserves true.
| settingsPath: "<memory>", | ||
| detail: "failed to normalize server settings", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Settings update error drops schema validation details
Low Severity
The updateSettings handler switched from Schema.decodeEffect (which provided detailed schema issue formatting via SchemaIssue.makeFormatterDefault() and passed the cause) to Schema.decodeUnknownExit with a generic error message "failed to normalize server settings" and no cause. When a user submits an invalid settings patch, the error no longer explains what was invalid, making debugging significantly harder.
1 participant
What Changed
Why
UI Changes
Checklist
Note
High Risk
Large refactor of server boot, HTTP routing, and WebSocket protocol handling; mistakes could break client connectivity, auth gating, static/attachment serving, or startup sequencing across Bun/Node runtimes.
Overview
Refactors the server to a new layer-launched runtime: introduces
src/server.tsas the composition root (Bun/Node service selection, PTY + HTTP server selection, runtime services wiring) and replaces the previous CLI entrypoint withsrc/bin.ts+ a newsrc/cli.tsthat resolves config from flags/env (including newlogLevelonServerConfig).Replaces ad-hoc WebSocket request handling with a typed Effect RPC endpoint at
GET /ws(src/ws.ts), including token-based handshake auth and handlers/streams for orchestration, git, terminal, settings, keybindings, and workspace file ops; adds command normalization (orchestration/Normalizer.ts) to validate workspace roots and persist inbound image attachments.Splits HTTP concerns into dedicated router layers (
src/http.ts) forGET /health, attachment serving (by id or relative path), and static-file serving or dev-URL redirect with path traversal guards; updates logging (serverLogger.ts) to honor configured minimum log level. Also centralizes several error types into@t3tools/contracts, removes local git error definitions, and adjusts multiple integration-test timeouts to reduce flakiness.Written by Cursor Bugbot for commit 849a39c. This will update automatically on new commits. Configure here.
Note
Replace WebSocket push-based transport with Effect RPC layer and refactor server startup
WsRpcGroup) using typed request/response/stream contracts defined in wsRpc.ts.wsServer.ts, composing all application layers and HTTP/WS routes with Node/Bun platform adapters.GitCommandError,TerminalError,KeybindingsConfigError,ServerSettingsError,OpenError, etc.) from local server files into@t3tools/contractsso they are shared across client and server.ServerConfigStreamEvent,ServerLifecycleStreamEvent) with subscription RPC methods, replacing ad-hoc push channels.getLatestPush,WS_CHANNELS) is removed; all callers must use the new RPC stream subscription model.Macroscope summarized 849a39c.