Harden server Effect error handling and normalize service/runtime wiring#95
Harden server Effect error handling and normalize service/runtime wiring#95juliusmarminge merged 8 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
WalkthroughCentralizes runtime Promise error handling via new Schema-tagged runtime errors and tryRuntimePromise wrappers, updates wait/sleep behavior, adds conditional --goto handling for editor launches, widens error typing in Git tests, introduces a Pty initialization error, normalizes WS error responses, and tweaks chat code font sizes. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/integration/OrchestrationEngineHarness.integration.ts`:
- Around line 345-356: The current teardown uses sequential tryRuntimePromise
calls (invoking providerService.stopAll, Scope.close, and runtime.dispose) and
then runs fs.rmSync(rootDir...), but if any of those throws the cleanup line
never runs; wrap the whole shutdown sequence in an Effect that guarantees
filesystem cleanup by using Effect.ensuring (or a finally-style combinator) so
fs.rmSync(rootDir, { recursive: true, force: true }) always runs regardless of
failures; specifically, create an Effect that executes the existing
tryRuntimePromise(...) calls (references: tryRuntimePromise,
providerService.stopAll, runtime.runPromise, Scope.close, runtime.dispose) and
attach .ensuring(Effect.sync(() => fs.rmSync(rootDir, { recursive: true, force:
true }))) to it so the temp dir is removed even if
stopAll/Scope.close/runtime.dispose fails.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/integration/orchestrationEngine.integration.test.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/keybindings.test.tsapps/server/src/open.test.tsapps/server/src/open.tsapps/server/src/terminal/Layers/NodePTY.tsapps/server/src/wsServer.tsapps/web/src/index.css
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/integration/OrchestrationEngineHarness.integration.ts`:
- Line 259: Scope.make is being called with a string; update the call to pass
the proper ExecutionStrategy enum/value instead of "sequential". Locate the
Scope.make(...) call (symbol: Scope.make) and replace the string argument with
the ExecutionStrategy token from the Effect API (e.g.,
ExecutionStrategy.sequential) or remove the argument entirely to use the
default; ensure you import or reference ExecutionStrategy so the value resolves
correctly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/integration/orchestrationEngine.integration.test.ts
juliusmarminge
force-pushed
the
codething/a38b78be
branch
from
dbff078 to
b213da0
Compare
- Replace raw `Effect.promise`/generic errors with `Effect.tryPromise` and tagged schema errors - Improve orchestration harness/test reliability with explicit timeout/runtime error types - Tighten related tests and error assertions to match typed error flows
- replace custom Promise-based sleep helpers with `Effect.sleep` - remove now-unused integration sleep error class and harness error mapping
- Use domain errors directly in GitManager tests (`GitHubCliError`, `TextGenerationError`) - Remove fixture-only GH error normalization in test scaffolding - Introduce shared `PtyAdapterInitializationError` and use it in NodePTY adapter init
juliusmarminge
force-pushed
the
codething/a38b78be
branch
from
b213da0 to
2742cd8
Compare
- Use generic type parameter for stream errors instead of unknown - Type ServerShape errors as ServerLifecycleError instead of unknown - Add error mapping for autoBootstrapProject and httpServerListen - Type makeServerProviderLayer with ProviderUnsupportedError - Use Effect.try with decodeUnknownSync in projector to avoid unknown requirements - Fix test layer types from any/unknown to never Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
Schema.TaggedErrorEffect errors.NodeServicesimport style andinstanceofinvariant checks).t3/...names to concise domain-scoped names (for exampleserver/*,git/*,checkpointing/*).--gotofile targets with line/column.Testing
Note
Medium Risk
Touches server startup/WebSocket routing and orchestration integration harness runtime wiring; behavior should be unchanged but error mapping and failure propagation paths are modified and could surface new failures or alter shutdown/start semantics.
Overview
Hardens async/server flows by replacing untyped promise failures with typed
Schema.TaggedErrorClasserrors and explicitEffecterror mapping. Integration harness waiting now times out withWaitForTimeoutError, runtime service loading/starting is wrapped viatryRuntimePromise(OrchestrationHarnessRuntimeError), and sleeps switch toEffect.sleep.Makes server lifecycle failures explicit and safer to handle.
createServernow fails withServerLifecycleError, maps auto-bootstrap and HTTP listen errors into it, tightensstopSignaltyping, and wrapsprojects.searchEntriesfailures into aRouteRequestErrorinstead of leaking raw promise rejections.Normalizes error checks and test typing across git/keybindings/orchestration. Uses
Schema.is(...)instead ofinstanceoffor tagged errors, tightens effect error types in git layers/tests (including fakeghfailures), switches orchestration projector decoding to syncSchema.decodeUnknownSyncwrapped inEffect.try, and disables Turbotestcaching.Written by Cursor Bugbot for commit be2bb4f. This will update automatically on new commits. Configure here.
Note
Harden server error handling by introducing structured tagged errors and wiring runtime/service flows to
Effect-based failures across server startup, orchestration harness, and test utilitiesIntroduce tagged errors for timeouts and runtime failures, normalize schema-tag checks, and convert server lifecycle and route handling to
ServerLifecycleError/RouteRequestError; refactor waits and sleeps toEffect.sleep/Scheduleand wrap dynamic imports and promises withEffect.tryPromisein apps/server/integration/OrchestrationEngineHarness.integration.ts, apps/server/src/wsServer.ts, and related helpers.📍Where to Start
Start with
createServerin apps/server/src/wsServer.ts, then reviewmakeOrchestrationIntegrationHarnessandwaitForin apps/server/integration/OrchestrationEngineHarness.integration.ts.Macroscope summarized be2bb4f.
Summary by CodeRabbit
Refactor
Tests