fix: sync pre-httpapi stability baseline#506
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR contains three independent improvement areas: desktop electron hardening (IPC store-get error handling, console EPIPE suppression, webview zoom state refactoring), provider reasoning-effort configuration for GPT-5 and Gemini models across multiple AI SDK providers, and server middleware reordering with CORS test validation. ChangesDesktop Electron Error Handling & State Management
Provider Reasoning-Effort & Thinking Configuration
OpenCode Server Middleware & CORS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/desktop-electron/src/main/logging.ts (1)
46-60: 💤 Low valueLGTM — solid EPIPE guard.
writeFnis a v5-introduced callback, so the monkey-patch is correctly targeted. The.bind(log.transports.console)preservesthis, and disabling a transport by setting its level tofalseis the documented API. TheisBrokenPipe()guard is type-safe and correctly narrowsunknown.One theoretical concern: if
initLogging()is called more than once,writeFnwill be double-wrapped (the inner wrapper then becomes the capturedwritein the outer). This is harmless in practice sinceinitLogging()is called once at startup, but a guard such as a module-level boolean flag would eliminate the possibility entirely.🛡️ Optional: guard against double-wrapping
+let consoleTransportInitialized = false + function initConsoleTransport() { + if (consoleTransportInitialized) return + consoleTransportInitialized = true const write = log.transports.console.writeFn.bind(log.transports.console) log.transports.console.writeFn = (options) => {🤖 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/desktop-electron/src/main/logging.ts` around lines 46 - 60, The initConsoleTransport function can be double-wrapped if initLogging() is invoked multiple times; add a simple module-level guard (e.g., a boolean like consoleTransportInited) checked at the top of initConsoleTransport and set to true after wrapping to prevent re-wrapping the existing log.transports.console.writeFn; reference initConsoleTransport, initLogging (caller), and log.transports.console.writeFn when making the change.packages/desktop-electron/src/main/logging.test.ts (1)
7-14: ⚡ Quick winSource-text assertions validate structure but not behavior.
All four
toContain()checks scan the raw.tssource for substrings. This means the test cannot distinguish correct logic from inverted logic — for example, it would pass equally forif (isBrokenPipe(err)) throw err(wrong direction) as for the currentif (!isBrokenPipe(err)) throw err, because the string'err.code === "EPIPE"'appears inisBrokenPipe()'s body regardless of how the result is used at the call site.Consider replacing with a lightweight mock-based behavioral test:
♻️ Proposed behavioral replacement
-import { describe, expect, test } from "bun:test" -import { readFileSync } from "node:fs" -import { resolve } from "node:path" - -const source = readFileSync(resolve(import.meta.dir, "logging.ts"), "utf8") - -describe("desktop logging", () => { - test("disables the console transport after a broken pipe", () => { - expect(source).toContain("initConsoleTransport()") - expect(source).toContain("log.transports.console.writeFn") - expect(source).toContain('err.code === "EPIPE"') - expect(source).toContain("log.transports.console.level = false") - }) -}) +import { describe, expect, test } from "bun:test" +import { isBrokenPipe, initConsoleTransportForTest } from "./logging" + +// Export these from logging.ts for testing, or restructure as shown below. +// Alternatively, test via a controlled mock of log.transports.console: + +describe("desktop logging", () => { + test("isBrokenPipe detects EPIPE errors", () => { + expect(isBrokenPipe({ code: "EPIPE" })).toBe(true) + expect(isBrokenPipe({ code: "ENOENT" })).toBe(false) + expect(isBrokenPipe(null)).toBe(false) + expect(isBrokenPipe("string")).toBe(false) + }) + + test("disables console transport and does not rethrow on EPIPE", () => { + const fakeTransport = { level: "silly" as string | false, writeFn: null as any } + const epipeError = Object.assign(new Error("EPIPE"), { code: "EPIPE" }) + fakeTransport.writeFn = () => { throw epipeError } + // call initConsoleTransportForTest(fakeTransport) to exercise the wrapper + // then assert fakeTransport.level === false + }) +})Note:
isBrokenPipeand a test-oriented hook would need to be exported fromlogging.ts. The sketch above shows the structure — adapt to your module's export conventions.🤖 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/desktop-electron/src/main/logging.test.ts` around lines 7 - 14, The test currently only asserts source text contains substrings and not behavior; replace it with a behavioral mock test that imports initConsoleTransport and isBrokenPipe (export isBrokenPipe and a test hook from logging.ts), then mock or stub log.transports.console.writeFn to simulate a write error with err.code === "EPIPE" and verify that initConsoleTransport disables the console transport (e.g., log.transports.console.level === false) when isBrokenPipe returns true and that it rethrows or leaves transport enabled when false; update logging.ts to export isBrokenPipe and any minimal test hook needed so the test can control the broken-pipe predicate and assert real runtime behavior rather than source text.packages/desktop-electron/src/renderer/webview-zoom.test.ts (1)
5-21: 🏗️ Heavy liftPrefer behavior-level tests over source-string assertions.
These checks are useful as a temporary guard, but they’re brittle (format/structure-sensitive) and can miss runtime regressions. Consider testing observable behavior instead (dispatch keydown events, assert
preventDefault, and assertwindow.api.setZoomFactor/ zoom state interactions).🤖 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/desktop-electron/src/renderer/webview-zoom.test.ts` around lines 5 - 21, Replace brittle source-string assertions in webview-zoom.test.ts with behavior-level tests: simulate keyboard events (e.g., '-' '+' '=' '0') that the module should handle, spy on Event.prototype.preventDefault to assert it was called only for keys that change zoom, and mock/spy window.api.setZoomFactor (and any setWebviewZoom/webviewZoom interactions) to assert the correct zoom values are requested and that requestedZoom/state updates only take effect after Electron accepts the change; locate helpers/variables by name: requestedZoom, setWebviewZoom, webviewZoom, and window.api.setZoomFactor when converting the tests to use dispatched KeyboardEvent and spies instead of string matching.
🤖 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/desktop-electron/src/main/ipc-window-config.test.ts`:
- Around line 29-31: The test currently computes start and end via
source.indexOf('ipcMain.handle("store-get"') and
source.indexOf('ipcMain.handle("store-set"', start) then slices without
validating those indices; add explicit bounds checks to ensure both markers were
found and that start < end before calling slice (e.g., verify start !== -1, end
!== -1, and end > start) and fail the test with a clear message if checks fail
so the regression test fails loudly when markers drift; update the variables
referenced (start, end, handler, source.indexOf(...) usages) accordingly.
---
Nitpick comments:
In `@packages/desktop-electron/src/main/logging.test.ts`:
- Around line 7-14: The test currently only asserts source text contains
substrings and not behavior; replace it with a behavioral mock test that imports
initConsoleTransport and isBrokenPipe (export isBrokenPipe and a test hook from
logging.ts), then mock or stub log.transports.console.writeFn to simulate a
write error with err.code === "EPIPE" and verify that initConsoleTransport
disables the console transport (e.g., log.transports.console.level === false)
when isBrokenPipe returns true and that it rethrows or leaves transport enabled
when false; update logging.ts to export isBrokenPipe and any minimal test hook
needed so the test can control the broken-pipe predicate and assert real runtime
behavior rather than source text.
In `@packages/desktop-electron/src/main/logging.ts`:
- Around line 46-60: The initConsoleTransport function can be double-wrapped if
initLogging() is invoked multiple times; add a simple module-level guard (e.g.,
a boolean like consoleTransportInited) checked at the top of
initConsoleTransport and set to true after wrapping to prevent re-wrapping the
existing log.transports.console.writeFn; reference initConsoleTransport,
initLogging (caller), and log.transports.console.writeFn when making the change.
In `@packages/desktop-electron/src/renderer/webview-zoom.test.ts`:
- Around line 5-21: Replace brittle source-string assertions in
webview-zoom.test.ts with behavior-level tests: simulate keyboard events (e.g.,
'-' '+' '=' '0') that the module should handle, spy on
Event.prototype.preventDefault to assert it was called only for keys that change
zoom, and mock/spy window.api.setZoomFactor (and any setWebviewZoom/webviewZoom
interactions) to assert the correct zoom values are requested and that
requestedZoom/state updates only take effect after Electron accepts the change;
locate helpers/variables by name: requestedZoom, setWebviewZoom, webviewZoom,
and window.api.setZoomFactor when converting the tests to use dispatched
KeyboardEvent and spies instead of string matching.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 720473a2-e25d-4377-92e8-8a9b19785f43
📒 Files selected for processing (10)
packages/desktop-electron/src/main/ipc-window-config.test.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/main/logging.test.tspackages/desktop-electron/src/main/logging.tspackages/desktop-electron/src/renderer/webview-zoom.test.tspackages/desktop-electron/src/renderer/webview-zoom.tspackages/opencode/src/provider/transform.tspackages/opencode/src/server/server.tspackages/opencode/test/provider/transform.test.tspackages/opencode/test/server/cors-middleware.test.ts
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error handling in Electron IPC and logging, refines webview zoom behavior to prevent default browser actions, and significantly updates model provider transformations to support granular reasoning efforts for GPT-5, Gemini-3, and Claude models. Additionally, server middleware was reordered to ensure CORS headers are correctly applied to unauthorized responses. Feedback suggests adding a return statement in the zoom key handler for consistency and aligning Azure reasoning effort logic with other providers for GPT-5 chat models to ensure uniform behavior across the platform.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/desktop-electron/src/renderer/webview-zoom.test.ts (1)
115-115: ⚡ Quick winAvoid hard-coding the floating-point artifact in the assertion.
Line 115 currently locks in
0.6000000000000001, which is representation-specific and can cause fragile failures if implementation rounds to0.6without changing behavior.Suggested test-robustness patch
- expect(setZoomFactor.mock.calls.map(([factor]) => factor)).toEqual([0.8, 0.6000000000000001, 1]) + const factors = setZoomFactor.mock.calls.map(([factor]) => factor) + expect(factors).toHaveLength(3) + expect(factors[0]).toBeCloseTo(0.8, 10) + expect(factors[1]).toBeCloseTo(0.6, 10) + expect(factors[2]).toBe(1)🤖 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/desktop-electron/src/renderer/webview-zoom.test.ts` at line 115, The assertion hard-codes a floating-point artifact (0.6000000000000001) making the test fragile; update the test in webview-zoom.test.ts to compare zoom factors with tolerance instead of exact equality by retrieving the values from setZoomFactor.mock.calls (the same map(([factor]) => factor) expression) and assert the middle value with a floating-point tolerant check (e.g., use Jest's toBeCloseTo or round to a fixed number of decimals) while keeping the other expected values as-is.
🤖 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/desktop-electron/src/renderer/webview-zoom.test.ts`:
- Line 115: The assertion hard-codes a floating-point artifact
(0.6000000000000001) making the test fragile; update the test in
webview-zoom.test.ts to compare zoom factors with tolerance instead of exact
equality by retrieving the values from setZoomFactor.mock.calls (the same
map(([factor]) => factor) expression) and assert the middle value with a
floating-point tolerant check (e.g., use Jest's toBeCloseTo or round to a fixed
number of decimals) while keeping the other expected values as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 426eecc0-0811-42e6-8736-eb8fff6d71e1
📒 Files selected for processing (7)
packages/desktop-electron/src/main/ipc-window-config.test.tspackages/desktop-electron/src/main/logging.test.tspackages/desktop-electron/src/main/logging.tspackages/desktop-electron/src/renderer/webview-zoom.test.tspackages/desktop-electron/src/renderer/webview-zoom.tspackages/opencode/src/provider/transform.tspackages/opencode/test/provider/transform.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-electron/src/main/ipc-window-config.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-electron/src/main/logging.ts
- packages/desktop-electron/src/renderer/webview-zoom.ts
66 tasks
Astro-Han
added a commit
that referenced
this pull request
May 9, 2026
Refs #477. This lands the PR5 foundation slice for the upstream sync track after #506. It intentionally absorbs the Effect-backed instance foundation and the smallest effectCmd proof chain without introducing the HttpApi backend, changing the default Hono listener, or pulling in session/warp/workspace migrations. Why this change exists: - upstream moved instance bootstrap and command execution toward Effect services; - PawWork needed the foundation before any later HttpApi parity work could be reviewed cleanly; - the old explicit InstanceBootstrap call sites were easy to miss across Hono, CLI, worktree, watcher, and tool paths; - effectCmd needed a minimal proof, but a broad CLI migration would have made rollback and review too noisy. What changed: - added Effect-backed InstanceBootstrap, InstanceStore, InstanceRuntime, InstanceLayer, InstanceRef, and InstanceState wiring; - made InstanceStore own boot, reload, dispose, failed-load retry, and single-flight behavior; - routed legacy Instance.provide, WithInstance, Hono instance middleware, CLI bootstrap, worktree reload, watcher, bash tool, and related service boundaries through the new instance context path; - kept plugin/config bootstrap failures fatal and documented that contract, while optional services continue to log and continue; - registered bootstrap subscriptions with the instance disposer so reload/dispose cleans them up; - added effectCmd and migrated only the models command as the proof chain; - preserved Instance.current across Effect.promise and nested AppRuntime.runPromise in the CLI proof path; - moved File service bootstrap reads from legacy ALS Instance.* to InstanceState.context, so File.init works when only InstanceRef is available during store boot; - added regression coverage for plugin/config bootstrap, InstanceStore load/reload/dispose, effectCmd ALS bridging, File.init under InstanceRef, and /init command events marking Project.setInitialized. Review and follow-up notes: - this is a production-path runtime change because default Hono and CLI paths now flow through InstanceStore; - Hono remains the default server path and fallback; - HttpApi parity was split out after the split trigger because importing it would have pulled in excluded listener/session/warp/workspace chains; - PR6 should continue #477 with non-default HttpApi parity only after this foundation is stable; - broad CLI command migration, desktop/app package consolidation, HttpApi default listener switch, v2 session, warp, sync ownership, /sync/steal, and workspace-aware patch apply remain explicitly out of scope. Verification: - GitHub checks on 3b48455 all passed: ci typecheck, unit-opencode, unit-desktop, unit-app, check, dependency-review, commit-lint, codeql, desktop-smoke, e2e-artifacts, and CodeRabbit; - local typecheck passed: bun --cwd packages/opencode typecheck; - focused tests passed: bun --cwd packages/opencode test test/file/index.test.ts test/project/instance-bootstrap-regression.test.ts; - full opencode CI test suite passed locally: bun --cwd packages/opencode test:ci, 2702 pass, 9 skip, 1 todo, 0 fail; - diff check passed: git diff --check; - exclusion scans found no HttpApi or warp/session scope matches in the changed package areas; - review thread check before merge found 0 unresolved review threads.
2 tasks
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
1 participant
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
Why
This is the PR 4 pre-HttpApi stability baseline for #477. It intentionally keeps the remaining upstream sync moving with one rollback surface around pre-HttpApi safety/stability, while excluding Effect foundation, effectCmd, HttpApi listener work, default listener switching, package consolidation, and v2 session/warping contracts.
Related Issue
Refs #477.
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
minimalvalues for versioned GPT-5.x models.Risk Notes
How To Verify
Screenshots or Recordings
Not required. This PR changes desktop shell behavior guards, not visible UI layout or copy.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
Bug Fixes
New Features
Improvements