Repository navigation
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds Bun support for synchronous SQLite access, PTY process spawning, and WebSocket transport. It preserves Node implementations through runtime selection and extracted lifecycle helpers. Orcad health and readiness output now report the runtime and PTY backend. New scripts validate Bun PTY behavior, native package loading, terminal serving, daemon reattachment, and scrollback replay. The package scripts include a combined Bun smoke-test command. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 16 files. (1 skipped: 1 unsupported.) Full details: Description checkExplanation The description provides extensive change and validation details, but it omits required template sections including ELI5, Linked Issue, Visual Proof, AI Disclosure, Review, Agent skill upstream boundary, Notes, and the checklist items. Resolution Add all required template sections. Include an issue reference in the form ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 59ba1d9b-d98e-4547-abea-a7c8511a9452
📒 Files selected for processing (4)
config/scripts/orcad-bun-pty-poc.mjsdocs/design/orcad-bun-runtime.mdsrc/main/sqlite/sync-database.test.tssrc/main/sqlite/sync-database.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| if (!globalThis.Bun?.Terminal) { | ||
| throw new Error('Run this proof with Bun 1.3.14 or newer') | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable repository knowledge ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print \
| sort
printf '%s\n' '--- reviewed file ---'
cat -n config/scripts/orcad-bun-pty-poc.mjs
printf '%s\n' '--- Bun/version/PTY references ---'
rg -n -i --glob '!node_modules' --glob '!dist' --glob '!build' \
'bun(\.version|@| 1\.3\.1[34]|terminal|pty)|orcad-bun-pty-poc|slot' .Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
log=/tmp/coderabbit-shell-logs/shell-output-q67MMq
printf '%s\n' '--- relevant lines from prior output ---'
rg -n -C 3 'config/scripts/orcad-bun-pty-poc\.mjs|Bun\.version|Bun 1\.3\.1[34]|bun:[^ ]*|bun@|orcad-bun-pty|Terminal' "$log" | head -n 240
printf '%s\n' '--- knowledge file headers ---'
find /tmp/coderabbit-repo-knowledge/stablyai/orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print \
| sort | while IFS= read -r f; do
printf '\n### %s\n' "$f"
head -n 8 "$f"
done
printf '%s\n' '--- reviewed script ---'
cat -n config/scripts/orcad-bun-pty-poc.mjs
printf '%s\n' '--- exact Bun references in likely config/docs files ---'
rg -n -i --glob '*.yml' --glob '*.yaml' --glob '*.json' --glob '*.md' --glob '*.mjs' \
'bun|pty|terminal' config docs .github package.json 2>/dev/null | head -n 300Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- reviewed script ---'
cat -n config/scripts/orcad-bun-pty-poc.mjs
printf '%s\n' '--- knowledge files ---'
find /tmp/coderabbit-repo-knowledge/stablyai/orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print | sort
printf '%s\n' '--- knowledge headers ---'
find /tmp/coderabbit-repo-knowledge/stablyai/orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print | sort \
| while IFS= read -r f; do
printf '\n### %s\n' "$f"
head -n 12 "$f"
done
printf '%s\n' '--- candidate Bun/PTy files ---'
git ls-files | grep -Ei '(^|/)([^/]*(bun|pty)[^/]*)$|(^|/)(docs?|design|config)/' \
| grep -Ei 'bun|pty|design|doc|config' | head -n 200
printf '%s\n' '--- bounded version references ---'
git grep -n -i -E 'Bun\.version|Bun 1\.3\.13|Bun 1\.3\.14|oven-sh/bun|bun-version|bun:' -- \
':!src' ':!vendor' ':!packages' 2>/dev/null | head -n 200 || trueRepository: stablyai/orca
Length of output: 15599
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Bun pin/version declarations ---'
git grep -n -i -E 'oven-sh/bun|setup-bun|bun-version|BUN_VERSION|bun@[0-9]|bun:[[:space:]]*[0-9]|Bun [0-9]+\.[0-9]+\.[0-9]+|1\.3\.14|1\.3\.13' -- \
'.github/**' 'config/**' 'docs/**' 'package.json' 'package-lock.json' 'pnpm-lock.yaml' \
'yarn.lock' 'bun.lockb' 'bunfig.toml' 'Dockerfile*' 2>/dev/null | head -n 300 || true
printf '%s\n' '--- design context ---'
sed -n '1,55p' docs/design/orcad-bun-runtime.mdRepository: stablyai/orca
Length of output: 7480
Enforce the stated Bun version floor. This feature check can accept Bun 1.3.13, which the design records as passing Bun.Terminal. Compare Bun.version with 1.3.14 so the guard matches its error message.
| exec: vi.fn(), | ||
| prepare: vi.fn((_sql: string) => statement) | ||
| } | ||
| let constructorOptions: { readonly?: boolean; strict: boolean } | undefined |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the duplicate declaration. constructorOptions is declared twice in the same scope, so Vitest cannot compile this file.
| this.database = new BunDatabase(path, { | ||
| ...(options.readOnly ? { readonly: true } : {}), | ||
| strict: true | ||
| }) | ||
| if (options.timeout !== undefined && Number.isFinite(options.timeout)) { | ||
| this.database.exec(`PRAGMA busy_timeout = ${Math.max(0, Math.trunc(options.timeout))}`) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
printf '%s\n' '--- applicable repository knowledge ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- database adapter ---'
sed -n '1,150p' src/main/sqlite/sync-database.ts
printf '%s\n' '--- direct SQLite contracts and tests ---'
rg -n --glob '*.{ts,tsx,js,jsx,json,md}' 'foreign_keys|DatabaseSync|BunDatabase|bun:sqlite|loadDatabaseSync|busy_timeout' src package.json bun.lockb package-lock.json pnpm-lock.yaml yarn.lock docs 2>/dev/null | head -200Repository: stablyai/orca
Length of output: 18289
🏁 Script executed:
printf '%s\n' '--- adapter tests ---'
sed -n '1,115p' src/main/sqlite/sync-database.test.ts
printf '%s\n' '--- repository conventions for this scope ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src.md
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src-main-daemon.md
printf '%s\n' '--- schema and foreign-key consumers ---'
rg -n -C 3 --glob '*.{ts,sql}' 'FOREIGN KEY|REFERENCES|PRAGMA foreign_keys|CREATE TABLE' src/main/sqlite src/main 2>/dev/null | head -240Repository: stablyai/orca
Length of output: 25105
🌐 Web query:
Node.js latest v24 node:sqlite DatabaseSync enableForeignKeyConstraints default and Bun bun:sqlite Database foreign keys default
💡 Result:
In Node.js (specifically within the node:sqlite module's DatabaseSync class), the enableForeignKeyConstraints option defaults to true [1][2][3]. This represents a change from standard SQLite behavior to ensure foreign key enforcement is active for new applications by default [4]. In contrast, Bun's built-in bun:sqlite Database does not enable foreign key constraints by default [5][6]. Because SQLite itself disables foreign keys by default for backward compatibility [7], users of bun:sqlite must explicitly enable them for every connection by executing the SQL command PRAGMA foreign_keys = ON [6].
Citations:
- 1: https://nodejs.org/docs/latest-v24.x/api/sqlite.html
- 2: https://nodejs.org/download/release/latest-v24.x/docs/api/sqlite.html
- 3: https://nodejs.org/dist/latest-v24.x/docs/api/sqlite.html
- 4: nodejs/node@a49abec6c3
- 5: https://bun.com/docs/runtime/sqlite
- 6: https://helpmetest.com/blog/bun-sqlite-testing/
- 7: https://sqlite.org/foreignkeys.html
🏁 Script executed:
printf '%s\n' '--- adapter callers ---'
rg -n -C 2 --glob '*.ts' 'new SyncDatabase|SyncDatabase\.Database|from .*\./.*sync-database|sync-database' src/main src/relay | head -240
printf '%s\n' '--- foreign-key declarations in source and migrations ---'
rg -n -C 2 --glob '*.{ts,tsx,js,jsx,sql}' '\bREFERENCES\b|\bFOREIGN\s+KEY\b|foreign_keys' src | head -240
printf '%s\n' '--- runtime support declarations ---'
rg -n -C 2 --glob 'package.json' '"engines"|"bun"|"node"' package.json src/**/package.json 2>/dev/nullRepository: stablyai/orca
Length of output: 20655
🏁 Script executed:
printf '%s\n' '--- non-test SyncDatabase write path ---'
sed -n '1,240p' src/main/automations/hermes-cron-output.ts
printf '%s\n' '--- orchestration database binding ---'
sed -n '1,220p' src/main/runtime/orchestration/db/orchestration-db.ts
printf '%s\n' '--- all tracked SQL files and references outside src ---'
git ls-files '*.sql' | head -80
rg -n -C 2 --glob '*.{ts,tsx,js,jsx,sql,json}' '\bREFERENCES\b|\bFOREIGN\s+KEY\b|foreign_keys' . --glob '!node_modules' --glob '!dist' --glob '!build' | head -160Repository: stablyai/orca
Length of output: 9472
Enable foreign keys in the Bun adapter. The Bun adapter does not preserve Node DatabaseSync's default foreign-key enforcement. A caller that writes FOREIGN KEY schemas can create orphan rows under Bun. Set PRAGMA foreign_keys = ON before application SQL and add an FK-violation integration test.
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 65-65: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/main/runtime/rpc/bun-websocket-transport.ts (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the connection cap instead of re-declaring it.
ws-transport.tsline 17 declares the sameMAX_WS_CONNECTIONS = 128. Two copies can drift, and the Bun and Node caps must stay identical. Export one constant and import it here.src/main/runtime/rpc/node-websocket-lifecycle.ts (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge the two
wstype imports.Static analysis reports
import/no-duplicatesfor modulews.♻️ Proposed fix
-import type { WebSocket } from 'ws' import type { Server as HttpServer } from 'node:http' -import type { WebSocketServer } from 'ws' +import type { WebSocket, WebSocketServer } from 'ws' import type { RemoteRuntimeServerHeartbeat } from './remote-runtime-server-heartbeat'Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8377893f-bd67-4dba-82b8-5b3716507d1e
📒 Files selected for processing (11)
config/scripts/orcad-bun-lifecycle-e2e.mjsconfig/scripts/orcad-bun-native-packages-smoke.mjsconfig/scripts/runtime-serve-terminal-smoke.mjspackage.jsonsrc/main/daemon/pty-subprocess/bun-pty-process.tssrc/main/daemon/pty-subprocess/native-pty-spawn.tssrc/main/orcad/orcad-health.tssrc/main/runtime/rpc/bun-websocket-transport.tssrc/main/runtime/rpc/node-websocket-lifecycle.tssrc/main/runtime/rpc/ws-transport.tssrc/main/server/serve-readiness.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| import type * as pty from 'node-pty' | ||
|
|
||
| type BunTerminal = { | ||
| closed: boolean | ||
| write(data: string | ArrayBufferView): number | ||
| resize(cols: number, rows: number): void | ||
| close(): void | ||
| } | ||
|
|
||
| type BunSubprocess = { | ||
| pid: number | ||
| terminal: BunTerminal | ||
| exited: Promise<number> | ||
| kill(signal?: string | number): void | ||
| } | ||
|
|
||
| type BunRuntime = { | ||
| spawn( | ||
| command: string[], | ||
| options: { | ||
| cwd: string | ||
| env: Record<string, string> | ||
| terminal: { | ||
| cols: number | ||
| rows: number | ||
| name: string | ||
| data(terminal: BunTerminal, data: Uint8Array<ArrayBuffer>): void | ||
| } | ||
| } | ||
| ): BunSubprocess | ||
| } | ||
|
|
||
| type BunGlobal = typeof globalThis & { Bun?: BunRuntime } | ||
|
|
||
| type BunPtyProcess = pty.IPty & { | ||
| destroy(): void | ||
| } | ||
|
|
||
| export function canUseBunPty(): boolean { | ||
| return typeof (globalThis as BunGlobal).Bun?.spawn === 'function' | ||
| } | ||
|
|
||
| export function spawnBunPty(args: { | ||
| file: string | ||
| args: string[] | ||
| cwd: string | ||
| env: Record<string, string> | ||
| cols: number | ||
| rows: number | ||
| }): BunPtyProcess { | ||
| const runtime = (globalThis as BunGlobal).Bun | ||
| if (!runtime) { | ||
| throw new Error('Bun runtime is unavailable') | ||
| } | ||
|
|
||
| let processHandle: BunSubprocess | ||
| let decodePending = '' | ||
| const dataListeners: ((data: string) => void)[] = [] | ||
| const exitListeners: ((event: { exitCode: number; signal?: number }) => void)[] = [] | ||
| const decoder = new TextDecoder() | ||
| let exited = false | ||
| let exitCode = 0 | ||
|
|
||
| const emitData = (data: string): void => { | ||
| for (const listener of dataListeners) { | ||
| listener(data) | ||
| } | ||
| } | ||
|
|
||
| const emitExit = (code: number): void => { | ||
| if (exited) { | ||
| return | ||
| } | ||
| exited = true | ||
| exitCode = code | ||
| const pending = decoder.decode() | ||
| if (pending) { | ||
| decodePending += pending | ||
| } | ||
| if (decodePending) { | ||
| emitData(decodePending) | ||
| decodePending = '' | ||
| } | ||
| for (const listener of exitListeners) { | ||
| listener({ exitCode: code }) | ||
| } | ||
| } | ||
|
|
||
| processHandle = runtime.spawn([args.file, ...args.args], { | ||
| cwd: args.cwd, | ||
| env: args.env, | ||
| terminal: { | ||
| cols: args.cols, | ||
| rows: args.rows, | ||
| name: args.env.TERM ?? 'xterm-256color', | ||
| data: (_terminal, data) => { | ||
| const decoded = decoder.decode(data, { stream: true }) | ||
| if (decoded) { | ||
| emitData(decoded) | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| void processHandle.exited.then(emitExit, () => emitExit(1)) | ||
|
|
||
| return { | ||
| pid: processHandle.pid, | ||
| process: args.file, | ||
| onData(listener) { | ||
| dataListeners.push(listener) | ||
| }, | ||
| onExit(listener) { | ||
| if (exited) { | ||
| listener({ exitCode }) | ||
| return | ||
| } | ||
| exitListeners.push(listener) | ||
| }, | ||
| write(data) { | ||
| if (!processHandle.terminal.closed) { | ||
| processHandle.terminal.write(data) | ||
| } | ||
| }, | ||
| resize(cols, rows) { | ||
| processHandle.terminal.resize(cols, rows) | ||
| }, | ||
| kill() { | ||
| processHandle.kill('SIGTERM') | ||
| }, | ||
| destroy() { | ||
| if (!processHandle.terminal.closed) { | ||
| try { | ||
| processHandle.kill(process.platform === 'win32' ? 'SIGTERM' : 'SIGHUP') | ||
| } catch { | ||
| // The subprocess may have exited between the close and destroy calls. | ||
| } | ||
| processHandle.terminal.close() | ||
| } | ||
| } | ||
| } as unknown as BunPtyProcess | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
fd -HI '.bun-version'
rg -n 'BUN_EXECUTABLE|bun@|"bun"|bun-version' package.json Dockerfile* config/scripts 2>/dev/nullRepository: stablyai/orca
Length of output: 390
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- candidate version/config files ---'
git ls-files | grep -E '(^|/)(\.bun-version|package\.json|bun\.lockb?|Dockerfile[^/]*|.*bun.*|.*container.*|.*workflow.*|.*ya?ml)$' | head -200
printf '%s\n' '--- repository guidance scopes ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- Bun references in tracked configuration/source ---'
rg -n -S --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' '(BUN_EXECUTABLE|bun-version|bun@[0-9]|Bun [0-9]|Bun\.Terminal|terminal:|\"packageManager\"|packageManager)' . | head -300Repository: stablyai/orca
Length of output: 44117
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- applicable guidance ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src-main-daemon.md
printf '%s\n' '--- Bun design document ---'
cat -n docs/design/orcad-bun-runtime.md
printf '%s\n' '--- Bun PTY proof ---'
cat -n config/scripts/orcad-bun-pty-poc.mjs
printf '%s\n' '--- Bun lifecycle script ---'
cat -n config/scripts/orcad-bun-lifecycle-e2e.mjs
printf '%s\n' '--- targeted setup/version references ---'
rg -n -S --glob '.github/**' --glob 'config/**' --glob 'docs/**' --glob 'package.json' --glob 'orca.yaml' \
'(setup-bun|oven-sh|bun-version|BUN_VERSION|BUN_EXECUTABLE|Bun 1\.|bun:|bun install|download.*bun|install.*bun|runtime.*bun|orcad)' . | head -300
printf '%s\n' '--- relevant learning ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings/src-main-pty.mdRepository: stablyai/orca
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -u
log=/tmp/coderabbit-shell-logs/shell-output-560lZy
printf '%s\n' '--- section markers in prior output ---'
rg -n -- '--- (applicable guidance|Bun design document|Bun PTY proof|Bun lifecycle script|targeted setup/version references|relevant learning) ---' "$log"
printf '%s\n' '--- exact Bun/version lines from prior output ---'
rg -n -S --max-count 120 '(Bun 1\.|Bun\.Terminal|setup-bun|oven-sh|bun-version|BUN_VERSION|BUN_EXECUTABLE|bun install|download.*bun|install.*bun|runtime.*bun|orcad)' "$log" | head -200
printf '%s\n' '--- file sizes ---'
wc -l docs/design/orcad-bun-runtime.md config/scripts/orcad-bun-pty-poc.mjs config/scripts/orcad-bun-lifecycle-e2e.mjs /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src-main-daemon.md /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings/src-main-pty.md
printf '%s\n' '--- bounded Bun proof ---'
cat -n config/scripts/orcad-bun-pty-poc.mjs
printf '%s\n' '--- bounded Bun design references ---'
rg -n -S -C 3 '(Bun|bun|version|PTY|terminal)' docs/design/orcad-bun-runtime.md | head -240
printf '%s\n' '--- bounded Bun lifecycle references ---'
rg -n -S -C 3 '(Bun|bun|version|PTY|terminal)' config/scripts/orcad-bun-lifecycle-e2e.mjs | head -240Repository: stablyai/orca
Length of output: 42822
Pin Bun to a validated version or check Bun.Terminal before selecting this path.
The repository’s PTY proof requires globalThis.Bun.Terminal, while canUseBunPty() checks only Bun.spawn. A runtime can therefore pass the capability check and reach Bun.spawn({ terminal }) without PTY support. No Bun version pin appears in the tracked configuration. Use the validated platform-specific Bun version, or add the Bun.Terminal capability check.
| return { | ||
| pid: processHandle.pid, | ||
| process: args.file, | ||
| onData(listener) { | ||
| dataListeners.push(listener) | ||
| }, | ||
| onExit(listener) { | ||
| if (exited) { | ||
| listener({ exitCode }) | ||
| return | ||
| } | ||
| exitListeners.push(listener) | ||
| }, | ||
| write(data) { | ||
| if (!processHandle.terminal.closed) { | ||
| processHandle.terminal.write(data) | ||
| } | ||
| }, | ||
| resize(cols, rows) { | ||
| processHandle.terminal.resize(cols, rows) | ||
| }, | ||
| kill() { | ||
| processHandle.kill('SIGTERM') | ||
| }, | ||
| destroy() { | ||
| if (!processHandle.terminal.closed) { | ||
| try { | ||
| processHandle.kill(process.platform === 'win32' ? 'SIGTERM' : 'SIGHUP') | ||
| } catch { | ||
| // The subprocess may have exited between the close and destroy calls. | ||
| } | ||
| processHandle.terminal.close() | ||
| } | ||
| } | ||
| } as unknown as BunPtyProcess |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -nP '\.pause\(\)|\.resume\(\)|handleFlowControl' --type=ts src/main -C3
rg -nP '\.onData\(|\.onExit\(' --type=ts src/main -A6 | rg -n 'dispose'
rg -nP '\.cols\b|\.rows\b' --type=ts src/main/daemon -C3
rg -nP '\.kill\(\s*[\x27"]' --type=ts src/main/daemon -C3Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- repository conventions and learnings ---'
head -5 /tmp/coderabbit-repo-knowledge/stablyai/orca-89dc44e4/*/*.md 2>/dev/null || true
printf '%s\n' '--- adapter ---'
cat -n src/main/daemon/pty-subprocess/bun-pty-process.ts | sed -n '1,180p'
printf '%s\n' '--- directly bound types and handle wrapper ---'
cat -n src/main/daemon/pty-subprocess.ts | sed -n '1,130p'
cat -n src/main/daemon/pty-subprocess/subprocess-handle.ts | sed -n '1,230p'
printf '%s\n' '--- resolved callers ---'
cat -n src/main/providers/local-pty-provider.ts | sed -n '1180,1260p'
rg -n 'createPtySubprocess|BunPtyProcess|ptyProcesses|onData\\(|onExit\\(|\\.kill\\(' src/main/daemon src/main/providers/local-pty-provider.ts -g '*.ts' -C2Repository: stablyai/orca
Length of output: 20125
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- Bun adapter bindings ---'
rg -n 'spawnBunPty|canUseBunPty|bun-pty-process|SpawnedDaemonPty|spawnNativeDaemonPty' src -g '*.ts' -C3
printf '%s\n' '--- native spawn implementation ---'
fd -i 'native-pty-spawn.ts' .
native=$(fd -i 'native-pty-spawn.ts' . | head -1)
[ -n "$native" ] && { ast-grep outline "$native"; cat -n "$native" | sed -n '1,260p'; }
printf '%s\n' '--- exact PTY size and subscription consumers ---'
rg -n 'getAppliedSize|proc\\.cols|proc\\.rows|onData\\([^)]*\\).*dispose|onExit\\([^)]*\\).*dispose|\\.kill\\([^)]' src/main/daemon src/main/providers -g '*.ts' -C3Repository: stablyai/orca
Length of output: 9023
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- scoped conventions and learnings ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print -exec sh -c 'echo "--- $1"; head -80 "$1"' _ {} \;
printf '%s\n' '--- Bun/native spawn result paths ---'
rg -n 'spawnBunPty|canUseBunPty|bun-pty-process|spawnNativeDaemonPty' src -g '*.ts' -C4Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- pty process map and Bun execution path ---'
rg -n 'const ptyProcesses|let ptyProcesses|ptyProcesses =|ptyProcesses\\.set|ptyProcesses\\.get|spawnAt\\(' src/main/providers/local-pty-provider.ts src/main/daemon/pty-subprocess -g '*.ts' -C4
printf '%s\n' '--- all relevant direct member calls ---'
rg -n -F 'proc.cols' src/main
rg -n -F 'proc.rows' src/main
rg -n -F 'proc.pause()' src/main
rg -n -F 'proc.resume()' src/main
rg -n -F 'proc.kill(' src/main
rg -n -F '.kill(' src/main/daemon/pty-subprocess src/main/providers/local-pty-provider.ts -C2
printf '%s\n' '--- node-pty dependency/type source ---'
rg -n '"node-pty"|interface IPty|kill\\(signal|onData\\(' package.json pnpm-lock.yaml yarn.lock package-lock.json node_modules/@types/node-pty node_modules/node-pty 2>/dev/null | head -120Repository: stablyai/orca
Length of output: 7815
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- local provider ownership and spawn assignment ---'
sed -n '1,380p' src/main/providers/local-pty-provider.ts
rg -n -F 'ptyProcesses' src/main/providers/local-pty-provider.ts -C3
rg -n -F 'spawnNativeDaemonPty' src/main/providers/local-pty-provider.ts src/main/daemon -C3
rg -n -F 'createPtySubprocess' src/main/providers/local-pty-provider.ts src/main/daemon -C3
printf '%s\n' '--- package contract references ---'
rg -n -F 'node-pty' package.json pnpm-lock.yaml pnpm-workspace.yaml 2>/dev/null | head -80
rg -n -F 'interface IPty' . -g '*.d.ts' -g '*.ts' 2>/dev/null | head -40Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- daemon handle contract and event disposal ---'
cat -n src/main/daemon/session-subprocess-handle.ts | sed -n '1,180p'
cat -n src/main/daemon/pty-subprocess/pre-listener-events.ts | sed -n '1,220p'
rg -n -F 'handle.pause' src/main/daemon -g '*.ts' -C3
rg -n -F 'handle.resume' src/main/daemon -g '*.ts' -C3
rg -n -F 'getPtyProcess' src/main -g '*.ts' -C3
printf '%s\n' '--- local provider spawn path ---'
sed -n '900,995p' src/main/providers/local-pty-provider.ts
rg -n -F 'pty.spawn' src/main/providers/local-pty-provider.ts -C4
printf '%s\n' '--- node-pty version ---'
rg -n -F 'node-pty' package.json pnpm-lock.yaml 2>/dev/null | head -60Repository: stablyai/orca
Length of output: 12137
🌐 Web query:
node-pty IPty interface cols rows handleFlowControl pause resume onData IDisposable kill signal
💡 Result:
The IPty interface in the node-pty library represents a pseudoterminal instance, providing methods and properties to interact with a spawned process [1][2]. Key properties and methods include: - cols and rows: Readonly properties indicating the current column and row dimensions of the pseudoterminal [1][2]. These can be updated using the resize(columns, rows) method [1][3][2]. - handleFlowControl: A boolean property that enables or disables automatic flow control [3][2][4]. When enabled, specific input strings (defaulting to XOFF '\x13' and XON '\x11') can pause and resume data transmission to the pseudoterminal [4][5]. These control characters are not passed to the underlying process when flow control is active [4][5]. - pause and resume: Methods used to manually pause or resume data processing for the pseudoterminal in conjunction with the flow control feature [3][2]. - onData: An event listener that fires when data is received from the pseudoterminal [1][2]. It returns an IDisposable [1][3]. - IDisposable: An interface defining a dispose method [3], which is used to clean up resources, such as stopping event listeners returned by onData [1][3]. - kill(signal?: string): A method to terminate the pseudoterminal process [3][2]. On Unix-like systems, a signal can be provided (defaulting to SIGHUP), though this parameter is not supported on Windows [3][6]. The library documentation and TypeScript definitions (found in the source repository's typings directory) serve as the primary authoritative reference for these APIs [1][7].
Citations:
- 1: https://github.com/microsoft/node-pty/blob/master/typings/node-pty.d.ts
- 2: https://deepwiki.com/microsoft/node-pty/1.2-api-reference
- 3: https://github.com/microsoft/node-pty/blob/d738123f/typings/node-pty.d.ts
- 4: https://github.com/microsoft/node-pty?tab=readme-ov-file
- 5: https://www.npmjs.com/package/node-pty
- 6: https://github.com/microsoft/node-pty/blob/d738123f/src/interfaces.ts
- 7: https://github.com/microsoft/node-pty
Complete the Bun PTY IPty adapter or expose a narrower type.
spawnNativeDaemonPty() returns spawnBunPty() as pty.IPty, but the adapter omits cols, rows, handleFlowControl, pause(), and resume(). Its onData() and onExit() methods return undefined instead of IDisposable. Its no-argument kill() sends SIGTERM, while IPty.kill() defaults to SIGHUP on POSIX. These mismatches can break consumers that use the value as an IPty.
| import type { WindowsShellSpawnAttempt } from '../../providers/windows-shell-fallback-chain' | ||
| import { assignHostProcessToKillOnCloseJob } from '../../windows/windows-pty-job' | ||
|
|
||
| import { canUseBunPty, spawnBunPty } from './bun-pty-process' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
rg -n 'canUseBunPty' --type=ts -C5
rg -n 'assignHostProcessToKillOnCloseJob' --type=ts -C5Repository: stablyai/orca
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- relevant repository conventions ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- candidate files ---'
fd -i 'native-pty-spawn|bun-pty-process|ssh-execution-boundary' .
printf '%s\n' '--- symbol locations ---'
rg -n -C5 'canUseBunPty|spawnBunPty|assignHostProcessToKillOnCloseJob' srcRepository: stablyai/orca
Length of output: 11625
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- daemon conventions ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src-main-daemon.md
printf '%s\n' '--- PTY learnings ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings/src-main-pty.md
printf '%s\n' '--- native PTY spawn ---'
cat -n src/main/daemon/pty-subprocess/native-pty-spawn.ts | sed -n '1,105p'
printf '%s\n' '--- Bun PTY implementation ---'
cat -n src/main/daemon/pty-subprocess/bun-pty-process.ts | sed -n '1,180p'
printf '%s\n' '--- Windows job contract ---'
cat -n src/main/windows/windows-pty-job.ts | sed -n '130,180p'Repository: stablyai/orca
Length of output: 12695
Guard the Bun PTY branch on Windows.
When canUseBunPty() is true on Windows, spawnAt returns spawnBunPty() before calling assignHostProcessToKillOnCloseJob(). This can leave PTY descendants running after an unexpected host exit. Add process.platform !== 'win32' to the condition.
| websocket: { | ||
| data: {} as Record<string, never>, | ||
| open: (socket) => this.handleOpen(socket), | ||
| message: (socket, message) => this.handleMessage(socket, message), | ||
| error: (socket, error) => { | ||
| this.adapters.get(socket)?.notify('error', error) | ||
| socket.terminate() | ||
| }, | ||
| close: (socket) => this.handleClose(socket) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🌐 Web query:
Bun.serve websocket handlers ping pong callback signature
💡 Result:
In Bun, you can handle WebSocket ping and pong events by adding ping and pong methods to the websocket handler object passed to Bun.serve() [1]. The signature for both handlers is: (ws: ServerWebSocket, data: Buffer) => void | Promise In this signature: - ws: The ServerWebSocket instance that received the ping or pong [2][1][3]. - data: A Buffer containing the data associated with the ping or pong [2][1][3]. Example usage: Bun.serve({ websocket: { //... other handlers like open, message, close ping(ws, data) { console.log("Received ping", data); }, pong(ws, data) { console.log("Received pong", data); }, }, fetch(req, server) { //... upgrade logic }, }); By default, Bun's WebSocket implementation automatically handles ping and pong responses if you do not explicitly define these handlers [1][4]. You can also control whether the server automatically sends pings using the sendPings option (which defaults to true) [1][4]. To manually send a ping or pong from the server, you can use the ws.ping(data) and ws.pong(data) methods available on the ServerWebSocket instance [2][3].
Citations:
- 1: https://bun.com/reference/bun/WebSocketHandler
- 2: https://bun.com/reference/bun/WebSocketHandler/ping
- 3: https://bun.sh/reference/bun/WebSocketHandler/pong
- 4: https://bun.com/docs/runtime/http/websockets
🏁 Script executed:
printf '%s\n' '--- applicable conventions ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- transport and heartbeat references ---'
sed -n '1,260p' src/main/runtime/rpc/bun-websocket-transport.ts
printf '%s\n' '--- heartbeat implementation ---'
rg -n -A30 -B12 'noteAlive|missedProbeLimit|heartbeat' src/main/runtime
printf '%s\n' '--- Node pong path and Bun version/type declarations ---'
rg -n -A12 -B12 'pong|WebSocketHandler|Bun\\.serve|bun' src/main package.json bun.lockb bun.lock 2>/dev/nullRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
printf '%s\n' '--- convention files ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print | sort
printf '%s\n' '--- convention headers ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -maxdepth 2 -type f -name '*.md' -print0 |
xargs -0 -r -n1 sh -c 'printf "\n### %s\n" "$0"; head -5 "$0"'
printf '%s\n' '--- Bun transport ---'
cat -n src/main/runtime/rpc/bun-websocket-transport.ts | sed -n '1,240p'
printf '%s\n' '--- heartbeat definitions ---'
rg -n -A24 -B8 'noteAlive|missedProbeLimit' src/main/runtime/rpc
printf '%s\n' '--- Node pong binding ---'
rg -n -A12 -B12 'on\(.pong.|pong' src/main/runtime/rpc/ws-transport.ts
printf '%s\n' '--- Bun dependency/version references ---'
rg -n -A3 -B3 '"bun"|"`@types/bun`"|bun-types|BunServerWebSocket|WebSocketHandler' package.json package-lock.json pnpm-lock.yaml yarn.lock bun.lockb 2>/dev/nullRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
printf '%s\n' '--- runtime-rpc learning ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings/src-main-runtime-rpc.md
printf '%s\n' '--- heartbeat source ---'
cat -n src/main/runtime/rpc/remote-runtime-server-heartbeat.ts | sed -n '1,100p'
printf '%s\n' '--- Bun transport remainder ---'
cat -n src/main/runtime/rpc/bun-websocket-transport.ts | sed -n '220,290p'
printf '%s\n' '--- package manifests and Bun references ---'
find . -maxdepth 3 -type f \( -name 'package.json' -o -name 'bun.lockb' -o -name 'bun.lock' -o -name 'pnpm-lock.yaml' -o -name 'yarn.lock' -o -name 'package-lock.json' \) -print
rg -n -A4 -B4 '"bun"|"`@types/bun`"|bun-types' --glob 'package.json' --glob 'bun.lock' --glob 'pnpm-lock.yaml' --glob 'yarn.lock' --glob 'package-lock.json' .Repository: stablyai/orca
Length of output: 7646
🏁 Script executed:
printf '%s\n' '--- package metadata ---'
cat package.json | sed -n '1,220p'
printf '%s\n' '--- Bun-related tracked files and configuration ---'
git ls-files | rg -i '(^|/)(bun|.*bun.*)|bun-version|BUN_VERSION'
printf '%s\n' '--- Bun transport tests and call sites ---'
rg -n -A18 -B8 'BunWebSocketTransport|canUseBunWebSocketTransport|globalThis.*Bun|serve\\(' src/main/runtime/rpc --glob '*.{ts,tsx}'
printf '%s\n' '--- exact Bun dependency entries in lockfile ---'
rg -n -A8 -B4 '(^|[ /])bun(@|:|$)|`@types/bun`|bun-types' pnpm-lock.yamlRepository: stablyai/orca
Length of output: 24010
Add a Bun pong handler for idle-client liveness.
Bun’s WebSocketHandler.pong callback receives (socket, data: Buffer). Add it to the local websocket contract and call this.options.heartbeat.noteAlive(adapter). Without it, an idle client can reach missedProbeLimit because only handleOpen and handleMessage mark the adapter alive.
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 (3)
src/main/runtime/rpc/bun-websocket-transport.ts (2)
7-10: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMap the adapter state from Bun’s
readyState.
ServerWebSocketexposesreadyState, notclosed. The current getter therefore reportsOPENafter closure, so delayed replies pass the guard and callsendon the closed socket.Source: MCP tools
16-20: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPass a boolean to
server.stop.Bun treats
trueas abrupt shutdown only when the argument is a boolean.{ closeActiveConnections: true }selects graceful shutdown and can delay the returned promise. Change the type and call tostop(true).Source: MCP tools
src/main/runtime/rpc/ws-transport.ts (1)
141-145: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External · Exploitability: Moderate
Preserve TLS when selecting Bun.
When
tlsCertandtlsKeyare set, pass them toBun.serve({ tls: { cert: tlsCert, key: tlsKey } }), or disable Bun for TLS configurations. Otherwise,start()exposes the secure endpoint without TLS.Source: MCP tools
🧹 Nitpick comments (1)
src/main/runtime/rpc/static-web-client-handler.ts (1)
19-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the static-file resolution logic.
createStaticWebClientResponseduplicateshandleStaticRequestfor path checks,stat, headers, and file validation. Extract a concrete function such asresolveStaticWebFileso the Bun and Node paths cannot drift.As per coding guidelines,
src/**/*.{ts,tsx}changes must extend an existing implementation instead of creating a parallel version.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 47089f2f-8034-4b1e-a8e6-51c730240a10
📒 Files selected for processing (7)
config/scripts/orcad-bun-lifecycle-e2e.mjsdocs/design/orcad-bun-runtime.mdsrc/main/global-fetch-call-site-audit.test.tssrc/main/runtime/rpc/bun-websocket-transport.tssrc/main/runtime/rpc/static-web-client-handler.tssrc/main/runtime/rpc/ws-transport.tssrc/shared/runtime-client-export-parity.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| The current implementation now has a Bun-native WebSocket transport and Bun.Terminal daemon | ||
| backend. A Node CLI can pair to Bun orcad; a Bun orcad can start and reattach its Bun daemon. | ||
|
|
||
| Results: | ||
|
|
||
| - Bun publishes readiness with a real WebSocket RPC endpoint. | ||
| - Node CLI pairing, repo RPC, worktree RPC, terminal creation, input, output, and scrollback work. | ||
| - Runtime restart preserves the daemon PID and terminal scrollback. | ||
| - `node:sqlite` is absent under Bun; the narrow synchronous adapter uses `bun:sqlite`. | ||
| - Static web-client requests are served through the Bun fetch path. | ||
| - A legacy Node daemon can still be adopted by Bun orcad; backend identity stays outside the daemon wire. | ||
|
|
||
| The POC does not claim production portability yet. Bun's missing read-side PTY pause/resume and | ||
| Windows process-job handles remain explicit platform gaps. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable repository guidance ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -type f -name '*.md' -print \
| sort \
| while read -r f; do
printf '\n### %s\n' "$f"
head -80 "$f"
done
printf '%s\n' '--- target document ---'
cat -n docs/design/orcad-bun-runtime.md | sed -n '1,140p'
printf '%s\n' '--- nearby proof and claim references ---'
rg -n -i --glob '!docs/design/orcad-bun-runtime.md' \
'Bun-native|restart|scrollback|terminal I/O|terminal creation|readiness|WebSocket RPC|daemon PID|control plane|bun:sqlite|PTTY' \
. | head -240Repository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- measured evidence and final claims ---'
cat -n docs/design/orcad-bun-runtime.md | sed -n '13,130p'
printf '%s\n' '--- changed files ---'
git status --short
git diff --stat
printf '%s\n' '--- Bun proof references in focused paths ---'
rg -n -i --glob '*.md' --glob '*.ts' --glob '*.js' --glob '*.json' \
'Bun-native|bun:sqlite|Bun.Terminal|scrollback|restart|daemon PID|control plane|terminal (I/O|creation|input|output)|readiness|WebSocket RPC' \
docs src tests package.json .github 2>/dev/null | head -260Repository: stablyai/orca
Length of output: 45919
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- proof scripts and their entrypoints ---'
fd -t f -i 'orcad-bun|runtime-serve-terminal|bun-native-packages' config/scripts src tests
printf '\n--- package smoke command ---\n'
sed -n '25,42p' package.json
for f in \
config/scripts/orcad-bun-lifecycle-e2e.mjs \
config/scripts/runtime-serve-terminal-smoke.mjs \
config/scripts/orcad-bun-pty-poc.mjs \
config/scripts/orcad-bun-native-packages-smoke.mjs
do
if [ -f "$f" ]; then
printf '\n### %s (%s lines)\n' "$f" "$(wc -l < "$f")"
ast-grep outline "$f"
fi
doneRepository: stablyai/orca
Length of output: 5471
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Bun lifecycle E2E ---'
cat -n config/scripts/orcad-bun-lifecycle-e2e.mjs
printf '%s\n' '--- terminal smoke: readiness, terminal operations, restart path ---'
cat -n config/scripts/runtime-serve-terminal-smoke.mjs | sed -n '1,230p'
cat -n config/scripts/runtime-serve-terminal-smoke.mjs | sed -n '231,417p'
printf '%s\n' '--- Bun PTY smoke ---'
cat -n config/scripts/orcad-bun-pty-poc.mjsRepository: stablyai/orca
Length of output: 29722
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- daemon adoption and migration proof references ---'
rg -n -i \
'legacy.*daemon|daemon.*legacy|adopt|migration|same-daemon|session scrollback|terminal scrollback' \
config/scripts src/main/orcad src/main/runtime src/orcad docs/design \
--glob '*.mjs' --glob '*.js' --glob '*.ts' --glob '*.md' | head -240
printf '%s\n' '--- all runtime smoke script invocations and Bun-related test names ---'
rg -n -i \
'orcad-bun-lifecycle-e2e|runtime-serve-terminal-smoke|BUN_EXECUTABLE|runtimeKind|ptyBackend|daemon-reattach|scrollback-replay' \
package.json config/scripts src tests --glob '*.mjs' --glob '*.js' --glob '*.ts' --glob '*.json' | head -240Repository: stablyai/orca
Length of output: 45325
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- orcad daemon supervision and runtime selection ---'
ast-grep outline src/main/orcad/orcad-daemon-supervision.ts
ast-grep outline src/main/orcad/orcad-entry.ts
cat -n src/main/orcad/orcad-daemon-supervision.ts | sed -n '1,220p'
cat -n src/main/orcad/orcad-entry.ts | sed -n '80,220p'Repository: stablyai/orca
Length of output: 12481
Limit the legacy-daemon migration claim. The Bun lifecycle proof covers Node CLI pairing, Bun terminal input/output, Bun orcad restart, daemon PID reuse, and scrollback replay. It does not start a legacy Node daemon and then adopt it from Bun orcad. Keep the Bun-native claims, but change lines 50 and 113 or add that cross-runtime proof.
| // `global.fetch` in any way (call, alias, fallback like `input.fetch ?? | ||
| // globalThis.fetch`). `typeof globalThis.fetch` type annotations are exempt. | ||
| const GLOBAL_FETCH_LINE = /(^|[^.\w])fetch\(|(?<!typeof )\bglobal(This)?\.fetch\b/ | ||
| const GLOBAL_FETCH_LINE = /(^|[^:.\w])fetch\(|(?<!typeof )\bglobal(This)?\.fetch\b/ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- scoped repository knowledge ---'
find /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4 -type f -name '*.md' -print
printf '%s\n' '--- target file outline ---'
ast-grep outline src/main/global-fetch-call-site-audit.test.ts
printf '%s\n' '--- target file ---'
cat -n src/main/global-fetch-call-site-audit.test.ts
printf '%s\n' '--- direct references ---'
rg -n -C 3 'GLOBAL_FETCH_LINE|global-fetch-call-site-audit|fetch\\(' src/mainRepository: stablyai/orca
Length of output: 9232
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- applicable conventions ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/src.md
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions/repo-wide.md
printf '%s\n' '--- relevant learning ---'
cat /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings/src-main.md
printf '%s\n' '--- regex behavior ---'
node - <<'JS'
const pattern = /(^|[^:.\w])fetch\(|(?<!typeof )\bglobal(This)?\.fetch\b/
const cases = [
['label:fetch()', true],
['condition?x:fetch()', true],
['fetch()', true],
[' x.fetch()', false],
['obj.fetch()', false],
['globalThis.fetch()', true],
['typeof globalThis.fetch', false],
]
for (const [source, expected] of cases) {
const actual = pattern.test(source)
console.log(JSON.stringify({source, actual, expected, pass: actual === expected}))
}
JSRepository: stablyai/orca
Length of output: 5454
Restore : as an allowed prefix for fetch(.
GLOBAL_FETCH_LINE excludes valid label:fetch() and condition?x:fetch() calls, so the audit can miss these global fetch call sites. Restore [^.\w] or add regression fixtures.
GitHub Actions
Failure
|
Too many files changed for review (1982 files, 100 file limit). |
8f4cb49 to
e066f88
Compare
nwparker
changed the title
296b386 to
a68b6f3
Compare
|
Rebased carefully onto main at Resolved the overlapping changes by carrying main's fixes into the extracted owners. Post-rebase checks caught and fixed the runtime identity guard, stale folder ownership snapshot, and RPC type-parity integration. Consolidated the duplicate argument parser and checked that all five relocated function bodies are byte-identical. Independent reviewers checked the affected ownership, persistence, relay, subscription, and RPC paths. The final five-commit main refresh was conflict-free, and range-diff shows all three branch commits unchanged through that refresh. Verification:
Limits: this was rebase verification, not a fresh full-suite or Windows/Linux hardware certification. Migration rollout remains default-off. The full-diff whitespace check still reports two blank context lines in the xterm source patch; ordinary integration-edit whitespace checks pass, and patch syntax was left intact. |
…release gate) Dormant seam for the Phase 3 port of #16741 (design D9.3): the shared contracts later T1/T2 slices build on. No production module imports any of it yet; only tests do. Existing behavior is unchanged: the call-queue hold check is a no-op until something calls holdIdleSelectors. Adds: - RelayWorkAdmission + relay-work-drain-contract: closes relay handler admission and waits for in-flight work, still admitting the cleanup requests/notifications (cancel, acks, unwatch) that let that work finish. - TransportPublicationDrain: counts local write completions against one transport and fails sticky once that transport is replaced. - pty-ownership-transfer-release-gate: exact-match opt-in (ORCA_ENABLE_PTY_OWNERSHIP_TRANSFER_MUTATION=1) that SSH core uses to keep live transfer off (D9.2). - RuntimeRpcCallQueuePool.holdIdleSelectors + RuntimeRpcCallQueueBusyError: atomically fence idle selectors while routing changes. Porting note (source: #16741 head a68b6f3, merge-base 277c289; T1 per the #24137 track manifest): - Taken verbatim: pty-ownership-transfer-release-gate(.test), runtime-rpc-call-queue delta (+ retirement test), relay-work-admission API and behavior, transport-publication-drain API and behavior. - Adapted: relay-work-drain-contract omits relay.reset, relay.recoverPreparedReset (T3) and relay.networkTunnel.close/.frame (T4); those contracts do not exist on this base and join the sets when their tracks land. Promise.withResolvers replaced with plain promises in relay-reachable code, because the legacy relay bundle still targets node18 hosts. RequestContext.transportGeneration is not added here (it belongs with the dispatcher slice), so the admission test drops it. Added transport-publication-drain.test.ts, since #16741 covered it only through relay-producer-publication-drain. - Dropped: the pty-source-credit-contract/-validation/-record/ledger-test deltas. They only carry the ownershipTransfer output envelope, which is live-transfer engine (T7, deferred per D9.2), so the credit contract on this base is already the seam T2 needs. The post-slice assertPtySourceSpan re-validation came with that envelope and is dropped with it. - Not in this slice (next T1 slices, which are also the first consumers): dispatcher wiring (dispatcher-client-state / rpc-routing / producer-transport / notification-publication + RequestContext.transportGeneration) that constructs RelayWorkAdmission; relay-producer-publication-drain on TransportPublicationDrain; fs/git stream shutdown; ws-transport + node-websocket-lifecycle + websocket-transport-limits; renderer flow-controller deltas. Later consumers: T2 ssh-pty-preparation-admission (drain contract), ssh-pty-source-preparation-drain (release gate); T4 tunnels (publication drain); T5 runtime-environment-call-queue (selector hold). No wire change: no new RPC fields, methods, or opcodes.
First consumer of the T1 work-drain seam: every request and notification handler registered on the relay dispatcher now runs under RelayWorkAdmission, and the dispatcher exposes beginWorkDrain(exclude?) and assertActiveWorkContext(context). RequestContext gains an optional transportGeneration, stamped from client.generation for both requests and notifications, so later producers can pin publication to one transport. Behavior today is unchanged: nothing in production calls beginWorkDrain yet (the shutdown/reset caller is T3), so admission never closes. Decision: pty.cancelDelivery joins the drain request allowlist. #16741 omits it. It only retires an existing source delivery (closes the ledger record, releases retained spans, wakes the publication so it drops its record), it cannot open a delivery or produce output, and a retry replays the remembered proof instead of mutating again. It is the request-shaped sibling of pty.ackData / pty.setDeliveryPaused, which are already allowlisted. Refusing it during a drain would make the client's cancellation proof fail (ssh_source_cancellation_proof_invalid) and leave the delivery retained until detach, turning a provable cancel into an unverifiable one. Revisit only if live ownership transfer (T7, deferred) needs a frozen source ledger. Porting note (source: #16741 head a68b6f3, merge-base 277c289; T1 per the track manifest): - Taken verbatim: dispatcher-client-state onRequest/onNotification wrapping, beginWorkDrain, assertActiveWorkContext; dispatcher-contract transportGeneration; dispatcher-rpc-routing generation stamping. - Adapted: RelayWorkAdmission.run is no longer an async function. It returns the handler's own promise and tracks completion on the side, because an async wrapper adds microtask turns to every relay response and broke existing tests on this base that await one turn after feed() (workspace-session-handler, pty-handler ownership/retired-surface). Refusal and sync throws still surface as rejections. - dispatcher-work-drain.test: kept the four dispatcher-only cases; replaced the two RelayGraceLifecycle.prepareShutdown cases with dispatcher-level equivalents (initiator exclusion, copied/settled context rejection); dropped the real-shutdown case (prepareShutdown/finishShutdown is T3). Added cases for transportGeneration stamping and pty.cancelDelivery during drain. Wire compatibility: no new RPC fields, methods, or opcodes on the wire. transportGeneration is relay-internal. Once a later track calls beginWorkDrain, a refused request gets an ordinary JSON-RPC error (-32000 relay_work_admission_closed) and a refused notification is dropped, which old and new clients already handle as a failed call.
…cation drain Second T1 consumer slice: relay producer publication can now be proven, not just enqueued. publishProducerNotification takes optional onSettled, settledTransportGeneration and isStillAdmitted, and settles exactly once on every path (sink completion, disposal, missing client, PTY admission refusal, capacity refusal, serialization failure) while keeping its existing boolean return and throw contract. assertSettledProducerTransport refuses a replaced or closed client generation and a sink without write callbacks. RelayProducerPublicationDrain builds on the landed TransportPublicationDrain: each publish is counted until its local write completes, and the drain fails sticky once the owner's authority or the transport generation changes, even for frames already queued behind backpressure (isStillAdmitted is rechecked at dequeue through the frame codec and producer transport). Existing producers (watcher events, agent hook envelopes, workspace snapshots) pass none of the new options and behave as before. The drain has no production caller yet; the browser network tunnel (T4) is its first. Porting note (source: #16741 head a68b6f3, merge-base 277c289; T1 per the track manifest): - Taken verbatim: dispatcher-client-writer supportsWriteCallback, dispatcher-frame-codec publicationAdmission, dispatcher-producer-transport isStillAdmitted plumbing, dispatcher-notification-publication settlement, relay-producer-publication-drain, and both tests (relay-producer-publication-drain.test, dispatcher-producer-settlement.test). These files were unchanged on this base since the merge-base, so the upstream hunks applied cleanly. - Adapted: nothing needed; the new code uses no Promise.withResolvers or Bun APIs, so it stays within the relay bundle's Node 18 floor. - Not in this slice: fs/git stream shutdown, ws-transport / node-websocket-lifecycle / websocket-transport-limits, renderer flow-controller deltas (later T1 slices). Wire compatibility: no new RPC fields, methods, or opcodes. Settlement, generation checks and admission are relay-local; a client of any version sees the same notification frames it does today, and a frame refused by the new checks is simply not sent, as a capacity refusal already is.
Third T1 slice of the #16741 port: the relay's two detached stream producers can now be fenced and awaited, not just flagged. RelayStreamRegistry (fs.readFileStream): - disposeAll permanently fences register/beginOperation (relay_file_stream_shutdown_fenced), waits for every close and for every admitted operation, and rejects with relay_file_stream_shutdown_incomplete while any handle is still unclosed, so a later disposeAll retries only the failed handles. - release coalesces concurrent callers onto one close, tolerates only EBADF as already-closed, and keeps a failed-close entry (aborted) for retry. - beginOperation / releaseUnregisteredHandle let work that opens a handle before registration be drained too (wired in the next commit). GitResponseStreamRegistry: disposeAll fences startStream (relay_response_stream_shutdown_fenced); disposeAllAndWait also awaits every scheduled or in-flight pump, and an aborted pump no longer publishes git.responseEnd after its final chunk write settles. reserveTerminalFrameSlot moves unchanged into fs-stream-terminal-frame-slots and now holds a registry operation per slot, so shutdown also waits for undelivered fs.streamEnd/fs.streamError frames. No wire change: no new methods, fields, or opcodes. Mixed versions are unaffected; a fenced relay answers with an ordinary request error. Porting note (source: #16741 head a68b6f3, merge-base 277c289; T1 per the track manifest): - Taken verbatim: fs-stream-registry delta, git-response-stream delta, fs-stream-registry-shutdown.test, git-response-stream-shutdown.test. - Adapted: Promise.withResolvers replaced with plain promises (the relay bundle targets node18). fs-stream-terminal-frame-slots is extracted from the reserveTerminalFrameSlot that main already has inline in fs-handler-file-read (keeping its rationale comment) rather than added as a second copy. The EBADF check uses a type guard instead of a cast, and the tests' stub casts carry SAFETY rationales. - Dropped: nothing.
…processes Wires the stream registries into relay shutdown. The dispatcher's RelayWorkAdmission drain (#24181) ends once fs.readFileStream answers its metadata or a handler answers a git response-stream sentinel; the pumps and file descriptors behind those answers are detached producers it does not see. RelayRuntimeServices.disposeOwnedProcesses now also awaits GitResponseStreamRegistry.disposeAllAndWait and FsHandler.disposeFileStreams, and rejects with relay_owned_process_shutdown_incomplete if either fails, so the grace lifecycle defers shutdown (its existing catch path) instead of exiting with an unclosed fd; the next attempt retries only failed handles. fs.readFileStream now holds a registry operation across metadata preparation (including the binary-prefix probe handle) and across each pump, closes pre-registration handles through releaseUnregisteredHandle, releases its terminal-frame slot even when the handle close fails, and publishes no terminal frame for a stream that was aborted or went stale while its last read was in flight. Behavior today: the grace/SIGTERM shutdown path already calls disposeOwnedProcesses after PTY disposal, so a shutdown now also waits for in-flight stream pumps and queued terminal frames (which settle on client close or dispatcher disposal). After disposal, stream requests are refused with a fenced error; if shutdown is deferred by a failed close, the relay stays up with streams fenced, matching #16741's one-way drain. No wire change. Porting note (source: #16741 head a68b6f3, merge-base 277c289; these files are T2 in the track manifest, taken here only as far as the stream drain needs them): - Taken verbatim: fs-handler-file-read delta, fs-handler-utils isBinaryFilePrefix releaseHandle parameter, fs-handler disposeFileStreams. - Adapted: relay-runtime-services takes only the responseStreams field and the response/file-stream drains in disposeOwnedProcesses. Skill-upload and AI Vault cleanup stay log-and-continue rather than joining the aggregate failure. Added a fs-handler-stream test proving disposeFileStreams waits for an undelivered terminal frame and fences new reads. - Dropped (left for T2): agentExecHandler disposal, disposeWatchers, the ownership-transfer adapter/capture wiring, and the relay-grace-lifecycle beginWorkDrain -> disposeOwnedProcesses -> beginWorkDrain sequence.
disposeOwnedProcesses now waits for the git response-stream and file-stream drains and rejects with relay_owned_process_shutdown_incomplete when either fails, but nothing exercised that path: relay-daemon tests stub the whole runtime. These tests pin that shutdown is not acknowledged before either drain settles, that both drain failures are aggregated after every owner was attempted (and a retry succeeds), and that skill/AI Vault failures stay log-and-continue in this slice. Porting note (source: #16741 head a68b6f3, src/relay/relay-runtime-owned-process-shutdown.test.ts, T2 in the track manifest): - Taken: the prototype-stub fixture, the "does not acknowledge cleanup before %s have settled" case (responses, fileStreams), and the no-vault-service case. - Adapted: Promise.withResolvers replaced with a plain promise; the fixture cast carries a SAFETY rationale; the retry case targets the stream drains instead of skill/vault, and a new case pins skill/vault as log-and-continue to match this slice's adaptation. - Dropped (left for T2): agentExecHandler and disposeWatchers cases, and skill/vault aggregate-failure cases.
…-port bind mode Port the #16741 T1 transport pieces onto the current Node-only transport. The per-socket lifecycle (pre-auth timer, heartbeat enrolment, finalize on close/error), over-capacity rejection and shutdown move out of ws-transport.ts into node-websocket-lifecycle.ts, and the size/connection caps into websocket-transport-limits.ts. WebSocketTransport gains an opt-in strictPort that binds only the configured port and fails closed instead of walking the fallback ladder; OrcaRuntimeRpcServer exposes it as requirePinnedWsPort, which also skips the persisted fallback port and stops the Unix socket transport before rethrowing, so a caller whose tunnel forwards one fixed port never runs on a port nobody can reach. Porting note (#16741 a68b6f3, T1): - Taken: node-websocket-lifecycle.ts, websocket-transport-limits.ts, WebSocketTransport strictPort, the ws-transport strict-port test, runtime-rpc-state/-pairing-types/-lifecycle requirePinnedWsPort. - Adapted: the lifecycle reads the message/close handlers through getters (the PR snapshotted them per connection) so a handler registered after start still reaches earlier sockets; binary frames convert through a checked RawData branch instead of `as Buffer`; the original Why comments move with the code; stop() keeps a single Node path (no AggregateError) and keeps the PR's closeAllConnections(), now covered by a test that an unanswered HTTP request no longer holds stop() open; the over-capacity test calls the extracted function instead of casting to a private. - Dropped: every bun-websocket-* file and the Bun branches in ws-transport.ts (D9.4); WEBSOCKET_TRANSPORT_MAX_BACKPRESSURE_BYTES (only the Bun transport read it); the relaxed EACCES `port`-field check in isPortListenFallbackError (it covered Bun's listen errors, Node's carry the port). - Not here: the orcad-entry wiring that sets requirePinnedWsPort for `--port` (T6, managed SSH deploy); the transportGeneration context plumbing (its only readers are T3/T7); holdIdleRuntimeEnvironmentCalls (its only caller is the T5 reconciliation coordinator). Wire compatibility: none. The bind policy is server-local, the option defaults off, and no RPC field, method or frame changes.
…ged server (#16741 T6-9) The destination half of a catalog migration: an orcad stages a T6-7 manifest (repositories, project groups, folder workspaces, dormant session, client, automation and worktree metadata, retired names, scrollback snapshots) with exclusive claims, then commits it with a receipt so a retried commit returns the same receipt and never imports twice. Served as orcad.migration.* runtime RPC behind the orcad.migration-catalog.v1 capability; the client refuses a host without it or with method-not-found, and any other failure is left for the caller to recheck. Dormant only: no live PTY projection. Inert on the desktop until T6-10. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…16741 T6-8) (#24519) Read-only export of a direct-SSH target's catalog and dormant state into the signed T6-7 manifest: repositories, folder workspaces and their project groups, worktree metadata and lineage, sparse presets, retired worktree names, the workspace session with bounded scrollback snapshots, automations, and client routing. Reads go through the profile-state Store via a read-only OrcadSourceExportPersistence domain; nothing retires the source. Adds the export-aware migration preflight on top of T6-5's dependents census, a resumable snapshot transfer driver with injected destination operations, and destination-side chunk staging keyed to a caller-supplied staged manifest. Lands the P7/P9 holds: session-owner projection hooks, syncDirectoryDurablySync and the durable-write mode, scrollback path and stored-bytes exports, retained refs, and dormant-tab buffer preservation. Inert until T6-10. Co-authored-by: m4air <m4air@Mac.localdomain>
…ged server (#16741 T6-9) The destination half of a catalog migration: an orcad stages a T6-7 manifest (repositories, project groups, folder workspaces, dormant session, client, automation and worktree metadata, retired names, scrollback snapshots) with exclusive claims, then commits it with a receipt so a retried commit returns the same receipt and never imports twice. Served as orcad.migration.* runtime RPC behind the orcad.migration-catalog.v1 capability; the client refuses a host without it or with method-not-found, and any other failure is left for the caller to recheck. Dormant only: no live PTY projection. Inert on the desktop until T6-10. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… on proven terminal exit (#16741 T8-c1+c2) A migration from a relay-hosted SSH target into a managed orcad now starts with a journal in its own sidecar directory, then the target's managed-owner fence, then a profile flush, before any remote call. A fence with no journal is unverifiable and never released; a journal whose fence is gone is stale and grants nothing; an unreadable journal fails closed. The fence requires every terminal the target ever leased to be proven exited, checked before the fence (with the relay's process list) and again under it. Same-owner claims now need the durable record that explains them. Inert until T8-c4/T6-10. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
* Revert "feat(orcad): source-side dormant export of a relay-hosted SSH target (#16741 T6-8) (#24519)" This reverts commit 783101b. * Revert "feat(ssh): update, roll back, recover and stop a managed orcad server (#16741 T6-5 follow-up) (#24463)" This reverts commit 38c2d1d. * Revert "feat(ssh): deploy and pair an empty managed orcad server over SSH (#16741 T6-5) (#24453)" This reverts commit 8b76683. * Revert "fix(ssh): orcad GC honors the activation journal; readiness requires proven daemon coverage (#16741 T6 follow-up) (#24451)" This reverts commit d3f8c50. * Revert "feat(ssh): remote orcad stop by request file and journaled decommission (#16741 T6-4) (#24449)" This reverts commit 43d9b43. * Revert "feat(orcad): supervisable server: stop requests, managed stop receipts and a lifetime that keeps its lock on failed teardown (#16741 T6-3) (#24433)" This reverts commit b093d3a. * Revert "feat(ssh): crash-safe orcad activation, rollback and recovery (#16741 T6-2) (#24423)" This reverts commit 1a9ac0e. * Revert "feat(runtime): SSH access links for paired servers in a downgrade-safe sidecar (#16741 T5-1+T5-2) (#24420)" This reverts commit 99db2bf. * Revert "feat(relay): capability-gated owner reset with a durable preparation journal (#16741 T3 R1) (#24418)" This reverts commit 34a582b. * Revert "feat(ssh): track connection-manager drains, test probes and provider continuations (#16741 T2 P3+P8a) (#24407)" This reverts commit d53063d. * Revert "feat(daemon): idle retirement, session census and recovery-only provider (#16741 T2 P4b) (#24409)" This reverts commit ff212db. * Revert "feat(ssh): add pty.resumeClient and split SSH PTY process listing (#16741 T2 P5+P6) (#24414)" This reverts commit 92cb717. * Revert "feat(relay): await owned watcher and agent children on shutdown (#16741 T2 P1) (#24400)" This reverts commit 6b36e4f. * Revert "feat(session): retry failed renderer session writes and verify local folder PTYs (#16741 T2 P9) (#24406)" This reverts commit d23ecef. * Revert "feat(ssh): remote orcad primitives on the pinned Node runtime (#16741 T6-1) (#24419)" This reverts commit dd87ae5. * Revert "fix(runtime): fence runtime-environment subscriptions and status probes by identity (#16741 T5-3) (#24421)" This reverts commit ece9e4d. * Revert "feat(orcad): migration manifest and dormant-state contracts (#16741 T6-7) (#24422)" This reverts commit 3fbdaba. * Revert "feat(ssh): wire SshConnection through the work and transport close ledgers (#16741 T2 P2) (#24401)" This reverts commit 4e8edc8. * Revert "feat(profiles): carry markdown frontmatter visibility in project transfers (#16741 T2 P7) (#24405)" This reverts commit 60c9326. * Revert "fix(runtime): project the PTY incarnation onto mobile session tabs (#24413)" This reverts commit 99e0303. * Revert "feat(daemon): tag daemon stream data with the PTY incarnation id (#16741 T2 P4a) (#24402)" This reverts commit 817af76. * Revert "feat(ssh): port the SSH connection work ledger and transport close ledger (#16741 T2) (#24210)" This reverts commit c991893. * Revert "feat(relay): fence and drain file and git response streams on shutdown (#24185)" This reverts commit dc08ffe. * Revert "refactor(runtime-rpc): extract the Node WebSocket lifecycle; opt-in pinned port (#24186)" This reverts commit a789233. * Revert "feat(relay): route relay handlers through work admission; producer publication drain (#24181)" This reverts commit 0b812bd. * Revert "feat(relay): land the #16741 T1 seam (work drain, publication drain, release gate) (#24156)" This reverts commit 3aa2d3a. --------- Co-authored-by: m4air <m4air@Mac.localdomain>
2 participants
Elegance pass
0.1.0+9671e8245ea2passed readiness, repo/worktree RPC, PTY output, daemon survival/reattachment, scrollback replay, and new input after restart.Summary
Consolidates the accumulated Bun-orcad and SSH/Remote convergence implementation, rebased onto
mainatef6eeab26e4.live,unverifiable, andexited.Rebase reconciliation
Fresh validation during this rebase
oxlint: passed (excluded only untracked generated.build/**scratch files).0.1.0+8f2b80f8ae61: Bun 1.4.0 included, 4,527 modules, 8.40 MB, zero Electron ornode:sqliteimports.Tests used background launch and mock-Keychain isolation flags. No real-host upgrade or installer rollout was performed as part of this rebase.
Still outside this validation / remaining release gates