fix(server): address final review findings

This commit is contained in:
Jinwoo-H
2026-08-12 22:45:02 -04:00
committed by Jinwoo-H
parent 234bb08d72
commit aac2fc8474
10 changed files with 80 additions and 59 deletions
@@ -38,8 +38,8 @@ describe('stopNodeServerVerifierDaemons', () => {
temporaryPaths.push(dataPath)
const daemonPath = join(dataPath, 'daemon')
mkdirSync(daemonPath)
writeFileSync(join(daemonPath, 'daemon-v4.pid'), JSON.stringify({ pid: 4101 }))
writeFileSync(join(daemonPath, 'daemon-v5.pid'), JSON.stringify({ pid: 5101 }))
writeFileSync(join(daemonPath, 'daemon-v4.pid'), JSON.stringify({ pid: 4101 }))
vi.spyOn(process, 'kill').mockImplementation(() => true)
const outcome = stopNodeServerVerifierDaemons(dataPath).catch((error: unknown) => error)
@@ -9,7 +9,7 @@ export async function stopNodeServerVerifierDaemons(dataPath: string): Promise<v
return
}
const shutdowns: Promise<void>[] = []
for (const entry of readdirSync(daemonPath)) {
for (const entry of readdirSync(daemonPath).sort()) {
const match = /^daemon-v(\d+)\.pid$/.exec(entry)
if (!match) {
continue
@@ -156,6 +156,7 @@ describe('prepareWorktreePushTargetWithExec', () => {
remoteUrl: FORK_SSH,
remoteCreated: true
})
expect(onRemoteCreated).toHaveBeenCalledTimes(1)
})
it('inherits remoteCreated when the predicate says a known worktree created the reused remote', async () => {
+2 -51
View File
@@ -1050,7 +1050,6 @@ export async function prepareWorktreePushTarget(
repoId?: string,
gitOptions: { wslDistro?: string; signal?: AbortSignal } = {},
options: {
createdWorktreeId?: string
onRemoteCreated?: (createdTarget: GitPushTarget) => void
onRemoteLifecycleAcquired?: (release: () => void) => void
} = {}
@@ -1059,7 +1058,6 @@ export async function prepareWorktreePushTarget(
localPushTargetRemoteLifecycleScope(repoPath, repoId, gitOptions.wslDistro)
)
let lifecycleTransferred = false
let createdRemote: GitPushTarget | null = null
try {
if (options.onRemoteLifecycleAcquired) {
options.onRemoteLifecycleAcquired(releaseLifecycle)
@@ -1079,34 +1077,9 @@ export async function prepareWorktreePushTarget(
)
: false,
{
onRemoteCreated: (createdTarget) => {
createdRemote = createdTarget
options.onRemoteCreated?.(createdTarget)
}
onRemoteCreated: options.onRemoteCreated
}
)
} catch (error) {
if (createdRemote && store && options.createdWorktreeId) {
try {
const rollbackSignal = createWorktreeRollbackSignal()
await cleanupUnusedWorktreePushTargetRemoteWithExec(
repoPath,
options.createdWorktreeId,
createdRemote,
store,
(args, cwd) =>
gitExecFileAsync(args, {
cwd,
...gitOptions,
signal: rollbackSignal
})
)
createdRemote = null
} catch (cleanupError) {
throw new AggregateError([error, cleanupError], 'Fork remote preparation rollback failed.')
}
}
throw error
} finally {
if (!lifecycleTransferred) {
releaseLifecycle()
@@ -1188,7 +1161,6 @@ async function prepareWorktreePushTargetSsh(
repoId?: string,
options: {
signal?: AbortSignal
createdWorktreeId?: string
onRemoteCreated?: (createdTarget: GitPushTarget) => void
onRemoteLifecycleAcquired?: (release: () => void) => void
} = {}
@@ -1201,7 +1173,6 @@ async function prepareWorktreePushTargetSsh(
const { remoteCreated: _ignoredRemoteCreated, ...sanitizedTarget } = target
let remoteName = target.remoteName
let remoteCreated = false
let createdRemote: GitPushTarget | null = null
try {
if (options.onRemoteLifecycleAcquired) {
options.onRemoteLifecycleAcquired(releaseLifecycle)
@@ -1229,8 +1200,7 @@ async function prepareWorktreePushTargetSsh(
} else {
remoteName = await ensureUniqueRemoteName(execGit, repoPath, target.remoteName)
remoteCreated = true
createdRemote = { ...sanitizedTarget, remoteName, remoteCreated: true }
options.onRemoteCreated?.(createdRemote)
options.onRemoteCreated?.({ ...sanitizedTarget, remoteName, remoteCreated: true })
await provider.exec(['remote', 'add', remoteName, target.remoteUrl], repoPath, {
signal: options.signal
})
@@ -1244,23 +1214,6 @@ async function prepareWorktreePushTargetSsh(
{ signal: options.signal }
)
return { ...sanitizedTarget, remoteName, ...(remoteCreated ? { remoteCreated: true } : {}) }
} catch (error) {
if (createdRemote && store && options.createdWorktreeId) {
try {
const rollbackSignal = createWorktreeRollbackSignal()
await cleanupUnusedWorktreePushTargetRemoteWithExec(
repoPath,
options.createdWorktreeId,
createdRemote,
store,
(args, cwd) => provider.exec(args, cwd, { signal: rollbackSignal })
)
createdRemote = null
} catch (cleanupError) {
throw new AggregateError([error, cleanupError], 'Remote fork preparation rollback failed.')
}
}
throw error
} finally {
if (!lifecycleTransferred) {
releaseLifecycle()
@@ -2064,7 +2017,6 @@ async function createRemoteWorktreeOperation(
repo.id,
{
signal,
createdWorktreeId: rollbackWorktreeId,
onRemoteCreated: (createdTarget) => {
createdPushTargetRemote = createdTarget
},
@@ -2729,7 +2681,6 @@ async function createLocalWorktreeOperation(
repo.id,
localWorktreeGitOptions,
{
createdWorktreeId,
onRemoteCreated: (createdTarget) => {
createdPushTargetRemote = createdTarget
},
+10
View File
@@ -2126,6 +2126,11 @@ describe('registerWorktreeHandlers', () => {
['remote', 'remove', 'pr-contributor-orca'],
expect.objectContaining({ cwd: '/workspace/repo', signal: cleanupSignal })
)
expect(
gitExecFileAsyncMock.mock.calls.filter(
([gitArgs]) => gitArgs[0] === 'remote' && gitArgs[1] === 'remove'
)
).toHaveLength(1)
})
it('removes a local fork remote when add mutates then rejects', async () => {
@@ -2180,6 +2185,11 @@ describe('registerWorktreeHandlers', () => {
['remote', 'remove', 'pr-contributor-orca'],
expect.objectContaining({ cwd: '/workspace/repo', signal: cleanupSignal })
)
expect(
gitExecFileAsyncMock.mock.calls.filter(
([gitArgs]) => gitArgs[0] === 'remote' && gitArgs[1] === 'remove'
)
).toHaveLength(1)
})
it('keeps the Orca-created marker when a new worktree reuses an Orca-created fork remote', async () => {
+3
View File
@@ -4385,6 +4385,9 @@ describe('OrcaRuntimeService', () => {
['remote', 'remove', 'pr-contributor-orca'],
expect.objectContaining({ cwd: TEST_REPO_PATH, signal: cleanupSignal })
)
expect(
gitSpy.mock.calls.filter(([gitArgs]) => gitArgs[0] === 'remote' && gitArgs[1] === 'remove')
).toHaveLength(1)
} finally {
gitSpy.mockRestore()
}
-1
View File
@@ -22480,7 +22480,6 @@ export class OrcaRuntimeService {
repo.id,
localWorktreeGitOptions,
{
createdWorktreeId,
onRemoteCreated: (createdTarget) => {
createdPushTargetRemote = createdTarget
},
+29 -2
View File
@@ -1,4 +1,5 @@
import { EventEmitter } from 'node:events'
import { setGlobalProxyFromEnv } from 'node:http'
import { homedir, tmpdir } from 'node:os'
import { dirname, join } from 'node:path'
@@ -171,6 +172,32 @@ export const webContents = {
getAllWebContents: () => []
}
class NodeHostProxySession {
async resolveProxy(): Promise<'DIRECT'> {
return 'DIRECT'
}
async setProxy(config: {
mode?: 'system' | 'fixed_servers'
proxyRules?: string
proxyBypassRules?: string
}): Promise<void> {
if (config.mode === 'fixed_servers' && config.proxyRules) {
setGlobalProxyFromEnv({
HTTP_PROXY: config.proxyRules,
HTTPS_PROXY: config.proxyRules,
NO_PROXY: config.proxyBypassRules?.replaceAll(';', ',') ?? ''
})
return
}
setGlobalProxyFromEnv(process.env)
}
async closeAllConnections(): Promise<void> {}
}
const nodeHostDefaultSession = new NodeHostProxySession()
export const safeStorage = {
isEncryptionAvailable: () => false,
encryptString: () => {
@@ -236,8 +263,8 @@ export const screen = {
}
export const session = {
defaultSession: null,
fromPartition: () => null
defaultSession: nodeHostDefaultSession,
fromPartition: () => nodeHostDefaultSession
}
export const protocol = { registerSchemesAsPrivileged: () => {} }
@@ -0,0 +1,31 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
const setGlobalProxyFromEnv = vi.hoisted(() => vi.fn())
vi.mock('node:http', () => ({ setGlobalProxyFromEnv }))
import { session } from './node-host-electron-facade'
describe('Node host proxy session', () => {
beforeEach(() => {
setGlobalProxyFromEnv.mockReset()
})
it('reports direct routing so environment proxy settings can be applied', async () => {
await expect(session.defaultSession.resolveProxy()).resolves.toBe('DIRECT')
})
it('applies fixed proxy settings to Node fetch and HTTP clients', async () => {
await session.defaultSession.setProxy({
mode: 'fixed_servers',
proxyRules: 'http://proxy.example:8080',
proxyBypassRules: 'localhost;*.internal'
})
expect(setGlobalProxyFromEnv).toHaveBeenCalledWith({
HTTP_PROXY: 'http://proxy.example:8080',
HTTPS_PROXY: 'http://proxy.example:8080',
NO_PROXY: 'localhost,*.internal'
})
})
})
@@ -31,9 +31,8 @@ describe('RemoteServerFields', () => {
/>
)
expect(markup.indexOf('npx @stablyai/orca@latest')).toBeLessThan(
markup.indexOf('add-server-name')
)
expect(markup).toContain('npx @stablyai/orca@rc')
expect(markup.indexOf('npx @stablyai/orca@rc')).toBeLessThan(markup.indexOf('add-server-name'))
expect(markup).toContain('Copy server command')
})