Repository navigation
Conversation
A synchronous fs syscall on the Electron main thread parks the whole app when its target is on a stalled mount (SMB/NFS, VPN drop, sleeping NAS, unmounted drive). The app stops repainting and stops answering Force Quit. This converts 28 such paths, covering the ones users actually hit: - orca.yaml hook config, read behind the POLLED git:status path, so a stalled repo froze the app on a timer with no user action - agent hook status/install for 14 agents, incl. the shared reader that 10 of them funnel through and a serial install loop with sync fs per body - credential status for Codex/Grok/MiniMax/speech, also called off-IPC by RateLimitService on a refresh timer -- froze a fully idle app - external path authorization and agent trust setup - profile index, keybindings, diagnostics discovery Measured first, since it shapes the fix: libuv's threadpool defaults to 4 threads, and at exactly poolSize concurrent stalled ops every other async fs call app-wide stops progressing. But the event loop stayed alive in all configurations (max timer gap 4ms). So async conversion does not bound a hung mount -- it keeps the main thread responsive, which is the reported symptom. UV_THREADPOOL_SIZE is raised to 16 as headroom, not as a bound. No timeout machinery was added; it cannot bound a block on its own thread. Where a sync twin had to stay (CLI processes, pre-window startup, callers that cannot await), the write path keeps single-thread serialization rather than relying on a promise chain a sync writer walks straight through: canonicalization moves off-thread while the read-modify-write stays sync. Where both twins must coexist, the async writer publishes its temp path and the sync writer unlinks it, turning the parked rename into a swallowed ENOENT -- a generation guard alone is spent once the writer parks. observability/local-file-sink.ts is deliberately untouched; its writeSync is a crash-durability contract that needs a process boundary, not an await.
📝 WalkthroughWalkthroughThis change moves main-process filesystem work to asynchronous APIs across agent hooks, trust presets, account stores, secure files, keybindings, diagnostics, worktrees, and profile indexes. Atomic writes now use temporary files, renames, cleanup, and per-path serialization. IPC handlers await asynchronous service operations. Hook services retain status and mutation behavior while adding asynchronous workflows. New tests cover event-loop responsiveness, stalled operations, race handling, atomicity, and cache consistency. A libuv threadpool probe and startup sizing module were also added. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
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}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (17)
src/main/orca-profiles/profile-index-async-store.ts (1)
195-213: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
copyIfPresentAsynchides real migration failures.The
catchblock treats every error the same. A missing source is expected.EACCES,ENOSPC, or a stalled-mount error oncopyFileis not. Both outcomes leave no trace, so a partial legacy migration is silent.Log non-
ENOENTerrors so the failure is diagnosable.♻️ Proposed change
- } catch { + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + console.warn('[orca-profiles] legacy migration copy failed:', target, error) + } await unlink(tmpTarget).catch(() => {}) }src/main/ipc/orca-profiles-main-thread-fs.test.ts (2)
64-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPatch the
defaultexport in both fs mock factories.Both factories return
{ ...actual }with the named functions replaced. Neither replacesactual.default. A module that usesimport fs from 'node:fs'and callsfs.mkdirSync(...)therefore reaches the real function and records nothing. Theexpect(syncFsCalls).toEqual([])assertions would then pass without proving anything.The current production code uses named imports, so the guard holds today. Patch
defaultso a future default import cannot silently disable this regression test.profile-index-veto-retry-integrity.test.tson Line 27 already does this.♻️ Proposed change
for (const name of RECORDED_SYNC) { const real = actual[name] as (...args: unknown[]) => unknown patched[name] = (...args: unknown[]) => { syncFsCalls.push(`${name} ${String(args[0])}`) return real(...args) } } - return patched + return { ...patched, default: patched } })Apply the same change to the
node:fs/promisesfactory.
256-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe parked promise never settles and has no rejection handler.
gate.blockis a promise that never resolves.parkedtherefore stays pending for the rest of the run.void parked.then(...)attaches no rejection handler. If the operation later rejects, the unhandled rejection surfaces in an unrelated test.Attach a no-op rejection handler.
♻️ Proposed change
- void parked.then(() => { - settled = true - }) + void parked.then( + () => { + settled = true + }, + () => { + settled = true + } + )src/main/copilot/hook-service.ts (1)
310-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAsync twins were added by copying the sync method body instead of extracting the pure mutation. In both files the new async method repeats the config-mutation logic of its sync counterpart, and only the read and write calls differ. Any future change to the event sweep must be applied twice, so the CLI process and the main process can diverge.
src/main/gemini/hook-service.tsshows the target pattern in this same PR:applyManagedHooksandstripManagedHookshold the pure logic, and the sync and async methods differ by three lines.
src/main/copilot/hook-service.ts#L310-L349: extract the 30-line hook-rebuild block shared withinstallinto abuildInstalledCopilotConfig(config, scriptPath)helper, and call it from both methods.src/main/droid/hook-service.ts#L291-L315: extract the managed-command sweep shared withremoveinto astripManagedDroidHooks(config)helper, matching the existingbuildInstalledDroidConfigprecedent in the same file.src/main/agent-hooks/managed-agent-hook-controls.ts (1)
176-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the threadpool size stated in the comment.
The comment states the libuv threadpool has 4 threads. This PR sets
UV_THREADPOOL_SIZEto 16 at startup, so the stated number is stale. The serial choice still holds; only the justification number is wrong.♻️ Proposed comment update
-// Why: serial, not Promise.all — libuv's threadpool is 4 threads, so fanning 14 -// agent configs out at once just starves every other async fs caller. +// Why: serial, not Promise.all — the libuv threadpool is bounded, so fanning 14 +// agent configs out at once starves every other async fs caller.src/main/cursor/hook-service.ts (1)
176-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared merge and sweep logic like the sibling services do.
install()(Lines 119-174) andinstallAsync()(Lines 176-231) hold the same 45 lines of stale-hook sweeping, dual-shape stripping, managed-entry replacement, andversionpinning.remove()andremoveAsync()duplicate the strip loop. Only the read and write calls differ. A future rule change must be applied twice, so the CLI sync path and the app async path can diverge silently.
src/main/command-code/hook-service.tsandsrc/main/antigravity/hook-service.tsalready solve this with sharedbuildInstalledConfigandstripManagedHookshelpers. Apply the same shape here.♻️ Proposed extraction
+function buildInstalledConfig(config: HooksConfig, scriptPath: string): Record<string, unknown> { + const command = getManagedCommand(scriptPath) + const nextHooks = { ...config.hooks } + const managedEvents = new Set<string>(CURSOR_EVENTS) + const isManagedCommand = createManagedCommandMatcher(getManagedScriptFileName()) + + for (const [eventName, definitions] of Object.entries(nextHooks)) { + if (managedEvents.has(eventName) || !Array.isArray(definitions)) { + continue + } + const cleaned = removeManagedCommands(definitions, isManagedCommand).filter( + (definition) => !isManagedCommand(definition.command as string | undefined) + ) + if (cleaned.length === 0) { + delete nextHooks[eventName] + } else { + nextHooks[eventName] = cleaned + } + } + + for (const eventName of CURSOR_EVENTS) { + const current = Array.isArray(nextHooks[eventName]) ? nextHooks[eventName] : [] + const cleaned = removeManagedCommands(current, isManagedCommand).filter( + (definition) => !isManagedCommand(definition.command as string | undefined) + ) + nextHooks[eventName] = [...cleaned, buildManagedCommandDefinition(command)] + } + + const nextConfig: Record<string, unknown> = { ...config, hooks: nextHooks } + if (nextConfig.version === undefined) { + nextConfig.version = 1 + } + return nextConfig +}Then both twins reduce to the read, the helper call, the writes, and the status read:
async installAsync(): Promise<AgentHookInstallStatus> { const configPath = getConfigPath() const scriptPath = getManagedScriptPath() const config = await readHooksJsonAsync(configPath) if (!config) { return parseErrorStatus(configPath) } - // ...45 duplicated lines... + const nextConfig = buildInstalledConfig(config, scriptPath) await writeManagedScriptAsync(scriptPath, getManagedScript()) await writeHooksJsonAsync(configPath, nextConfig) return this.getStatusAsync() }Also applies to: 318-343
src/main/grok/managed-hook-script.ts (1)
11-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the string replacement against silent drift.
WINDOWS_HOOK_PAYLOAD_FORM_LINEduplicates the final line produced bybuildWindowsAgentHookPostCommandinsrc/main/agent-hooks/installer-utils.ts. If that shared builder changes the payload line (spacing, redirection, or the^continuation),String.prototype.replacematches nothing and returns the command unchanged. The generated Windows script then posts withoutgrokHome, and no error is raised.Add a module-load assertion so the mismatch fails fast instead of silently degrading the hook payload.
♻️ Proposed guard
-const WINDOWS_GROK_HOOK_POST_COMMAND = buildWindowsAgentHookPostCommand('grok').replace( - WINDOWS_HOOK_PAYLOAD_FORM_LINE, - ` --data-urlencode "grokHome=%ORCA_GROK_HOME%" ^\r\n${WINDOWS_HOOK_PAYLOAD_FORM_LINE}` -) +const WINDOWS_GROK_HOOK_POST_COMMAND = ((): string => { + const base = buildWindowsAgentHookPostCommand('grok') + if (!base.includes(WINDOWS_HOOK_PAYLOAD_FORM_LINE)) { + throw new Error('Grok Windows hook command no longer contains the expected payload line') + } + return base.replace( + WINDOWS_HOOK_PAYLOAD_FORM_LINE, + ` --data-urlencode "grokHome=%ORCA_GROK_HOME%" ^\r\n${WINDOWS_HOOK_PAYLOAD_FORM_LINE}` + ) +})()src/main/orca-yaml-hooks-main-thread-block.test.ts (1)
141-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact paths passed to
readFileMock.The mock returns the shared YAML body for any path other than
ISSUE_COMMAND. A regression that reads the wrong shared path would still satisfy this test. Pin both read targets so the test detects a path change.♻️ Proposed assertion
expect(syncCalls).toEqual([]) + expect(readFileMock.mock.calls.map((call) => call[0])).toEqual([ISSUE_COMMAND, YAML]) })src/main/grok/hook-service.ts (1)
310-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the managed-hook sweep shared by
removeandremoveAsync.Lines 310-322 duplicate Lines 284-296 exactly. Only the read and the write differ between the two methods.
buildInstalledConfigalready follows the shared-mutation pattern for the install path; apply the same pattern here so a future change to the sweep cannot diverge between the synchronous and asynchronous methods.♻️ Proposed extraction
Add the shared helper next to
buildInstalledConfig:function buildRemovedConfig(config: HooksConfig): void { const nextHooks = { ...config.hooks } const isManagedCommand = createManagedCommandMatcher(getManagedScriptFileName()) for (const [eventName, definitions] of Object.entries(nextHooks)) { if (!Array.isArray(definitions)) { continue } const cleaned = removeManagedCommands(definitions, isManagedCommand) if (cleaned.length === 0) { delete nextHooks[eventName] } else { nextHooks[eventName] = cleaned } } config.hooks = nextHooks }Then reduce
removeAsync:- const nextHooks = { ...config.hooks } - const isManagedCommand = createManagedCommandMatcher(getManagedScriptFileName()) - for (const [eventName, definitions] of Object.entries(nextHooks)) { - if (!Array.isArray(definitions)) { - continue - } - const cleaned = removeManagedCommands(definitions, isManagedCommand) - if (cleaned.length === 0) { - delete nextHooks[eventName] - } else { - nextHooks[eventName] = cleaned - } - } - - config.hooks = nextHooks + buildRemovedConfig(config) await writeHooksJsonAsync(configPath, config)Apply the same replacement inside
removeat Lines 284-298.src/main/agent-trust-presets.ts (1)
204-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sharing the Codex trust-root traversal between the sync and async twins.
resolveCodexProjectTrustRootAsyncduplicates every branch ofresolveCodexProjectTrustRoot, including the reciprocal-link validation that stops workspace-controlled.gitmetadata from widening trust. Two copies of a security check can diverge. A later fix applied to one copy only would leave the other path permissive.You can parameterize the traversal by its reader and canonicalizer, then keep both entry points as thin wrappers.
♻️ Sketch of a shared traversal
+type TrustRootIo = { + readText: (path: string) => Promise<string> | string + canonical: (path: string) => Promise<string> | string +} + +async function resolveTrustRootWith(workspacePath: string, io: TrustRootIo): Promise<string> { + // single implementation of the .git / worktrees / backlink checks +}src/main/minimax/minimax-cookie-store.test.ts (1)
134-154: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for a read that resolves after a clear.
This test covers a queued save landing after a clear. It does not cover the symmetric case:
readMiniMaxSessionCookieAsyncstarting before a clear and resolving after it. That path republishes the cache today. See the issue raised onsrc/main/minimax/minimax-cookie-store.tsLines 162-200.Add a test that holds
readFileMockpending, runsclearMiniMaxSessionCookieAsync()to completion, releases the read, and then assertsreadMiniMaxSessionCookie()returns null.src/main/speech/openai-api-key-store-off-main-thread.test.ts (1)
63-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset the store module state between tests.
The store keeps
cachedOpenAiSpeechApiKeyand its generation counter at module scope. The static import at Lines 53-58 loads the module once for the whole file, so state carries across tests. Line 102 assertsreadOpenAiSpeechApiKey()throws only because the preceding test happens to clear the key.Reset the module registry in
beforeEachand re-import the store, or add an exported test-only reset. This removes the ordering dependency.src/shared/secure-file-async-write.test.ts (1)
166-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering a chained mutation after a failed one.
chainSecureMutationinsrc/shared/secure-file.tsLine 145 usesprevious.then(run, run)so a rejected mutation still runs the next queued mutation. No test asserts this. Add a case that fails the first write and then queues a second write on the same path, and assert the second write publishes.src/main/devin/hook-service.ts (1)
81-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the mutation serializer into a shared concrete module.
serializeConfigMutationis duplicated insrc/main/devin/hook-service.ts,src/main/kimi/hook-service.ts, andsrc/main/hermes/hook-service.ts.src/main/amp/hook-service.tsuses the identical serializer underserializePluginMutation. Move it to a concretely named module such assrc/main/agent-hooks/hook-config-mutation-queue.ts, then import it from each hook service.src/main/hermes/hook-service.ts (1)
146-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared async filesystem primitives into one module. This PR adds the same three primitives to several hook services:
isMissingPath, an atomic temp-write-plus-rename helper, and a module-levelpendingMutationserializer. Each copy is byte-similar, so a later fix to one copy will not reach the others. Move them to a concrete domain module, for examplesrc/main/agent-hooks/agent-config-file-io.ts, and export acreateMutationSerializer()factory so each service keeps its own queue.
src/main/hermes/hook-service.ts#L146-L184: replaceisMissingPath,writeConfigFile, andserializeConfigMutationwith imports from the shared module; keep the.bakbehavior as an option.src/main/amp/hook-service.ts#L112-L141: replacewriteTextFileAtomicandserializePluginMutationwith the shared helper andcreateMutationSerializer().src/main/kimi/hook-service.ts#L106-L142: replacewriteConfigTomlandserializeConfigMutationwith the shared helper andcreateMutationSerializer().src/main/devin/hook-config-json.ts#L13-L17: use the sharedisMissingPathinstead of the inlineENOENT/ENOTDIRcheck.Do not name the new module
utils,helpers,common, orshared, as per coding guidelines: "Do not use vague names such ashelpers,utils,common,misc, orshared-stufffor files, folders, or modules; use concrete domain names".Source: Coding guidelines
config/scripts/libuv-threadpool-starvation-probe.mjs (1)
17-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the probe against Windows and non-numeric input.
mkfifodoes not exist on Windows, soexecFileSyncthrowsENOENTwith no explanation. A non-numeric first argument makesstuckCountNaN,Array.from({ length: NaN })returns an empty array, and the probe reports a healthy verdict without parking any thread.♻️ Proposed fix
-const stuckCount = Number(process.argv[2] ?? 4) +if (process.platform === 'win32') { + console.error('This probe needs mkfifo; run it on macOS or Linux.') + process.exit(1) +} +const parsedStuckCount = Number(process.argv[2] ?? 4) +if (!Number.isInteger(parsedStuckCount) || parsedStuckCount < 1) { + console.error('stuckCount must be a positive integer') + process.exit(1) +} +const stuckCount = parsedStuckCountsrc/main/ipc/filesystem-auth.ts (1)
60-76: 🩺 Stability & Availability | 🔵 TrivialConsider bounding the gate wait, and consider evicting entries for permanently stalled paths.
A
realpathon a dead mount never settles. Its entry then stays inpendingExternalPathCanonicalizationsfor the process lifetime, andawaitPendingCanonicalizationsForblocks every later read of that path or any descendant of it, with no path to recovery other than a restart. The event loop stays responsive, so the symptom is a permanently pending IPC reply rather than a freeze.The PR states that no timeout mechanism is added, so this is scope guidance, not a blocker. A bounded race (for example,
Promise.racewith a timer that resolves tofalse) would let the read fall through to the remaining checks and fail with the normal access-denied path instead of hanging.Also applies to: 422-430
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd5e9787-a64f-4e3e-bf21-cd34be0e1186
📒 Files selected for processing (97)
config/scripts/libuv-threadpool-starvation-probe.mjssrc/cli/handlers/agent-hooks.tssrc/main/agent-hooks/agent-hook-install-off-main-thread.test.tssrc/main/agent-hooks/hooks-json-async-write-serialization.test.tssrc/main/agent-hooks/hooks-json-async-write.tssrc/main/agent-hooks/hooks-json-read.tssrc/main/agent-hooks/installer-utils.tssrc/main/agent-hooks/managed-agent-hook-controls.tssrc/main/agent-hooks/managed-agent-hook-mutation-serialization.test.tssrc/main/agent-hooks/managed-agent-hook-registry.tssrc/main/agent-hooks/managed-hook-stdin-lifecycle.test.tssrc/main/agent-trust-presets-off-main-thread.test.tssrc/main/agent-trust-presets.tssrc/main/amp/hook-service-main-thread-sync-fs.test.tssrc/main/amp/hook-service.test.tssrc/main/amp/hook-service.tssrc/main/antigravity/hook-service.tssrc/main/claude/hook-service.tssrc/main/claude/managed-hook-script.tssrc/main/codex-accounts/fs-utils.tssrc/main/codex-accounts/service.tssrc/main/codex-accounts/system-default-identity-off-main-thread.test.tssrc/main/codex/config-toml-trust.tssrc/main/command-code/hook-service.tssrc/main/copilot/hook-service.tssrc/main/cursor/hook-service.tssrc/main/cursor/managed-hook-script.tssrc/main/devin/hook-config-json-main-thread-sync-fs.test.tssrc/main/devin/hook-config-json.test.tssrc/main/devin/hook-config-json.tssrc/main/devin/hook-service.test.tssrc/main/devin/hook-service.tssrc/main/droid/hook-service.tssrc/main/droid/managed-hook-script.tssrc/main/gemini/hook-service.tssrc/main/git/worktree-shared-directories-poll-block.test.tssrc/main/git/worktree-shared-directories.tssrc/main/grok-accounts/status.test.tssrc/main/grok-accounts/status.tssrc/main/grok/hook-service.tssrc/main/grok/managed-hook-script.tssrc/main/hermes/hook-service-main-thread-sync-fs.test.tssrc/main/hermes/hook-service.test.tssrc/main/hermes/hook-service.tssrc/main/hooks.tssrc/main/index.tssrc/main/ipc/agent-hooks.test.tssrc/main/ipc/agent-hooks.tssrc/main/ipc/agent-trust.tssrc/main/ipc/codex-accounts.tssrc/main/ipc/diagnostics-preview-main-thread-fs.test.tssrc/main/ipc/diagnostics.test.tssrc/main/ipc/diagnostics.tssrc/main/ipc/filesystem-auth-external-path-async.test.tssrc/main/ipc/filesystem-auth.tssrc/main/ipc/filesystem.tssrc/main/ipc/grok-accounts.tssrc/main/ipc/hosted-review.tssrc/main/ipc/keybindings-main-thread-fs.test.tssrc/main/ipc/keybindings.test.tssrc/main/ipc/keybindings.tssrc/main/ipc/minimax-credentials.test.tssrc/main/ipc/minimax-credentials.tssrc/main/ipc/orca-profile-auth-handlers.test.tssrc/main/ipc/orca-profile-request-args.tssrc/main/ipc/orca-profiles-main-thread-fs.test.tssrc/main/ipc/orca-profiles.test.tssrc/main/ipc/orca-profiles.tssrc/main/ipc/speech.tssrc/main/ipc/worktrees.test.tssrc/main/ipc/worktrees.tssrc/main/keybindings/keybinding-file.tssrc/main/keybindings/keybinding-service.tssrc/main/kimi/hook-service-main-thread-sync-fs.test.tssrc/main/kimi/hook-service.test.tssrc/main/kimi/hook-service.tssrc/main/libuv-threadpool-size.test.tssrc/main/libuv-threadpool-size.tssrc/main/minimax/minimax-cookie-store-off-main-thread.test.tssrc/main/minimax/minimax-cookie-store.test.tssrc/main/minimax/minimax-cookie-store.tssrc/main/orca-profiles/profile-index-async-store.tssrc/main/orca-profiles/profile-index-document.tssrc/main/orca-profiles/profile-index-store.tssrc/main/orca-profiles/profile-index-veto-retry-integrity.test.tssrc/main/orca-yaml-hooks-main-thread-block.test.tssrc/main/rate-limits/grok-auth-off-main-thread.test.tssrc/main/rate-limits/grok-auth.test.tssrc/main/rate-limits/grok-auth.tssrc/main/runtime/orca-runtime-git.tssrc/main/speech/openai-api-key-store-off-main-thread.test.tssrc/main/speech/openai-api-key-store.test.tssrc/main/speech/openai-api-key-store.tssrc/shared/secure-file-async-write.test.tssrc/shared/secure-file.test.tssrc/shared/secure-file.tssrc/shared/secure-path-hardening-snapshot.ts
💤 Files with no reviewable changes (2)
- src/main/grok-accounts/status.test.ts
- src/main/grok-accounts/status.ts
| it('follows a symlinked config to its real path instead of replacing the link', async () => { | ||
| const { symlink } = await import('node:fs/promises') | ||
| const realPath = join(state.home, 'dotfiles-settings.json') | ||
| const linkPath = join(state.home, 'settings.json') | ||
| await writeFile(realPath, '{}\n', 'utf-8') | ||
| await symlink(realPath, linkPath) | ||
|
|
||
| await writeHooksJsonAsync(linkPath, { hooks: { Stop: [] } }) | ||
|
|
||
| const { lstat } = await import('node:fs/promises') | ||
| expect((await lstat(linkPath)).isSymbolicLink()).toBe(true) | ||
| expect(JSON.parse(await readFile(realPath, 'utf-8')).hooks).toEqual({ Stop: [] }) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Skip the symlink test on Windows.
This test calls symlink. Ordinary Windows CI agents lack the elevation or Developer Mode permission required to create symlinks, so symlink fails with EPERM and the test fails for an environment reason. The neighbouring mode-preservation test already guards platform-specific behaviour with process.platform !== 'win32'.
🛡️ Proposed guard
- it('follows a symlinked config to its real path instead of replacing the link', async () => {
+ it.skipIf(process.platform === 'win32')(
+ 'follows a symlinked config to its real path instead of replacing the link',
+ async () => {
const { symlink } = await import('node:fs/promises')
const realPath = join(state.home, 'dotfiles-settings.json')
const linkPath = join(state.home, 'settings.json')
await writeFile(realPath, '{}\n', 'utf-8')
await symlink(realPath, linkPath)
await writeHooksJsonAsync(linkPath, { hooks: { Stop: [] } })
const { lstat } = await import('node:fs/promises')
expect((await lstat(linkPath)).isSymbolicLink()).toBe(true)
expect(JSON.parse(await readFile(realPath, 'utf-8')).hooks).toEqual({ Stop: [] })
- })
+ }
+ )Based on learnings: in test files under src/**/*.test.ts, any test that creates filesystem symlinks should be guarded with it.skipIf(process.platform === 'win32'). As per coding guidelines: "code, commands, and scripts must work on macOS, Linux, and Windows".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('follows a symlinked config to its real path instead of replacing the link', async () => { | |
| const { symlink } = await import('node:fs/promises') | |
| const realPath = join(state.home, 'dotfiles-settings.json') | |
| const linkPath = join(state.home, 'settings.json') | |
| await writeFile(realPath, '{}\n', 'utf-8') | |
| await symlink(realPath, linkPath) | |
| await writeHooksJsonAsync(linkPath, { hooks: { Stop: [] } }) | |
| const { lstat } = await import('node:fs/promises') | |
| expect((await lstat(linkPath)).isSymbolicLink()).toBe(true) | |
| expect(JSON.parse(await readFile(realPath, 'utf-8')).hooks).toEqual({ Stop: [] }) | |
| }) | |
| it.skipIf(process.platform === 'win32')( | |
| 'follows a symlinked config to its real path instead of replacing the link', | |
| async () => { | |
| const { symlink } = await import('node:fs/promises') | |
| const realPath = join(state.home, 'dotfiles-settings.json') | |
| const linkPath = join(state.home, 'settings.json') | |
| await writeFile(realPath, '{}\n', 'utf-8') | |
| await symlink(realPath, linkPath) | |
| await writeHooksJsonAsync(linkPath, { hooks: { Stop: [] } }) | |
| const { lstat } = await import('node:fs/promises') | |
| expect((await lstat(linkPath)).isSymbolicLink()).toBe(true) | |
| expect(JSON.parse(await readFile(realPath, 'utf-8')).hooks).toEqual({ Stop: [] }) | |
| } | |
| ) |
Sources: Coding guidelines, Learnings
| if (isPermissionError(error) && process.platform === 'win32') { | ||
| try { | ||
| grantDirAcl(dirname(path)) | ||
| await writeFile(path, content, { encoding: 'utf-8', mode }) | ||
| return | ||
| } catch { | ||
| // icacls failure is not actionable; re-throw the original EPERM | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate grantDirAcl and check for synchronous child-process or fs usage.
fd -i 'win32-utils*' src | xargs -r rg -n -C 10 'function grantDirAcl|execFileSync|spawnSync|execSync'Repository: stablyai/orca
Length of output: 12418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation and call sites without executing repository code.
printf '--- locate file ---\n'
fd -i 'hooks-json-async-write.ts' src
printf '\n--- outline ---\n'
ast-grep outline src/main/agent-hooks/hooks-json-async-write.ts --view compact || true
printf '\n--- relevant source lines ---\n'
sed -n '1,150p' src/main/agent-hooks/hooks-json-async-write.ts
printf '\n--- call sites of grantDirAcl and execution-mode related async path imports ---\n'
rg -n -C 3 'grantDirAclAsync|grantDirAcl\([' src/main src/test src/e2e 2>/dev/null || trueRepository: stablyai/orca
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation and call sites without executing repository code.
echo '--- locate file ---'
fd -i 'hooks-json-async-write.ts' src || true
echo
echo '--- outline ---'
ast-grep outline src/main/agent-hooks/hooks-json-async-write.ts --view compact || true
echo
echo '--- relevant source lines ---'
sed -n '1,160p' src/main/agent-hooks/hooks-json-async-write.ts
echo
echo '--- call sites of grantDirAcl and async ACL helper imports ---'
rg -n -C 4 'grantDirAclAsync|grantDirAcl\(|execFileWithoutBlocking|writeWithAclRetry' src/main src/test src/e2e 2>/dev/null || trueRepository: stablyai/orca
Length of output: 24803
Use the async ACL retry path or make ACL grants async.
writeWithAclRetry imports and calls synchronous grantDirAcl, which runs execFileSync; Windows identity resolution also uses execFileSync. A recursive ACL grant can wait up to 60s, so the EPERM fallback blocks the async main-thread write path. Route this path to the non-blocking ACL implementation instead.
| export function writeHooksJsonAsync( | ||
| configPath: string, | ||
| config: HooksConfig, | ||
| options?: { preserveMode?: boolean } | ||
| ): Promise<void> { | ||
| // Why: key on the caller's path, not the resolved one — the symlink probe is | ||
| // itself part of the critical section. | ||
| return serializeAtomicFileWrite(configPath, () => writeHooksJsonNow(configPath, config, options)) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List call sites of the sync and async hooks-json writers to find paths written by both.
rg -nP --type=ts '\b(writeHooksJson|writeHooksJsonAsync|writeManagedScript|writeManagedScriptAsync)\s*\(' -g '!**/*.test.ts' -C 2
# Inspect the shared serialization map implementation.
fd -i 'fs-utils*' src/main/codex-accounts | xargs -r rg -n -C 8 'serializeAtomicFileWrite'Repository: stablyai/orca
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -u
echo "== repo files matching hooks/json/fs utils =="
git ls-files | rg -i 'hooks-json|installer-utils|fs-utils|agent-hooks' | sed -n '1,120p'
echo
echo "== writer references (all tracked files, excluding tests if present) =="
rg -nP -e '\b(writeHooksJson|writeHooksJsonAsync|writeManagedScript|writeManagedScriptAsync)\s*\(' . -g '!**/*.test.ts' -g '!**/*.spec.ts' -C 3 || true
echo
echo "== serializeAtomicFileWrite references =="
rg -nP -e 'serializeAtomicFileWrite' . -g '!**/*.test.ts' -g '!**/*.spec.ts' -C 6 || trueRepository: stablyai/orca
Length of output: 37294
🏁 Script executed:
#!/bin/bash
set -u
echo "== hook config path constants/helpers =="
rg -n -C 5 'hooksWritePath|hooksJsonPath|getManagedJsonPath|configPath|hookConfigWritePath|writeHooksJsonPath|HooksWritePath' src/src | sed -n '1,220p' || true
rg -n -C 5 'hooksWritePath|hooksJsonPath|getManagedJsonPath|configPath|hookConfigWritePath|writeHooksJsonPath|writeHooksJsonAsync|writeHooksJson\(' src/main | sed -n '1,260p' || true
echo
echo "== hook-service configPath definitions =="
for f in src/main/**/hook-service.ts; do
name=$(basename "$f")
echo "--- $name ---"
rg -n -C 8 'const configPath|let configPath|function .*config|this\.(status|getConfig)|writeHooksJson(Async?)\(' "$f" | sed -n '1,220p' || true
done
echo
echo "== target callers and resolved path setup =="
for f in src/main/command-code/hook-service.ts src/main/droid/hook-service.ts src/main/copilot/hook-service.ts src/main/cursor/hook-service.ts src/main/gemini/hook-service.ts src/main/antigravity/hook-service.ts src/main/grok/hook-service.ts src/main/command-code/hook-service.ts src/main/droid/hook-service.ts src/main/copilot/hook-service.ts; do
if git ls-files -s "$f" >/dev/null; then
echo "--- $f ---"
rg -n -C 6 'const configPath|let configPath|statusPath|get.*Path|install|remove|writeHooksJson' "$f" | sed -n '1,160p' || true
fi
done
echo
echo "== codex hooks path generation definitions =="
rg -n -C 8 'hooksWritePath|hooksJsonPath|writeCodexHooksJson|writeHooksJson\(' src/main/codex src/main/codex-accounts src/main/codex-real-home-hook-install.ts | sed -n '1,240p' || trueRepository: stablyai/orca
Length of output: 50370
Include the synchronous hooks-JSON writers in atomic-file serialization.
writeHooksJsonAsync chains on config paths with serializeAtomicFileWrite, but writeHooksJson bypasses that chain in the managed hook services and Codex install paths. A synchronous write can occur while the async chain is reading and renaming, so the earlier writer can publish stale or lost content. Add writeHooksJson to the same per-path critical section, or replace the sync writer if all current consumers require the async path.
| async function waitForDeferredRealpath(): Promise<void> { | ||
| for (let attempt = 0; attempt < 50 && deferredNativeRealpaths.resume.length === 0; attempt++) { | ||
| await new Promise((fulfill) => setTimeout(fulfill, 0)) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Determine which realpath API the trust-preset writers use.
rg -n -C3 'realpath' src/main/agent-trust-presets.ts src/main/codex/config-toml-trust.ts src/shared/secure-file.tsRepository: stablyai/orca
Length of output: 8874
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg 'agent-trust-presets-off-main-thread\.test\.ts|codex-accounts/fs-utils\.ts|agent-trust-presets\.ts|config-toml-trust\.ts$'
echo
echo "== test outline =="
ast-grep outline src/main/agent-trust-presets-off-main-thread.test.ts --view expanded || true
echo
echo "== relevant test sections =="
sed -n '80,135p' src/main/agent-trust-presets-off-main-thread.test.ts
sed -n '280,335p' src/main/agent-trust-presets-off-main-thread.test.ts
echo
echo "== fs-utils realpathNativeAsync =="
sed -n '1,140p' src/main/codex-accounts/fs-utils.tsRepository: stablyai/orca
Length of output: 9435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test mock setup =="
sed -n '1,105p' src/main/agent-trust-presets-off-main-thread.test.ts
echo
echo "== agent-trust-presets async paths =="
rg -n -C3 'realpathNativeAsync|realpath' src/main/agent-trust-presets.ts
echo
echo "== fs-utils realpathNativeAsync full section =="
rg -n -C10 'export function realpathNativeAsync|function realpathNativeAsync|realpathNativeAsync' src/main/codex-accounts/fs-utils.ts
echo
echo "== test mock lines with realpath =="
rg -n -C2 "realpathSync|realpathCallback|realpath\\(|node:fs|vi\\.mock" src/main/agent-trust-presets-off-main-thread.test.tsRepository: stablyai/orca
Length of output: 10074
Fail fast when deferred realpaths are not parked.
waitForDeferredRealpath() can return after 50 zero-timeout cycles with deferredNativeRealpaths.resume still empty. Add an assertion after the loop so a broken realpath.native mock cannot make the interleaving/hang tests pass without exercising the real code path.
| remove(): Promise<AgentHookInstallStatus> { | ||
| return serializePluginMutation(async () => { | ||
| const pluginPath = getPluginPath() | ||
| const state = await readLocalPluginState(pluginPath) | ||
| if (state.kind === 'managed') { | ||
| await unlink(pluginPath) | ||
| return this.getStatus() | ||
| } | ||
| return statusFromState(pluginPath, state) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Tolerate a concurrent delete in remove().
readLocalPluginState and unlink are separate operations. If another process deletes the plugin file between the two calls, unlink rejects with ENOENT and the rejection escapes remove(). The caller then reports an error status for a file that is already absent.
Ignore missing-path errors from unlink.
🛡️ Proposed fix
const state = await readLocalPluginState(pluginPath)
if (state.kind === 'managed') {
- await unlink(pluginPath)
+ await unlink(pluginPath).catch((error: unknown) => {
+ if (!isMissingPath(error)) {
+ throw error
+ }
+ })
return this.getStatus()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| remove(): Promise<AgentHookInstallStatus> { | |
| return serializePluginMutation(async () => { | |
| const pluginPath = getPluginPath() | |
| const state = await readLocalPluginState(pluginPath) | |
| if (state.kind === 'managed') { | |
| await unlink(pluginPath) | |
| return this.getStatus() | |
| } | |
| return statusFromState(pluginPath, state) | |
| }) | |
| } | |
| remove(): Promise<AgentHookInstallStatus> { | |
| return serializePluginMutation(async () => { | |
| const pluginPath = getPluginPath() | |
| const state = await readLocalPluginState(pluginPath) | |
| if (state.kind === 'managed') { | |
| await unlink(pluginPath).catch((error: unknown) => { | |
| if (!isMissingPath(error)) { | |
| throw error | |
| } | |
| }) | |
| return this.getStatus() | |
| } | |
| return statusFromState(pluginPath, state) | |
| }) | |
| } |
| beforeEach(() => { | ||
| home = mkdtempSync(join(tmpdir(), 'orca-kimi-sync-fs-')) | ||
| kimiHome = join(home, '.kimi-code') | ||
| originalHome = process.env.HOME | ||
| originalKimiHome = process.env.KIMI_CODE_HOME | ||
| process.env.HOME = home | ||
| process.env.KIMI_CODE_HOME = kimiHome | ||
| hangReads.value = false | ||
| stalledRoot.value = home | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Setting HOME does not redirect homedir() on Windows.
os.homedir() reads USERPROFILE on Windows, not HOME. KIMI_CODE_HOME covers the config path, but the managed script path resolves under ~/.orca. On Windows, install() then writes into the real user profile and stalledRoot.value = home never matches those paths, so the syncFsCalls assertions no longer cover the script write.
src/main/minimax/minimax-cookie-store-off-main-thread.test.ts mocks node:os homedir() for the same reason. Use the same approach here, or set USERPROFILE as well.
🔧 Minimal fix
originalHome = process.env.HOME
+ originalUserProfile = process.env.USERPROFILE
originalKimiHome = process.env.KIMI_CODE_HOME
process.env.HOME = home
+ process.env.USERPROFILE = home
process.env.KIMI_CODE_HOME = kimiHomeRestore USERPROFILE in afterEach in the same way as HOME.
As per coding guidelines: "code, commands, and scripts must work on macOS, Linux, and Windows".
Source: Coding guidelines
| async function writeConfigToml(configPath: string, text: string): Promise<void> { | ||
| const dir = dirname(configPath) | ||
| mkdirSync(dir, { recursive: true }) | ||
| if (existsSync(configPath)) { | ||
| try { | ||
| if (readFileSync(configPath, 'utf-8') === text) { | ||
| return | ||
| } | ||
| } catch { | ||
| // Fall through to the atomic write path. | ||
| await mkdir(dir, { recursive: true }) | ||
| try { | ||
| if ((await readFile(configPath, 'utf-8')) === text) { | ||
| return | ||
| } | ||
| } catch { | ||
| // Absent or unreadable: fall through to the atomic write path. | ||
| } | ||
| const tmpPath = join(dir, `.${Date.now()}-${randomUUID()}.tmp`) | ||
| try { | ||
| writeFileSync(tmpPath, text, 'utf-8') | ||
| if (existsSync(configPath)) { | ||
| copyFileSync(configPath, `${configPath}.bak`) | ||
| } | ||
| renameSync(tmpPath, configPath) | ||
| } finally { | ||
| if (existsSync(tmpPath)) { | ||
| try { | ||
| unlinkSync(tmpPath) | ||
| } catch { | ||
| // best effort | ||
| await writeFile(tmpPath, text, 'utf-8') | ||
| try { | ||
| await copyFile(configPath, `${configPath}.bak`) | ||
| } catch (error) { | ||
| // No prior config to back up; any other failure still aborts the write. | ||
| if (!isMissingPath(error)) { | ||
| throw error | ||
| } | ||
| } | ||
| await rename(tmpPath, configPath) | ||
| } finally { | ||
| // ENOENT after a successful rename is the normal case. | ||
| await unlink(tmpPath).catch(() => {}) | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict permissions on the temporary file and the .bak copy.
config.toml holds user credentials; src/main/kimi/hook-service.test.ts line 79 asserts api_key = "sk-secret" survives an install. writeFile creates the temporary file with the default mode (0o666 minus umask), and rename keeps that mode on config.toml. copyFile also creates config.toml.bak with default permissions, and the backup is never removed. On a multi-user host, other accounts can read the key.
Create the temporary file with mode 0o600, and apply the same mode to the backup.
🔒️ Proposed fix
const tmpPath = join(dir, `.${Date.now()}-${randomUUID()}.tmp`)
try {
- await writeFile(tmpPath, text, 'utf-8')
+ await writeFile(tmpPath, text, { encoding: 'utf-8', mode: 0o600 })
try {
await copyFile(configPath, `${configPath}.bak`)
+ await chmod(`${configPath}.bak`, 0o600)
} catch (error) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function writeConfigToml(configPath: string, text: string): Promise<void> { | |
| const dir = dirname(configPath) | |
| mkdirSync(dir, { recursive: true }) | |
| if (existsSync(configPath)) { | |
| try { | |
| if (readFileSync(configPath, 'utf-8') === text) { | |
| return | |
| } | |
| } catch { | |
| // Fall through to the atomic write path. | |
| await mkdir(dir, { recursive: true }) | |
| try { | |
| if ((await readFile(configPath, 'utf-8')) === text) { | |
| return | |
| } | |
| } catch { | |
| // Absent or unreadable: fall through to the atomic write path. | |
| } | |
| const tmpPath = join(dir, `.${Date.now()}-${randomUUID()}.tmp`) | |
| try { | |
| writeFileSync(tmpPath, text, 'utf-8') | |
| if (existsSync(configPath)) { | |
| copyFileSync(configPath, `${configPath}.bak`) | |
| } | |
| renameSync(tmpPath, configPath) | |
| } finally { | |
| if (existsSync(tmpPath)) { | |
| try { | |
| unlinkSync(tmpPath) | |
| } catch { | |
| // best effort | |
| await writeFile(tmpPath, text, 'utf-8') | |
| try { | |
| await copyFile(configPath, `${configPath}.bak`) | |
| } catch (error) { | |
| // No prior config to back up; any other failure still aborts the write. | |
| if (!isMissingPath(error)) { | |
| throw error | |
| } | |
| } | |
| await rename(tmpPath, configPath) | |
| } finally { | |
| // ENOENT after a successful rename is the normal case. | |
| await unlink(tmpPath).catch(() => {}) | |
| } | |
| } | |
| async function writeConfigToml(configPath: string, text: string): Promise<void> { | |
| const dir = dirname(configPath) | |
| await mkdir(dir, { recursive: true }) | |
| try { | |
| if ((await readFile(configPath, 'utf-8')) === text) { | |
| return | |
| } | |
| } catch { | |
| // Absent or unreadable: fall through to the atomic write path. | |
| } | |
| const tmpPath = join(dir, `.${Date.now()}-${randomUUID()}.tmp`) | |
| try { | |
| await writeFile(tmpPath, text, { encoding: 'utf-8', mode: 0o600 }) | |
| try { | |
| await copyFile(configPath, `${configPath}.bak`) | |
| await chmod(`${configPath}.bak`, 0o600) | |
| } catch (error) { | |
| // No prior config to back up; any other failure still aborts the write. | |
| if (!isMissingPath(error)) { | |
| throw error | |
| } | |
| } | |
| await rename(tmpPath, configPath) | |
| } finally { | |
| // ENOENT after a successful rename is the normal case. | |
| await unlink(tmpPath).catch(() => {}) | |
| } | |
| } |
| /** Sync twin kept for the MiniMax config resolver, which RateLimitService calls synchronously. */ | ||
| export function readMiniMaxSessionCookie(): string | null { | ||
| if (cachedMiniMaxCookie !== null) { | ||
| return cachedMiniMaxCookie | ||
| } | ||
| const keyPath = getMiniMaxCookiePath() | ||
| if (!existsSync(keyPath)) { | ||
| return null | ||
| } | ||
| // Why: keep hardening out of the decode/decrypt try below so a chmod/ACL | ||
| // failure isn't misreported as a decrypt failure (matches hasMiniMaxSessionCookie). | ||
| let raw: Buffer | ||
| try { | ||
| hardenExistingSecureFile(keyPath) | ||
| // Why: a single read replaces existsSync+readFileSync; ENOENT already means "not configured". | ||
| raw = readFileSync(keyPath) | ||
| } catch (error) { | ||
| console.warn('[minimax] Failed to harden MiniMax cookie file while reading', error) | ||
| if (isMissingFileError(error)) { | ||
| return null | ||
| } | ||
| throw new Error('MiniMax session cookie could not be decrypted') | ||
| } | ||
| try { | ||
| const raw = readFileSync(keyPath) | ||
| const envelope = decodeCookieEnvelope(raw) | ||
| cachedMiniMaxCookie = envelope ? readEnvelope(envelope) : readLegacyCookie(raw) | ||
| scheduleCookieRehardening(keyPath) | ||
| cachedMiniMaxCookie = decodeStoredCookie(raw) | ||
| return cachedMiniMaxCookie | ||
| } | ||
|
|
||
| export async function readMiniMaxSessionCookieAsync(): Promise<string | null> { | ||
| if (cachedMiniMaxCookie !== null) { | ||
| return cachedMiniMaxCookie | ||
| } | ||
| const keyPath = getMiniMaxCookiePath() | ||
| let raw: Buffer | ||
| try { | ||
| raw = await readFile(keyPath) | ||
| } catch (error) { | ||
| console.error('[minimax] failed to decode/decrypt session cookie', error) | ||
| if (isMissingFileError(error)) { | ||
| return null | ||
| } | ||
| throw new Error('MiniMax session cookie could not be decrypted') | ||
| } | ||
| scheduleCookieRehardening(keyPath) | ||
| cachedMiniMaxCookie = decodeStoredCookie(raw) | ||
| return cachedMiniMaxCookie | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Cleared MiniMax cookie can return to the in-memory cache through the read path. Both sites trace to one root cause: readMiniMaxSessionCookieAsync publishes cachedMiniMaxCookie without consulting miniMaxCookieCacheGeneration, so a read that overlaps a clear restores the cleared credential, and no test exercises that ordering.
src/main/minimax/minimax-cookie-store.ts#L162-L200: captureminiMaxCookieCacheGenerationbefore thereadFileawait and assigncachedMiniMaxCookieat Line 198 only when the generation is unchanged.src/main/minimax/minimax-cookie-store.test.ts#L134-L154: add a test that holdsreadFileMockpending, completesclearMiniMaxSessionCookieAsync(), releases the read, and assertsreadMiniMaxSessionCookie()returns null.
📍 Affects 2 files
src/main/minimax/minimax-cookie-store.ts#L162-L200(this comment)src/main/minimax/minimax-cookie-store.test.ts#L134-L154
| } catch (error) { | ||
| await rm(tmpFile, { force: true }) | ||
| throw error | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not let the temp-file cleanup replace the original error.
rm at Line 176 is awaited inside the catch. force: true suppresses ENOENT only. An EACCES, EPERM, or EBUSY rejection replaces the original write error and skips the throw error at Line 177. The caller then sees a cleanup failure instead of the real cause.
Wrap the cleanup so the original error always propagates.
🐛 Proposed fix
} catch (error) {
- await rm(tmpFile, { force: true })
+ await rm(tmpFile, { force: true }).catch(() => undefined)
throw error
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| await rm(tmpFile, { force: true }) | |
| throw error | |
| } | |
| } catch (error) { | |
| await rm(tmpFile, { force: true }).catch(() => undefined) | |
| throw error | |
| } |
| async function applySecurePathRestrictionAsync( | ||
| targetPath: string, | ||
| isDirectory: boolean, | ||
| platform: NodeJS.Platform, | ||
| sync: boolean | ||
| ): Promise<boolean> { | ||
| // Why: Windows hardening is a PowerShell spawn, not an fs syscall — out of the stalled-mount blast radius, so reuse it verbatim. | ||
| if (platform === 'win32') { | ||
| return applySecurePathRestriction(targetPath, isDirectory, platform, sync) | ||
| } | ||
| await chmod(targetPath, isDirectory ? 0o700 : 0o600) | ||
| return true | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Align the async restriction contract with the sync twin: return false instead of rejecting.
applySecurePathRestrictionAsync returns Promise<boolean>, but the POSIX branch at Line 231 cannot return false. If chmod fails, the promise rejects.
The sync twin applySecurePathRestriction returns a boolean, and writeSecureFile Line 102 branches on it: a failed restriction skips the cache update and the write still succeeds. The async path behaves differently. At Line 172 the rename at Line 170 has already published the credential. A chmod rejection there jumps to the catch at Line 175, removes a temp file that no longer exists, and rethrows. The caller reports a failed save while the credential file is on disk, possibly with inherited permissions.
saveMiniMaxSessionCookieAsync in src/main/minimax/minimax-cookie-store.ts Lines 136-143 then skips its cache update and propagates the error to the IPC handler, so the UI reports failure for a save that persisted.
Catch the chmod failure and return false, matching the sync twin.
🔒 Proposed fix
async function applySecurePathRestrictionAsync(
targetPath: string,
isDirectory: boolean,
platform: NodeJS.Platform,
sync: boolean
): Promise<boolean> {
// Why: Windows hardening is a PowerShell spawn, not an fs syscall — out of the stalled-mount blast radius, so reuse it verbatim.
if (platform === 'win32') {
return applySecurePathRestriction(targetPath, isDirectory, platform, sync)
}
- await chmod(targetPath, isDirectory ? 0o700 : 0o600)
- return true
+ try {
+ await chmod(targetPath, isDirectory ? 0o700 : 0o600)
+ return true
+ } catch {
+ return false
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function applySecurePathRestrictionAsync( | |
| targetPath: string, | |
| isDirectory: boolean, | |
| platform: NodeJS.Platform, | |
| sync: boolean | |
| ): Promise<boolean> { | |
| // Why: Windows hardening is a PowerShell spawn, not an fs syscall — out of the stalled-mount blast radius, so reuse it verbatim. | |
| if (platform === 'win32') { | |
| return applySecurePathRestriction(targetPath, isDirectory, platform, sync) | |
| } | |
| await chmod(targetPath, isDirectory ? 0o700 : 0o600) | |
| return true | |
| } | |
| async function applySecurePathRestrictionAsync( | |
| targetPath: string, | |
| isDirectory: boolean, | |
| platform: NodeJS.Platform, | |
| sync: boolean | |
| ): Promise<boolean> { | |
| // Why: Windows hardening is a PowerShell spawn, not an fs syscall — out of the stalled-mount blast radius, so reuse it verbatim. | |
| if (platform === 'win32') { | |
| return applySecurePathRestriction(targetPath, isDirectory, platform, sync) | |
| } | |
| try { | |
| await chmod(targetPath, isDirectory ? 0o700 : 0o600) | |
| return true | |
| } catch { | |
| return false | |
| } | |
| } |
Greptile SummaryThis PR converts 28 synchronous filesystem calls on the Electron main thread to async equivalents, moving them onto libuv's threadpool where a stalled SMB/NFS mount parks a threadpool slot instead of freezing the entire app.
Confidence Score: 4/5Safe to merge with one fix: the async hooks-json reader classifies ENOTDIR as unreadable rather than absent, causing agents to report an error state instead of not-installed when a config path parent component is a non-directory. The async-conversion work is thorough and mechanically sound: serialization chains, veto mechanisms, and generation counters are all correctly wired. One behavioral gap in readHooksJsonWithRawAsync maps only ENOENT to missingConfig(), diverging from the sync twin's existsSync which treats ENOTDIR identically. Every other async absence-check in the PR already handles both codes — this is a targeted one-line fix. Files Needing Attention: src/main/agent-hooks/hooks-json-read.ts needs the ENOTDIR case added to the async twin's error mapping before merge.
|
| Filename | Overview |
|---|---|
| src/main/agent-hooks/hooks-json-read.ts | Added async twin readHooksJsonWithRawAsync; only maps ENOENT to missingConfig, diverging from sync twin's existsSync which treats ENOTDIR/EACCES the same way |
| src/main/rate-limits/grok-auth.ts | Added readGrokAuthSessionAsync and toGrokAccountStatus; async twin uses a two-step access+readFile pattern with minor TOCTOU window |
| src/main/codex-accounts/fs-utils.ts | Added writeFileAtomicallyAsync and the core serializeAtomicFileWrite chain; Windows retry paths mirror the sync twin correctly |
| src/shared/secure-file.ts | Added writeSecureFileAsync, removeSecureFileAsync, hardenExistingSecureFileAsync; per-path promise chain correctly serializes mutations |
| src/main/agent-hooks/hooks-json-async-write.ts | New async twin of the hooks.json writer; uses serializeAtomicFileWrite and correctly handles backup-before-rename atomicity |
| src/main/orca-profiles/profile-index-async-store.ts | New async store with epoch+veto mechanism to handle sync/async writer races; serializes per-path with runExclusive |
| src/main/git/worktree-shared-directories.ts | Added async cache with single-flight refresh serving stale values when a refresh is in flight; handles sync-twin revision bumps correctly |
| src/main/agent-hooks/managed-agent-hook-controls.ts | install/remove now async; module-level serializeMutation chain prevents concurrent operations from interleaving |
| src/main/libuv-threadpool-size.ts | Raises UV_THREADPOOL_SIZE to 16 at import time; imported first in index.ts to fix pool size before it is lazily created |
| src/main/ipc/filesystem-auth.ts | authorizeExternalPath now async; adds awaitPendingCanonicalizationsFor so reads are never denied while canonicalization is in flight |
| src/main/orca-profiles/profile-index-store.ts | Sync bootstrap kept; calls claimInFlightAsyncIndexTmpPath+unlinkSync before every write to veto async renames in flight |
Reviews (1): Last reviewed commit: "fix(main): move freeze-prone sync fs off..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| raw = await readFile(configPath, 'utf-8') | ||
| } catch (error) { | ||
| return (error as NodeJS.ErrnoException).code === 'ENOENT' ? missingConfig() : unreadableConfig() |
There was a problem hiding this comment.
ENOENT is mapped to missingConfig(), but the sync twin's existsSync returns false for ENOTDIR and EACCES too, routing them all to missingConfig(). Every other async absence-check added in this PR (e.g., isMissingFileError in minimax, grokAuthReadErrorResult in grok-auth) checks both codes. Without the ENOTDIR case, a hooks.json path whose parent component is a regular file returns null config (error) instead of {} (absent), making agents report an error state rather than "not installed".
| return (error as NodeJS.ErrnoException).code === 'ENOENT' ? missingConfig() : unreadableConfig() | |
| const code = (error as NodeJS.ErrnoException).code | |
| return code === 'ENOENT' || code === 'ENOTDIR' ? missingConfig() : unreadableConfig() |
| // RateLimitService snapshot can never disagree about being signed in. | ||
| if (!(await pathIsReachable(path))) { | ||
| return { status: 'missing' } | ||
| } | ||
| let contents: string | ||
| try { | ||
| contents = await readFile(path, 'utf-8') | ||
| } catch (err) { | ||
| return grokAuthReadErrorResult(err) | ||
| } | ||
| return parseGrokAuthFile(contents) | ||
| } | ||
|
|
||
| async function pathIsReachable(path: string): Promise<boolean> { | ||
| try { |
There was a problem hiding this comment.
readGrokAuthSessionAsync does access(path) then readFile(path). grokAuthReadErrorResult already returns { status: 'missing' } for ENOENT from readFile, so the access probe is redundant in the common path. If access succeeds but readFile then fails with EACCES (file became unreadable between the two calls), grokAuthReadErrorResult returns { status: 'error' }, while the sync twin's existsSync would have returned false (missing). A single readFile with both ENOENT and EACCES mapped to missing would close the window and match the sync twin.
This branch has not been deployed
1 participant
What
A synchronous fs syscall on the Electron main thread parks the entire app when its target is on a stalled mount (SMB/NFS, VPN drop, sleeping NAS, unmounted external drive). The app stops repainting and stops answering Force Quit. This converts 28 such paths to async, selected from a reproduction pass over 43 documented freezes.
Highest-value ones, because they need no user action to fire:
orca.yamlhook configgit:statushandler — a stalled repo froze the app on a timerRateLimitServiceon a refresh timer — froze a fully idle appinstallManagedAgentHookswas a serial loop with sync fs in every bodyorcaProfiles:listThe measurement that shaped the fix
I instrumented this before writing any of it, because it determines whether the whole approach is sound. FIFOs with no writer stand in for a hung mount (
open(2)blocks uninterruptibly, same shape as an SMB stat in D-state):Two conclusions, both load-bearing:
poolSizeconcurrent stalled ops all other async fs app-wide stops.UV_THREADPOOL_SIZEis raised to 16 as headroom — explicitly not as a bound.No timeout/deadline machinery was added. A main-thread
setTimeoutcannot bound a main-thread block — the timer lives on the thread it would need to bound.The probe is included as
config/scripts/libuv-threadpool-starvation-probe.mjsso this is re-runnable rather than folklore.Atomicity
This is where the risk actually lives. Every
tmp→renameand read-modify-write in these files was serialized for free by the single thread; async breaks that. A per-path promise chain is not sufficient when a sync twin is retained — the sync writer walks straight through it. Two approaches used:realpathon the user's workspace;~/.codex/config.tomland~/.copilot/config.jsonare small files in HOME. Await the canonicalization, keep the read-modify-write synchronous. Fixes the freeze and keeps the single thread as the serialization mechanism.await rename. Same pattern as fix(quit): stop durable state writes from parking the main thread on quit #11931.observability/local-file-sink.tsis deliberately untouched — itswriteSyncis a crash-durability contract that needs a process boundary, not an await.Verification
tsc --noEmit) exit 0. oxlint + oxfmt clean.tmp+rename, 1 guard-spent-by-the-await, plus a module-top-levelpromisifythat broke two unrelated suites at collection). All were fixed and re-verified; the fixes are in this branch.profile-index-veto-retry-integrity.test.tsconstructs the interleaving and asserts no duplicate; it includes a guard assertion so it fails rather than silently passing if the race doesn't occur.Risk notes
src/main/ipc/pty.test.tsandprovider-dispatch.test.tsbroke mid-development from a module-load-timepromisify(realpath.native)in a new module. Fixed (bound lazily), and both suites pass — but it's a reminder that new main-process modules must not touchfsat import time.sendSyncbeforeunloadhandshake andpty:spawnchange synchronous IPC contracts and need their own PR rather than a mechanical sweep.