diff --git a/src/main/hook-archive-timeout-observation.test.ts b/src/main/hook-archive-timeout-observation.test.ts index 62f4b17a386..2c9917dd616 100644 --- a/src/main/hook-archive-timeout-observation.test.ts +++ b/src/main/hook-archive-timeout-observation.test.ts @@ -10,7 +10,6 @@ vi.mock('./effective-hook-config', () => ({ const REPO: Repo = { id: 'r', path: '/repo', displayName: 'r', badgeColor: '#000', addedAt: 0 } -/** Run a real archive script in a real shell, under a deadline short enough to test. */ async function runArchive(script: string, timeoutMs = 400) { const { runHook } = await import('./hooks') const dir = mkdtempSync(join(tmpdir(), 'orca-hook-deadline-')) @@ -22,14 +21,12 @@ async function runArchive(script: string, timeoutMs = 400) { } } -// Why a real shell (#19334): this bug is invisible to a mock. Node's `exec({ timeout })` SIGTERMs -// the child and reports whatever it chose to do, so a hook that traps SIGTERM and exits 0 came -// back as a PASS — a hook cut off mid-archive, indistinguishable from one that finished its work. +// Real shells expose SIGTERM traps that exit 0 after an archive timeout (#19334). describe.skipIf(process.platform === 'win32')('archive hook deadline', () => { it('fails a hook that traps SIGTERM and exits zero, despite its zero exit', async () => { const result = await runArchive("trap 'exit 0' TERM; sleep 30") expect(result.success).toBe(false) - // Withheld, so the removal gate reads `unverifiable` rather than a pass. + // Missing exitCode keeps the removal verdict unverifiable. expect(result.exitCode).toBeUndefined() expect(result.output).toContain('timed out') }, 20_000) @@ -38,7 +35,6 @@ describe.skipIf(process.platform === 'win32')('archive hook deadline', () => { const started = Date.now() const result = await runArchive("trap '' TERM; sleep 30") expect(result.success).toBe(false) - // A hook that ignores the signal must not hold a removal open until it finishes. expect(Date.now() - started).toBeLessThan(10_000) }, 20_000) diff --git a/src/main/hook-runtime-target.ts b/src/main/hook-runtime-target.ts index 304321d24e7..744ace1973f 100644 --- a/src/main/hook-runtime-target.ts +++ b/src/main/hook-runtime-target.ts @@ -40,7 +40,7 @@ export function getHookWslContext( return null } - // Why: project runtime can route a Windows checkout through WSL, so hooks need the Linux view of the path. + // Project runtime may route a Windows checkout through WSL; hooks need its Linux path. return { distro: wslDistro, linuxPath: toLinuxPath(cwd) diff --git a/src/main/hook-termination-real-process.test.ts b/src/main/hook-termination-real-process.test.ts index a314a2d99f4..09927bfa2b3 100644 --- a/src/main/hook-termination-real-process.test.ts +++ b/src/main/hook-termination-real-process.test.ts @@ -15,7 +15,6 @@ const alive = (pid: number): boolean => { } } -/** Run a hook past its deadline and report which of its real processes survived. */ async function survivorsAfterDeadline( script: string ): Promise<{ shell: boolean; child: boolean; output: string; pids: number[] }> { @@ -32,8 +31,7 @@ async function survivorsAfterDeadline( expect(result.success).toBe(false) expect(existsSync(pidFile)).toBe(true) pids = readFileSync(pidFile, 'utf8').trim().split(/\s+/).map(Number) - // Without this, a script that recorded only the shell leaves `pids[1]` undefined, `alive` - // throws, and the missing descendant reads as dead — a test that passes on nothing. + // Require both PIDs so a missing descendant cannot make the liveness assertion pass vacuously. expect(pids).toHaveLength(2) expect(pids.every((pid) => Number.isSafeInteger(pid) && pid > 0)).toBe(true) // SIGTERM lands at the deadline; resistant descendants need the real SIGKILL grace. @@ -43,25 +41,19 @@ async function survivorsAfterDeadline( for (const pid of pids) { try { process.kill(pid, 'SIGKILL') - } catch { - /* already gone */ - } + } catch {} } rmSync(dir, { recursive: true, force: true }) } } -// NO `process.kill` mock, deliberately. The defect this file exists for — `exec` silently ignoring -// `detached`, so the shell was never a group leader and the group signal reached nothing — is -// invisible to a mocked `process.kill`, because the mock makes the signal-0 probe succeed whether -// or not a real group exists. That is the precise condition the bug turns on. +// Mocked kill probes cannot prove a process group exists; exercise real signals here. describe.skipIf(process.platform === 'win32')('hook termination against real processes', () => { it('kills the shell and its child when the deadline expires', async () => { const { shell, child, output } = await survivorsAfterDeadline( 'echo "archive step 3 of 7"\nsleep 120 &\necho "$$ $!" > "$PWD/pids"\nwait' ) expect({ shell, child }).toEqual({ shell: false, child: false }) - // The gate reports this run as `unverifiable`; what the hook printed is the only clue why. expect(output).toContain('archive step 3 of 7') }, 30_000) diff --git a/src/main/hooks-archive-exit-observation.test.ts b/src/main/hooks-archive-exit-observation.test.ts index 7b046e844c8..5856b2bcb02 100644 --- a/src/main/hooks-archive-exit-observation.test.ts +++ b/src/main/hooks-archive-exit-observation.test.ts @@ -10,10 +10,7 @@ vi.mock('./effective-hook-config', () => ({ const REPO: Repo = { id: 'r', path: '/repo', displayName: 'r', badgeColor: '#000', addedAt: 0 } -/** - * A real EventEmitter, so an `error` with no listener throws exactly as Node's would — which is the - * whole point of the stream-error row below. Replays its chunks to whoever subscribes to `data`. - */ +/** EventEmitter preserves Node’s fatal unhandled-error behavior while replaying chunks. */ class FakeStream extends EventEmitter { constructor(private readonly chunks: string[]) { super() @@ -30,7 +27,6 @@ class FakeStream extends EventEmitter { } } -/** Minimal ChildProcess stand-in: runHook reads the streams and waits for close/error. */ function fakeChild( outcome: { code?: number | null; signal?: NodeJS.Signals | null } | Error, stdoutChunks: string[] = [], @@ -79,10 +75,7 @@ async function runArchiveWith( return result } -// Why (#19334): an ABSENT exitCode is what the removal gate reads as `unverifiable`. Every row here -// is a way a hook can fail to deliver one. The timeout and termination arms of the same contract -// are covered against REAL processes in hook-termination-real-process.test.ts — deliberately not -// here, because a mocked child cannot show whether a process group exists. +// Real-process timeout/group tests live in hook-termination-real-process.test.ts (#19334). describe('archive hook exit observation', () => { it('passes a clean run through without an exit code', async () => { await expect(runArchiveWith({ code: 0 })).resolves.toEqual({ success: true, output: '' }) @@ -99,8 +92,6 @@ describe('archive hook exit observation', () => { }) it('caps what it retains from a hook that floods stdout', async () => { - // `exec`'s 1 MiB maxBuffer is gone with `spawn`; without a cap a flooding hook grows the main - // process's heap for the whole 120 s deadline. const megabyte = 'x'.repeat(1024 * 1024) const result = await runArchiveWith( { code: 0 }, @@ -111,8 +102,6 @@ describe('archive hook exit observation', () => { }) it('survives an error on the output stream', async () => { - // An `error` with no listener is an uncaught exception, and in the main process that is the - // app. `exec` never covered this either — its only `error` listener is on the child. await expect( runArchiveWith({ code: 0 }, ['partial'], new Error('EIO: read failed')) ).resolves.toMatchObject({ success: true }) @@ -125,8 +114,7 @@ describe('archive hook exit observation', () => { it.each([ ['was killed by a signal', { code: null, signal: 'SIGKILL' as const }], - // A real spawn failure carries a STRING code; the guard under test is `typeof code === - // 'number'`, so a bare Error would pass even if that guard regressed. + // Spawn errors carry string codes; a bare Error would not exercise the numeric-code guard. ['never started', Object.assign(new Error('spawn /bin/bash ENOENT'), { code: 'ENOENT' })] ])('withholds the exit code when the hook %s', async (_label, outcome) => { const result = await runArchiveWith(outcome) diff --git a/src/main/hooks-effective-hook-resolution.test.ts b/src/main/hooks-effective-hook-resolution.test.ts index 6e2f1d674a0..fffaf37bc56 100644 --- a/src/main/hooks-effective-hook-resolution.test.ts +++ b/src/main/hooks-effective-hook-resolution.test.ts @@ -7,7 +7,6 @@ import { TEST_WORKTREE_PATH } from './hooks-test-fixtures' -// Mock fs used by loadHooks vi.mock('fs', () => ({ readFileSync: vi.fn(), existsSync: vi.fn(), @@ -18,7 +17,6 @@ vi.mock('fs', () => ({ })) describe('getEffectiveHooks', () => { - // We need to dynamically import after mocking const makeRepo = (hookSettings?: { mode?: 'auto' | 'override' setupRunPolicy?: 'ask' | 'run-by-default' | 'skip-by-default' @@ -31,7 +29,6 @@ describe('getEffectiveHooks', () => { vi.mocked(fs.existsSync).mockReturnValue(true) vi.mocked(fs.readFileSync).mockReturnValue('scripts:\n setup: |\n echo "yaml setup"\n') - // Re-import to pick up mocks const { getEffectiveHooks } = await import('./hooks') const repo = makeRepo() const result = getEffectiveHooks(repo) diff --git a/src/main/hooks-issue-command.test.ts b/src/main/hooks-issue-command.test.ts index b303b9d724f..675e32c7fa5 100644 --- a/src/main/hooks-issue-command.test.ts +++ b/src/main/hooks-issue-command.test.ts @@ -10,7 +10,6 @@ import { TEST_REPO_PATH } from './hooks-test-fixtures' -// Mock fs used by loadHooks vi.mock('fs', () => ({ readFileSync: vi.fn(), existsSync: vi.fn(), @@ -28,7 +27,6 @@ vi.mock('child_process', () => ({ exec: vi.fn(), execFile: vi.fn(), execFileSync: vi.fn(), - // runner.ts imports spawn from child_process transitively. spawn: vi.fn() })) @@ -203,7 +201,7 @@ describe('createIssueCommandRunnerScript', () => { ['rev-parse', '--git-path', 'orca/issue-command-runner.cmd'], { cwd: 'C:\\repo-worktree' } ) - // Why: the runner file is batch (.cmd) while the pane that launches it is still Git Bash. + // The pane remains Git Bash even when it launches a batch runner. expect(result.shell).toEqual({ family: 'posix' }) } finally { Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) diff --git a/src/main/hooks-orca-yaml-parsing.test.ts b/src/main/hooks-orca-yaml-parsing.test.ts index b99e30cdf24..6c4ccec406a 100644 --- a/src/main/hooks-orca-yaml-parsing.test.ts +++ b/src/main/hooks-orca-yaml-parsing.test.ts @@ -1,7 +1,6 @@ import { describe, expect, it, vi } from 'vitest' import { parseOrcaYaml } from './hooks' -// Mock fs used by loadHooks vi.mock('fs', () => ({ readFileSync: vi.fn(), existsSync: vi.fn(), @@ -338,9 +337,7 @@ describe('parseOrcaYaml', () => { expect(result?.worktree?.sharedDirectories).toEqual(['node_modules']) }) - // Why: `resolve()` collapses `.` when the link is created, but Git reports the - // collapsed path — keeping the raw entry would leave a link that every later - // comparison misses, which is the permanently-dirty worktree this feature fixes. + // Resolve collapses path segments; raw entries would never match Git’s normalized paths. it('drops sharedDirectories entries that still need path collapsing', () => { const result = parseOrcaYaml( [ diff --git a/src/main/hooks-runner.test.ts b/src/main/hooks-runner.test.ts index f1d0d9efe6f..68b807b7a45 100644 --- a/src/main/hooks-runner.test.ts +++ b/src/main/hooks-runner.test.ts @@ -22,8 +22,6 @@ vi.mock('fs', () => ({ vi.mock('child_process', () => ({ exec: vi.fn(), execFileSync: execFileSyncMock, - // runner.ts imports these from child_process; stubs prevent - // "missing export" errors when the mock is resolved transitively. execFile: vi.fn(), spawn: vi.fn() })) @@ -63,7 +61,6 @@ describe('createSetupRunnerScript', () => { ORCA_WORKTREE_PATH: 'C:\\repo\\feature\\', ORCA_WORKSPACE_NAME: 'feature' }), - // Why: native Windows worktrees without a configured setup shell keep the cmd runner. shell: { family: 'cmd' } }) expect(vi.mocked(fs.writeFileSync)).toHaveBeenCalledWith( @@ -111,7 +108,7 @@ describe('createSetupRunnerScript', () => { ORCA_WORKTREE_PATH: '/c/repo/feature', CONDUCTOR_ROOT_PATH: '/c/Users/jinwo/git/orca', GHOSTX_ROOT_PATH: '/c/Users/jinwo/git/orca', - // Why: a display name, never a path — it must survive the conversion untouched. + // Workspace names are labels and must not undergo path conversion. ORCA_WORKSPACE_NAME: 'feature' }), shell: { family: 'posix' } @@ -121,7 +118,7 @@ describe('createSetupRunnerScript', () => { '#!/usr/bin/env bash\nset -e\npnpm install\n', 'utf-8' ) - // Why: chmod over a native Windows path is meaningless; only the WSL branch sets the bit. + // Only guest Linux paths need chmod; native Windows paths do not. expect(vi.mocked(fs.chmodSync)).not.toHaveBeenCalledWith( 'C:\\repo\\.git\\worktrees\\feature\\orca\\setup-runner.sh', 0o755 @@ -185,8 +182,7 @@ describe('createSetupRunnerScript', () => { const runner = buildWindowsRunnerScript('echo hello!world!') - // Why: launchers invoke the runner under cmd /v:on, so the runner must disable delayed - // expansion itself or `!world!` is consumed as a variable reference. + // Disable inherited cmd delayed expansion so !world! remains literal. expect(runner).toContain('setlocal EnableExtensions DisableDelayedExpansion') expect(runner).toContain('call echo hello!world!') }) @@ -194,10 +190,7 @@ describe('createSetupRunnerScript', () => { it('refuses to run a shebang script as a batch command', async () => { const { buildWindowsRunnerScript } = await import('./setup-runner-script-text') - // Regression: a POSIX script reaching the cmd runner (no Git Bash terminal, or an SSH - // Windows host) must not have its interpreter-agnostic prefix executed under cmd — - // `pnpm install` would run and only a later bash-only line would fail, leaving the - // worktree half set up. + // Reject POSIX scripts before cmd runs even their portable prefix and leaves setup incomplete. const runner = buildWindowsRunnerScript('#!/usr/bin/env bash\npnpm install\nsource .env') expect(runner).not.toContain('call pnpm install') @@ -218,8 +211,7 @@ describe('createSetupRunnerScript', () => { it.skipIf(process.platform === 'win32')( 'runs a script whose shebang carries invocation-only bash flags', async () => { - // Regression: `set` rejects `-l`/`-s`/`-i` with exit 2, and the runner's own `set -e` turns - // that into an aborted setup before the first user line. + // Reapplying login/interactive flags through set would abort before the first user line. const { buildPosixRunnerScript } = await import('./setup-runner-script-text') const { mkdtempSync, rmSync, writeFileSync } = await vi.importActual('node:fs') const { execFileSync } = await vi.importActual('node:child_process') @@ -435,7 +427,7 @@ describe('createIssueCommandRunnerScript', () => { { wslDistro: 'Ubuntu' } ) - // Why: the runner path is written back in native Windows form, so the launch needs /mnt again. + // Runner paths are stored in Windows form and need /mnt translation again at launch. expect(result.runnerScriptPath).toBe('C:\\repo\\.git\\orca\\issue-command-runner.sh') expect(result.shell).toEqual({ family: 'posix', executable: 'wsl.exe' }) } finally { diff --git a/src/main/hooks-setup-runner-script.test.ts b/src/main/hooks-setup-runner-script.test.ts index 2b3da9d0974..3544d70f7fb 100644 --- a/src/main/hooks-setup-runner-script.test.ts +++ b/src/main/hooks-setup-runner-script.test.ts @@ -3,7 +3,6 @@ import type * as GitRunner from './git/runner' import { describe, expect, it, vi } from 'vitest' import { makeHookTestRepo } from './hooks-test-fixtures' -// Mock fs used by the runner-script writers vi.mock('fs', () => ({ readFileSync: vi.fn(), existsSync: vi.fn(), @@ -21,7 +20,6 @@ vi.mock('child_process', () => ({ exec: vi.fn(), execFile: vi.fn(), execFileSync: vi.fn(), - // runner.ts imports spawn from child_process transitively. spawn: vi.fn() })) @@ -131,8 +129,7 @@ describe('createSetupRunnerScript', () => { }) it('keeps batch setup scripts on cmd.exe when the terminal is Git Bash', async () => { - // Regression (#6967): a Git Bash terminal preference used to hand pre-existing - // batch setup scripts to bash, where `copy`/`xcopy`/`if errorlevel` do not exist. + // Batch scripts must retain cmd semantics even when the pane uses Git Bash (#6967). gitExecFileSyncMock.mockReset() gitExecFileSyncMock.mockReturnValue('C:\\repo\\.git\\orca\\setup-runner.cmd\n') const fs = await import('node:fs') @@ -162,8 +159,7 @@ describe('createSetupRunnerScript', () => { ) expect(result).toMatchObject({ runnerScriptPath: 'C:\\repo\\.git\\orca\\setup-runner.cmd', - // Why: `shell` is the pane that types the launch command — still Git Bash here. The - // runner's own .cmd extension is what says the file is batch. + // shell describes the pane; the .cmd extension selects the runner’s interpreter. shell: { family: 'posix' } }) } finally { @@ -172,8 +168,7 @@ describe('createSetupRunnerScript', () => { }) it('hands a Git Bash pane a launch command MSYS cannot rewrite for a cmd runner', async () => { - // Regression (#6896): `cmd.exe /c "C:\...\setup-runner.cmd"` typed into Git Bash has its - // `/c` switch rewritten into a drive path, so cmd opens interactively and setup never runs. + // MSYS rewrites cmd.exe /c into a drive path, leaving setup stuck at an interactive prompt (#6896). gitExecFileSyncMock.mockReset() gitExecFileSyncMock.mockReturnValue('C:\\repo\\.git\\orca\\setup-runner.cmd\n') const originalPlatform = process.platform @@ -193,7 +188,6 @@ describe('createSetupRunnerScript', () => { const command = buildSetupRunnerCommand(result.runnerScriptPath, 'windows', result.shell) expect(command).not.toContain('cmd.exe /c') - // Why: the batch runner must never be handed to bash either. expect(command).not.toContain('bash ') expect(command).toContain('powershell.exe') } finally { @@ -202,8 +196,7 @@ describe('createSetupRunnerScript', () => { }) it('replays interpreter flags declared on the shebang line', async () => { - // Regression: the runner is launched as `bash `, so `-euo pipefail` on the script's - // own `#!` line never reaches the interpreter unless the runner re-applies it. + // bash ignores the script’s shebang options unless the runner reapplies them. gitExecFileSyncMock.mockReset() gitExecFileSyncMock.mockReturnValue('C:\\repo\\.git\\orca\\setup-runner.sh\n') const fs = await import('node:fs') @@ -463,8 +456,7 @@ describe('resolveSetupRunnerShell', () => { const { resolveSetupRunnerShell } = await import('./worktree-runner-script') const { resolveWindowsGitBashShellPath } = await import('./git-bash') - // Regression: a stale configured path used to commit setup to a .sh runner the - // PTY could never spawn, hanging wait-for-setup until the 2h timeout. + // Reject stale shells before setup waits on a runner the PTY cannot spawn. expect( resolveSetupRunnerShell( { terminalWindowsShell: 'C:\\Program Files\\Git\\bin\\bash.exe' }, diff --git a/src/main/hooks-test-fixtures.ts b/src/main/hooks-test-fixtures.ts index adea3ecfbeb..6e3e1635d02 100644 --- a/src/main/hooks-test-fixtures.ts +++ b/src/main/hooks-test-fixtures.ts @@ -9,7 +9,6 @@ export const TEST_WORKTREE_ORCA_YAML_PATH = join(TEST_WORKTREE_PATH, 'orca.yaml' export const TEST_ISSUE_COMMAND_PATH = join(TEST_REPO_PATH, '.orca', 'issue-command') export const TEST_GITIGNORE_PATH = join(TEST_REPO_PATH, '.gitignore') -/** Minimal repo record the hooks module reads; hookSettings shape varies per suite. */ export const makeHookTestRepo = (hookSettings?: unknown): Repo => ({ id: 'test-id', diff --git a/src/main/hooks.test.ts b/src/main/hooks.test.ts index 3b911de99c6..caf31a31594 100644 --- a/src/main/hooks.test.ts +++ b/src/main/hooks.test.ts @@ -3,7 +3,6 @@ import type * as GitRunner from './git/runner' import { beforeEach, describe, expect, it, vi } from 'vitest' import { makeHookTestRepo } from './hooks-test-fixtures' -// Mock fs used by loadHooks vi.mock('fs', () => ({ readFileSync: vi.fn(), existsSync: vi.fn(), @@ -20,8 +19,6 @@ const { spawnMock, runWslProcessMock, gitExecFileSyncMock } = vi.hoisted(() => ( })) vi.mock('child_process', () => ({ - // One `spawn` for both: hooks.ts runs the script through it, and runner.ts imports it - // transitively. A second key here silently shadowed the first. spawn: spawnMock, execFileSync: vi.fn() })) @@ -35,7 +32,6 @@ vi.mock('./git/runner', async () => ({ gitExecFileSync: gitExecFileSyncMock })) -/** Minimal ChildProcess stand-in: hooks.ts reads the streams and waits for close/error. */ function fakeChild(exit: { code?: number | null; signal?: NodeJS.Signals | null } = { code: 0 }) { const listeners: Record void)[]> = {} const stream = { setEncoding: () => {}, on: () => {} } @@ -59,7 +55,6 @@ function fakeChild(exit: { code?: number | null; signal?: NodeJS.Signals | null } beforeEach(() => { - // Clear as well as re-arm: these assertions are order-sensitive and calls otherwise accumulate. spawnMock.mockClear() spawnMock.mockImplementation(() => fakeChild()) }) @@ -113,8 +108,7 @@ describe('runHook', () => { }) it('does not run setup scripts with a half-activated conda env', async () => { - // Why: setup scripts source conda exactly like a shell rc does, so the - // orphaned CONDA_SHLVL sentinel surfaces as an opaque hook failure (#14195). + // An orphaned CONDA_SHLVL sentinel breaks hook environment activation (#14195). let capturedEnv: Record | undefined spawnMock.mockImplementation((_script, options) => { capturedEnv = (options as { env: Record }).env @@ -180,8 +174,7 @@ describe('runHook', () => { expect.objectContaining({ cwd: '/repo/worktree', shell: '/bin/bash', - // Setup hooks run unattended: git in them must not pop the OS - // credential helper's OAuth window and loop it (issue #7652). + // Unattended Git must not open credential-helper OAuth windows (#7652). env: expect.objectContaining({ GIT_TERMINAL_PROMPT: '0', GCM_INTERACTIVE: 'never' @@ -237,8 +230,7 @@ describe('runHook', () => { loginPath: 'preferred', script: 'echo hello', cwd: '/home/jin/feature', - // #7652 regression: the unattended WSL hook branch must carry the - // credential guard into the guest env. + // WSL hooks must receive the credential guard inside the guest (#7652). env: expect.objectContaining({ ORCA_ROOT_PATH: '/mnt/c/Users/jinwo/git/orca', ORCA_WORKTREE_PATH: '/home/jin/feature', @@ -369,8 +361,7 @@ describe('runHook', () => { }) it('settles WSL hooks when wsl.exe never reports completion', async () => { - // Why no fake timers: the timeout is now runProcess's own, internal to the - // mocked runWslProcess -- there is nothing left in hooks.ts to advance. + // The mocked runWslProcess owns the timeout; hooks.ts has no timer to advance. spawnMock.mockReset() spawnMock.mockImplementation(() => fakeChild()) runWslProcessMock.mockReset() diff --git a/src/main/hooks.ts b/src/main/hooks.ts index 25ad08689cc..c1fed732ec3 100644 --- a/src/main/hooks.ts +++ b/src/main/hooks.ts @@ -21,18 +21,11 @@ import { } from '@orca/process-host/process-tree-termination' import { createOutputSink } from '@orca/process-host/bounded-output-sink' -const HOOK_TIMEOUT = 120_000 // 2 minutes +const HOOK_TIMEOUT = 120_000 type HookProcessOutcome = { success: boolean; output: string; exitCode?: number } -/** - * Turn a finished process into a hook verdict. - * - * Why `timedOut` decides before `code` (#19334): a hook that traps SIGTERM and exits 0 reports a - * zero exit for a run we cut off mid-archive. The exit code of something we stopped is not - * evidence it finished, so a timeout withholds the code and the removal gate reads that as - * `unverifiable` rather than as a pass. - */ +/** Timed-out hooks cannot prove completion, even if their SIGTERM handler exits 0 (#19334). */ function classifyHookProcessResult( result: { code: number | null; stdout: string; stderr: string; timedOut: boolean }, context: { hookName: string; cwd: string; timeoutMs: number } @@ -44,8 +37,7 @@ function classifyHookProcessResult( return { success: false, output: `${streams}\n${message}`.trim() } } if (result.code !== 0) { - // `null` means signalled: there is no exit code, and saying "exit code null" reads as a - // reporting glitch rather than the `unverifiable` verdict the gate is about to give it. + // A signaled exit has no code for the removal gate to verify. const message = result.code === null ? 'Command was terminated without reporting an exit code.' @@ -63,12 +55,7 @@ function classifyHookProcessResult( const SIGTERM_GRACE_MS = 2_000 -/** - * `exec` capped output at 1 MiB and killed the hook on overflow; `spawn` has no cap at all, and a - * hook flooding stdout for the full deadline can take the main process's heap with it. Truncation - * is reported in the output rather than as a failure — a chatty hook that exits 0 did succeed, and - * failing it for being chatty is the `exec` behaviour this is replacing. - */ +/** Bound retained output without failing chatty hooks that otherwise succeed. */ const HOOK_OUTPUT_LIMIT_BYTES = 10 * 1024 * 1024 function readSink(sink: ReturnType): string { @@ -77,7 +64,6 @@ function readSink(sink: ReturnType): string { : sink.text() } -/** A spawn failure: the process never started, so no exit was ever observed. */ function hookProcessError( error: Error, stdout: string, @@ -93,7 +79,6 @@ function hookProcessError( } } -/** A hook that never started reported no exit, so the code stays withheld. */ function hookSpawnFailure( error: unknown, context: { hookName: string; cwd: string } @@ -113,9 +98,6 @@ function getHookShell(): string | undefined { export { parseOrcaYaml } -/** - * Load hooks from orca.yaml in the given repo root. - */ export function loadHooks(repoPath: string): OrcaHooks | null { const yamlPath = join(repoPath, 'orca.yaml') if (!existsSync(yamlPath)) { @@ -130,14 +112,11 @@ export function loadHooks(repoPath: string): OrcaHooks | null { } } -/** - * Check whether an orca.yaml exists for a repo. - */ export function hasHooksFile(repoPath: string): boolean { return existsSync(join(repoPath, 'orca.yaml')) } -// Why: detect unrecognised keys so the UI can suggest an update instead of showing a "could not be parsed" error. +// Unknown keys suggest a newer config format; let the UI offer an update. const RECOGNIZED_ORCA_YAML_KEYS = new Set([ 'scripts', 'setupAgentStartupPolicy', @@ -147,12 +126,11 @@ const RECOGNIZED_ORCA_YAML_KEYS = new Set([ 'worktree' ]) -/** True when `orca.yaml` has a top-level key this version of Orca does not handle. */ export function hasUnrecognizedOrcaYamlKeys(repoPath: string): boolean { try { const content = readFileSync(join(repoPath, 'orca.yaml'), 'utf-8') for (const line of iterateLfScriptLines(content)) { - // Why: match bare `key:` at end-of-line too, since a mapping with a block value on the next line is valid YAML. + // A bare key: is valid YAML when its block value follows on the next line. const m = line.match(/^([A-Za-z][A-Za-z0-9_-]*):(\s|$)/) if (m != null && !RECOGNIZED_ORCA_YAML_KEYS.has(m[1])) { return true @@ -197,19 +175,14 @@ export function getSetupCommandSource( return null } -/** - * Run a named hook script in the given working directory. - */ export function runHook( hookName: 'setup' | 'archive', cwd: string, repo: Repo, hooksPath?: string, projectRuntime?: ProjectExecutionRuntimeResolution | HookRuntimeTarget, - /** Deadline override. Production uses HOOK_TIMEOUT; tests use it to exercise the timeout path. */ timeoutMs: number = HOOK_TIMEOUT - // Why (#19334): an absent exitCode means no exit was ever observed. The archive-hook removal - // gate reads that as `unverifiable` rather than folding it into a zero. + // Failed hooks without an observed exit are unverifiable to the removal gate (#19334). ): Promise<{ success: boolean; output: string; exitCode?: number }> { const hooks = getEffectiveHooks(repo, hooksPath) const script = hooks?.scripts[hookName] @@ -222,15 +195,13 @@ export function runHook( const wslInfo = getHookWslContext(cwd, runtimeTarget) if (wslInfo) { - // Why: hook scripts run inside WSL, so translate the ORCA_* Windows UNC paths to Linux paths. + // WSL hooks need Linux paths for ORCA_* values, including Windows UNC paths. const envVars = getSetupEnvVars(repo, cwd) const wslEnv: Record = {} for (const [key, value] of Object.entries(envVars)) { wslEnv[key] = toLinuxPath(value) } - // Why: same unattended-git guard as the non-WSL branch below (issue - // #7652) — only the guard flags and any indexed git-config protocol are - // meant to reach the guest; askpass stays host-side, same as before. + // Forward unattended Git flags and indexed config to WSL; keep host askpass out of the guest. const guardedEnv = promptGuardShellEnv(wslEnv) const guestEnv: Record = { ...wslEnv } for (const [key, value] of Object.entries(guardedEnv)) { @@ -248,9 +219,7 @@ export function runHook( distro: wslInfo.distro ?? undefined, loginPath: 'preferred', script, - // Why pinned: these are user-authored orca.yaml scripts and the native - // path runs /bin/bash. Defaulting to sh would fail bash-only hooks on WSL - // only -- a downgrade the user never asked for. + // Match native Bash hook semantics; sh would reject Bash-only scripts inside WSL. shell: 'bash', cwd: wslInfo.linuxPath, env: guestEnv, @@ -264,11 +233,7 @@ export function runHook( dropIncoherentCondaActivationEnv(shellHookEnv) return new Promise((resolve) => { - // Why we own the deadline (#19334): Node's `exec({ timeout })` SIGTERMs the child and then - // reports whatever it chose to do, so a hook that traps SIGTERM and exits 0 came back as a - // PASS — a hook cut off mid-archive, indistinguishable from one that finished. Settle on the - // deadline instead, and settle AT it, so a hook that traps and keeps running cannot hold a - // removal open. + // Settle at the deadline even if the hook traps SIGTERM or keeps running (#19334). let settled = false let deadline: NodeJS.Timeout | undefined const settle = (result: HookProcessOutcome): void => { @@ -281,32 +246,23 @@ export function runHook( } resolve(result) } - // Why `spawn` and not `exec` (#19334 follow-up): `detached` is a spawn-only option — `exec` - // accepts and ignores it, so the shell never became a group leader and the group signal below - // had nothing to reach. Passing `shell` as a string keeps Node's own platform invocation, which - // is what `exec` was being kept for: `cmd.exe /d /s /c` on Windows rather than a bare `-c`. + // exec ignores detached; spawn creates the process group needed for tree termination. const child = spawn(script, { cwd, shell: getHookShell(), - // Why: hooks run unattended; block Git Credential Manager's interactive prompt while keeping cached auth (issue #7652). + // Unattended hooks must suppress interactive Git prompts while retaining cached auth (#7652). env: promptGuardShellEnv(shellHookEnv), stdio: ['ignore', 'pipe', 'pipe'], - // Pinned, not left to Node's default, for the same reason `runProcess` pins it: a `cmd.exe` - // hook otherwise flashes a console window and takes focus. Pre-existing — `exec` did not set - // it either — but AGENTS.md asks for it pinned on every Windows spawn. + // Hide Windows hook consoles to prevent flashes and focus theft. windowsHide: true, - // Make the shell a group leader so its children can be reached. Not on Windows, which has no - // process groups in this sense and where `detached` means a new console instead. + // POSIX groups let signals reach descendants; Windows detached would create a new console. ...(process.platform === 'win32' ? {} : { detached: true }) }) const stdout = createOutputSink(HOOK_OUTPUT_LIMIT_BYTES) const stderr = createOutputSink(HOOK_OUTPUT_LIMIT_BYTES) child.stdout?.on('data', (chunk: Buffer | string) => stdout.write(chunk)) child.stderr?.on('data', (chunk: Buffer | string) => stderr.write(chunk)) - // Why listeners that do nothing: an unhandled `error` on a stream is an uncaught exception, and - // in the Electron main process that is the whole app. `exec` never covered this either — its - // only `error` listener is on the child — so this is a pre-existing gap, closed the way - // `runProcess` closes it. Losing output is not worth a crash; the exit code still gets through. + // Unhandled stream errors crash main; retain exit reporting even when output fails. for (const stream of [child.stdin, child.stdout, child.stderr]) { stream?.on('error', () => {}) } @@ -316,7 +272,6 @@ export function runHook( child.on('close', (code, signal) => { settle( classifyHookProcessResult( - // A signalled exit reports no code, which stays `unverifiable` rather than becoming a 0. { code: signal ? null : code, stdout: readSink(stdout), @@ -327,21 +282,16 @@ export function runHook( ) ) }) - // Why guarded: a spawn failure can settle before the deadline is armed, and arming one on a - // finished run would later signal a pid that is gone — and may by then belong to something else. + // Arming after settlement could later signal a reused PID. if (!settled) { deadline = setTimeout(() => { settle( classifyHookProcessResult( - // Keep what the hook printed: it is the only clue to why the removal gate says - // `unverifiable`. { code: null, stdout: readSink(stdout), stderr: readSink(stderr), timedOut: true }, { hookName, cwd, timeoutMs } ) ) - // Orca's own tree terminator: POSIX process groups, `taskkill /t /f` on Windows (where a - // bare `child.kill` reaches only the shell and leaves its descendants running), and the - // recycled-pid guard that hazard needs. SIGTERM first so a well-behaved hook can clean up. + // Terminate descendants too, with a SIGTERM grace period for cooperative hooks to clean up. void signalProcessTree(child, 'SIGTERM') setTimeout(() => { void forceTerminateProcessTree(child) diff --git a/src/main/host-tree-removal-asar.electron.test.ts b/src/main/host-tree-removal-asar.electron.test.ts index a41a55035ac..647c354c1fb 100644 --- a/src/main/host-tree-removal-asar.electron.test.ts +++ b/src/main/host-tree-removal-asar.electron.test.ts @@ -13,14 +13,7 @@ import { dirname, join } from 'node:path' import { afterAll, describe, expect, it } from 'vitest' import { removeTreeSync } from '../shared/windows-transient-lock-removal' -/** - * Why the real binary: Electron patches `fs` so a `*.asar` file reports `isDirectory() === true`, so - * a recursive `rm` descends into the archive, `rmdir`s a real file, and fails the parent with - * ENOTEMPTY. Plain Node has no such shim, so no in-process unit test can reproduce it — and every - * worktree that has run `pnpm install` carries a `default_app.asar`, which is what stranded 267 - * trash entries on the reporting machine. This runs the shipped `removeHostTree` against a real - * archive under the real binary. - */ +/** Electron’s asar fs shim makes recursive removal fail; plain Node cannot reproduce it. */ const requireFromTest = createRequire(import.meta.url) const electronBinary = requireFromTest('electron') as string const electronDist = join(dirname(requireFromTest.resolve('electron/package.json')), 'dist') @@ -29,7 +22,6 @@ const FIXTURE_ASAR = [ join(electronDist, 'resources/default_app.asar') ].find((candidate) => existsSync(candidate)) -// Mirrors the residue reported on the failing machine, down to the depth of the blocking leaf. const ENTRY_NAME = 'wt-1700000000000-abcdef01' const ASAR_PARENT = 'node_modules/.pnpm/electron/node_modules/electron/dist/App/Contents/Resources' @@ -40,7 +32,7 @@ afterAll(() => { try { removeTreeSync(root) } catch { - // A fixture the shim strands is exactly what this file is about; never fail teardown on it. + // Failed cases can strand archives; cleanup must not obscure the assertion. } } }) @@ -51,8 +43,7 @@ function buildDriver(bundlePath: string, target: string, resultPath: string): st return [ `const fs = require('node:fs')`, `const { removeHostTree } = require(${JSON.stringify(bundlePath)})`, - // Why noAsar for the read-back: the shim would report the stranded archive as a directory here - // too, so the residue listing has to be taken with real filesystem semantics. + // Inspect residue without the shim treating remaining archives as directories. `const withoutAsar = (fn) => { const prev = process.noAsar; process.noAsar = true; try { return fn() } finally { process.noAsar = prev } }`, `;(async () => {`, ` let failure = null`, @@ -77,11 +68,7 @@ async function bundleHostTreeRemoval(outFile: string): Promise { ssr: true, rollupOptions: { input: 'src/main/host-tree-removal.ts', - // Why mirror `isExternalMainModule` from electron.vite.config.ts exactly — CJS, and - // `original-fs` deliberately *not* externalized: the shipped bundle does not list it either, - // so if the archive-aware `rm` ever became a static import (or the bundler learned to fold - // `createRequire(...)('original-fs')`) production would silently degrade to the shimmed `fs` - // while a test that pre-externalized it kept passing. + // Match production externals: externalizing original-fs here could hide a bundling regression. output: { format: 'cjs' }, external: (id: string) => isBuiltin(id) || id === 'electron' || id.startsWith('electron/') } @@ -123,8 +110,6 @@ describe('removeHostTree against a tree holding an asar archive', () => { expect(run.status, run.stderr?.slice(-2000)).toBe(0) const probe = JSON.parse(readFileSync(resultPath, 'utf8')) as ProbeResult - // Without an asar-transparent `rm` this is `ENOTEMPTY` and the residue stops at the archive, - // on every attempt, forever — it is not a race a retry can win. expect(probe).toEqual({ failure: null, residue: [] }) }, 120_000 diff --git a/src/main/host-tree-removal.ts b/src/main/host-tree-removal.ts index f789a9861d0..01d30edf8ce 100644 --- a/src/main/host-tree-removal.ts +++ b/src/main/host-tree-removal.ts @@ -1,8 +1,4 @@ -// Why: every recursive host delete Orca performs (worktrees, terminal history, quarantined recovery -// generations) hits the same two hazards, so one helper exists so no call site forgets either. -// Windows stickiness — AV/indexers/late handle releases surface transient EBUSY/ENOTEMPTY/EPERM on a -// tree Node just emptied — and Electron's asar shim, which strands any tree holding a `*.asar` -// (see `asar-transparent-fs`). +// Host deletion needs Windows lock retries and unpatched fs for Electron’s .asar handling. import { win32 } from 'node:path' import { setTimeout as delay } from 'node:timers/promises' @@ -15,8 +11,7 @@ const WINDOWS_REMOVE_RETRY_DELAYS_MS = [250, 500, 1_000, 2_000] /** Convert a native host filesystem path to the Win32 long-path namespace. */ export function toHostFilesystemPath(targetPath: string): string { - // POSIX paths are used by WSL callers even while the Electron process runs - // on Windows; do not reinterpret those as drive-relative Win32 paths. + // WSL callers supply POSIX paths even on Windows; do not interpret them as drive-relative paths. return process.platform === 'win32' && isWindowsAbsolutePathLike(targetPath) && !isWslUncPath(targetPath) @@ -25,8 +20,7 @@ export function toHostFilesystemPath(targetPath: string): string { } export function toHostRemovalPath(targetPath: string): string { - // Why: Git for Windows can fail long recursive deletes even after Orca has - // proven the worktree target; Node's host deletion should use Win32 long paths. + // Node deletion uses Win32 long paths even after Git has verified the target. return toHostFilesystemPath(targetPath) } @@ -46,8 +40,7 @@ function isTransientWindowsRemovalError(error: unknown): boolean { export async function removeHostTree(targetPath: string): Promise { const removalPath = toHostRemovalPath(targetPath) const retryDelays = process.platform === 'win32' ? WINDOWS_REMOVE_RETRY_DELAYS_MS : [] - // Why: large Windows trees commonly surface transient ENOTEMPTY/EPERM while Node walks and - // removes nested directories; Node's own retries absorb that before the loop below has to. + // Node’s retries absorb transient directory locks before the outer retry loop. const rmOptions = transientLockRemovalOptions() let attempt = 0 @@ -59,8 +52,7 @@ export async function removeHostTree(targetPath: string): Promise { if (attempt >= retryDelays.length || !isTransientWindowsRemovalError(error)) { throw error } - // Why: Git/Node recursive deletes on Windows can observe a just-emptied - // directory before antivirus/indexers/handles release it. + // Antivirus, indexers, and late handle release can keep just-emptied Windows directories locked. await delay(retryDelays[attempt]) attempt += 1 } diff --git a/src/main/image-file-signature.ts b/src/main/image-file-signature.ts index 45ea7241ecf..31ecd72724d 100644 --- a/src/main/image-file-signature.ts +++ b/src/main/image-file-signature.ts @@ -3,7 +3,6 @@ export type ImageFileSignature = { extension: 'png' | 'jpg' | 'gif' | 'webp' } -/** Leading bytes needed to recognize every signature below. */ export const IMAGE_SIGNATURE_BYTES = 12 const PNG_SIGNATURE = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a] @@ -24,7 +23,7 @@ function startsWithAscii(buffer: Uint8Array, text: string, offset = 0): boolean ) } -// Why: magic bytes, not a decoder — enough to name a file or reject a non-image. +// Signature checks identify file types without proving the image decodes. export function detectImageFileSignature(buffer: Uint8Array): ImageFileSignature | null { if (startsWith(buffer, PNG_SIGNATURE)) { return { mimeType: 'image/png', extension: 'png' } diff --git a/src/main/in-flight-run-dedupe.ts b/src/main/in-flight-run-dedupe.ts index 388be57d80e..a0261f9f631 100644 --- a/src/main/in-flight-run-dedupe.ts +++ b/src/main/in-flight-run-dedupe.ts @@ -1,12 +1,4 @@ -/** - * Shares one in-flight promise per key so concurrent callers that want the same - * result run the work once. Purely a concurrency collapse, never a cache: the - * entry is dropped the moment the run settles, so the next caller starts fresh - * and re-reads whatever state the run depends on. - * - * Callers own the map, which keeps each lane's exact result type (including - * nullable ones) without a cast at every read. - */ +/** Share concurrent work only; settled results are never cached. */ export function dedupeInFlightRun( runs: Map>, key: string, @@ -18,9 +10,7 @@ export function dedupeInFlightRun( } const run = start() runs.set(key, run) - // Why: clear on rejection too, or one failure would wedge every later caller - // onto the same rejected promise. The identity check keeps a late settle from - // evicting a newer entry for the same key. + // Clear rejected runs too, without letting a late settlement evict a newer run. const clear = (): void => { if (runs.get(key) === run) { runs.delete(key) diff --git a/src/main/index.ts b/src/main/index.ts index 0771f03cd44..6409efdb900 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -51,11 +51,11 @@ function requestDesktopActivation(argv: readonly string[] = []): void { state.mainWindow?.webContents.send('ui:openSkillShare', shareId) }) state.osOpenedDocuments.capture(argv, publishOsOpenedDocuments) - // Why: a duplicate `orca serve` must not drag a headless server into opening a desktop window (#11935). + // Duplicate serve launches must not activate a desktop on a headless server (#11935). if (!shouldActivateDesktopForSecondInstance(argv)) { return } - // Why: the sidecar's throwaway profile must never become the user's window. + // The sidecar’s throwaway profile must never become the user’s window. if (isOrcadSidecarLaunch()) { requestDesktopHandoff() return @@ -63,19 +63,13 @@ function requestDesktopActivation(argv: readonly string[] = []): void { state.desktopActivationGate?.requestActivation() } -/** - * Hands buffered OS-opened document paths to a renderer that has proven it is listening. - * - * Until that proof arrives the paths stay buffered, because `webContents.send` to a renderer - * with no listener attached is dropped silently and the queue would be gone. - */ +/** Buffer documents until listener readiness; webContents.send silently drops unhandled messages. */ function publishOsOpenedDocuments(): void { const targetWindow = state.mainWindow if (!state.osDocumentOpenListenerReady || !targetWindow || targetWindow.isDestroyed()) { return } - // Why consumed before the await: a renderer pull racing this resolve must not take the same - // batch again. The restore() calls hand it back if delivery turns out to be impossible. + // Consume before awaiting to prevent duplicate renderer pulls; restore if delivery fails. const filePaths = state.osOpenedDocuments.consume() if (filePaths.length === 0) { return @@ -106,7 +100,7 @@ const preflightReady = runMainProcessPreflight({ requestDesktopActivation }) -// Why: when another process holds the lock we've already exited; skip file-writing side effects so this transient process never touches userData. +// Skip userData writes after losing the single-instance lock. if (preflightReady) { void leaseAppImageGenerationForCurrentProcess() app.on('open-url', (event, url) => { @@ -116,21 +110,19 @@ if (preflightReady) { event.preventDefault() requestDesktopActivation([url]) }) - // Why: macOS delivers "Open With" as open-file, often before `ready`, and only to a handler - // that claims the event. Unsupported paths stay unclaimed. + // macOS can deliver Open With before ready; claim only supported paths. app.on('open-file', (event, filePath) => { if (!state.osOpenedDocuments.captureFilePaths([filePath], publishOsOpenedDocuments)) { return } event.preventDefault() - // Why gated on isReady: pre-ready the cold-start window is already on its way, and - // activating the gate here would try to open one before Electron can. The sidecar has none. + // Cold-start activation must wait for Electron readiness; sidecars have no pending window. if (app.isReady() || isOrcadSidecarLaunch()) { requestDesktopActivation() } }) state.skillShareDeepLinks.capture(process.argv) - // Why no publish: nothing is listening this early, so the first renderer pulls these on mount. + // The first renderer pulls on mount; nothing listens this early. state.osOpenedDocuments.capture(process.argv) registerMainProcessIpcHandlers() installMainProcessQuitHandlers() diff --git a/src/main/integration-credential-file.ts b/src/main/integration-credential-file.ts index 3925dfabc71..363069dc4b6 100644 --- a/src/main/integration-credential-file.ts +++ b/src/main/integration-credential-file.ts @@ -17,8 +17,7 @@ import { type IntegrationCredentialService } from '../shared/integration-credential-errors' -// Why: connection status treats a token file as a saved credential; empty -// files read as "missing", so counting them would split-brain getStatus. +// Empty files mean no saved credential; keep status consistent with token reads. export function credentialFileHasContent(path: string): boolean { try { return statSync(path).size > 0 @@ -27,8 +26,7 @@ export function credentialFileHasContent(path: string): boolean { } } -// Falls back to 0600 plaintext when the OS keyring is unavailable, matching the -// Linear/Jira token writers so a keyring-less box still connects. +// Owner-only plaintext permits connections when the OS keyring is unavailable. export function writeEncryptedCredential( service: IntegrationCredentialService, path: string, @@ -44,29 +42,24 @@ export function writeEncryptedCredential( writeCredentialFileAtomic(path, Buffer.from(value, 'utf-8')) } -// Why (STA-3941): a direct write can truncate the previous credential if it -// fails or the app dies mid-write, leaving nothing to authenticate with. Write -// a sibling temp file, fsync it, then rename — readers only ever observe the -// old bytes or the complete new ones. +// Stage and fsync before rename so failed writes cannot truncate the saved credential (STA-3941). export function writeCredentialFileAtomic( path: string, data: Buffer, shouldPublish?: () => boolean ): boolean { - // Why unique: a concurrent writer sharing one temp name could truncate the bytes we publish. + // Unique temp names prevent concurrent writers from truncating staged bytes. const tempPath = `${path}.${process.pid}.${randomBytes(4).toString('hex')}.tmp` let handle: number | null = null try { if (process.platform === 'win32') { - // Why born restricted: a sibling would otherwise inherit the directory's DACL and replace - // the previous credential's owner-only ACL. + // Create owner-only staging files so replacement cannot inherit a permissive Windows DACL. writeRestrictedWindowsFile(tempPath, data) fsyncFileSync(tempPath) return publishStagedCredential(tempPath, path, shouldPublish) } handle = openSync(tempPath, 'w', 0o600) - // Why: write(2) is allowed to return a short count, so a single writeSync - // could publish a truncated credential — the very corruption this avoids. + // writeSync can return a short count; publish only after all bytes are written. let written = 0 while (written < data.length) { const bytes = writeSync(handle, data, written, data.length - written) @@ -83,15 +76,11 @@ export function writeCredentialFileAtomic( if (handle !== null) { try { closeSync(handle) - } catch { - // Already closed or invalid; the unlink below is what matters. - } + } catch {} } try { unlinkSync(tempPath) - } catch { - // Nothing to clean up. - } + } catch {} throw error } } @@ -110,9 +99,7 @@ function publishStagedCredential( return true } -// Why: writeFileSync's `mode` only applies when it creates the file, so -// rewriting an existing credential would silently keep looser permissions. -// Best-effort: Windows has no POSIX modes; its staging file is created owner-only instead. +// Creation mode does not tighten existing files; Windows hardens the staging ACL instead. export function restrictCredentialFileToOwner(path: string): void { try { chmodSync(path, 0o600) @@ -128,9 +115,7 @@ export class CredentialDecryptionError extends Error { } } -// Returns the stored token, null when the file is empty, and throws -// CredentialDecryptionError when the file holds ciphertext we cannot decrypt -// (e.g. the user denied the OS keychain prompt after an app re-sign). +// Undecryptable ciphertext throws CredentialDecryptionError; an empty file returns null. export function readStoredCredentialToken( service: IntegrationCredentialService, raw: Buffer @@ -155,8 +140,7 @@ function readPlaintextLegacyCredential( raw: Buffer ): string | null { const plaintext = decodeUtf8(raw) - // Why: legacy plaintext tokens are printable UTF-8; sealed ciphertext - // such as macOS v10 blobs must not be decoded into auth-header junk. + // Legacy plaintext is printable UTF-8; reject ciphertext masquerading as an auth-header value. if (plaintext === null || hasControlCharacter(plaintext)) { throw new CredentialDecryptionError(service) } diff --git a/src/main/integration-state-file-cache.test.ts b/src/main/integration-state-file-cache.test.ts index 1d03d0ec848..f1b3de9e802 100644 --- a/src/main/integration-state-file-cache.test.ts +++ b/src/main/integration-state-file-cache.test.ts @@ -90,7 +90,6 @@ describe('createIntegrationStateFileCache', () => { }) it('reloads when a file appears after an absent first read', () => { - // The desktop app commonly starts before any integration is connected. const path = tempFile() const cache = createIntegrationStateFileCache({ filePath: () => path, @@ -133,7 +132,6 @@ describe('createIntegrationStateFileCache', () => { }) it('recovers when another process writes between the stamp and the read', () => { - // A concurrent write must force another read instead of pinning stale content. const path = tempFile() writeFileSync(path, 'v1') let reads = 0 @@ -155,12 +153,11 @@ describe('createIntegrationStateFileCache', () => { cache.invalidate() writeDuringRead = true - // Reads 'theirs' but stamped against 'ours', so the next look must reload. + // Content read after a concurrent write still carries the earlier stamp, forcing one more read. expect(cache.get()).toEqual({ body: 'theirs' }) expect(cache.get()).toEqual({ body: 'theirs' }) expect(reads).toBe(3) - // Stamp and content now agree, so a further look is served from memory. expect(cache.get()).toEqual({ body: 'theirs' }) expect(reads).toBe(3) }) diff --git a/src/main/integration-state-file-cache.ts b/src/main/integration-state-file-cache.ts index 117c4c0ec71..9a735221410 100644 --- a/src/main/integration-state-file-cache.ts +++ b/src/main/integration-state-file-cache.ts @@ -14,7 +14,6 @@ function readStamp(path: string): string | null { } } -// Every Orca process on a host reads the same integration state files. export function createIntegrationStateFileCache(args: { filePath: () => string readFromDisk: () => T diff --git a/src/main/issue-command-file.ts b/src/main/issue-command-file.ts index 9bb69e82055..5f4b2d12b44 100644 --- a/src/main/issue-command-file.ts +++ b/src/main/issue-command-file.ts @@ -1,4 +1,4 @@ -// Why: `.orca/issue-command` is the per-user override; `orca.yaml` is the tracked project default. +// .orca/issue-command is local; orca.yaml supplies the tracked project default. import { readFileSync, existsSync, mkdirSync, writeFileSync, rmSync } from 'node:fs' import { join } from 'node:path' import { loadHooks } from './hooks' @@ -26,9 +26,6 @@ export type ResolvedIssueCommand = { source: 'local' | 'shared' | 'none' } -/** - * Resolve the GitHub issue command using local override first, then tracked repo config. - */ export function readIssueCommand(repoPath: string): ResolvedIssueCommand { const filePath = getIssueCommandFilePath(repoPath) let localContent: string | null = null @@ -54,10 +51,7 @@ export function readIssueCommand(repoPath: string): ResolvedIssueCommand { } } -/** - * Write the per-user issue command override to `{repoRoot}/.orca/issue-command`. - * Empty content deletes the override so the shared `orca.yaml` command applies again. - */ +/** Empty content removes the local override so the shared orca.yaml command applies. */ export async function writeIssueCommand( repoPath: string, content: string, @@ -82,7 +76,6 @@ export async function writeIssueCommand( writeFileSync(filePath, `${trimmed}\n`, 'utf-8') } catch (err) { console.error('[hooks] Failed to write issue command:', err) - // Why: re-throw so the IPC handler surfaces the write failure to the renderer's .catch(). throw err } } @@ -107,7 +100,6 @@ async function isIssueCommandIgnoredByGit( } } -/** Ensure `.orca` is in `.gitignore` so the per-user directory is never committed. */ function ensureOrcaDirIgnored(repoPath: string): void { const gitignorePath = join(repoPath, '.gitignore') try { diff --git a/src/main/jetbrains-windows-gui-launchers.ts b/src/main/jetbrains-windows-gui-launchers.ts index 900b1339745..2109a1a7247 100644 --- a/src/main/jetbrains-windows-gui-launchers.ts +++ b/src/main/jetbrains-windows-gui-launchers.ts @@ -5,11 +5,7 @@ import { stripMatchingQuotes } from './editor-launcher-name' -// Why: JetBrains installers ship `*64.exe` GUI-subsystem binaries. The -// Toolbox/installer `.cmd`/`.bat` shims (and short-name console `idea.exe` -// stubs) chain through helpers that allocate a visible Command Prompt even -// when the parent spawn uses windowsHide — the STA-3040 prompt-window loop. -// Maps each short console launcher name to its GUI sibling. +// JetBrains console shims can spawn visible helpers despite windowsHide; prefer GUI siblings. const JETBRAINS_WINDOWS_CONSOLE_TO_GUI: Readonly> = { idea: 'idea64', webstorm: 'webstorm64', @@ -26,7 +22,6 @@ const JETBRAINS_WINDOWS_CONSOLE_TO_GUI: Readonly> = { const JETBRAINS_WINDOWS_GUI_NAMES = new Set(Object.values(JETBRAINS_WINDOWS_CONSOLE_TO_GUI)) -/** GUI binary basename for a console or GUI launcher name, or null if unknown. */ function getGuiExecutableName(launcherBaseName: string): string | null { if (JETBRAINS_WINDOWS_GUI_NAMES.has(launcherBaseName)) { return launcherBaseName @@ -34,19 +29,7 @@ function getGuiExecutableName(launcherBaseName: string): string | null { return JETBRAINS_WINDOWS_CONSOLE_TO_GUI[launcherBaseName] ?? null } -/** - * The GUI `*64.exe` beside an already-resolved console entry point, or null. - * - * Why sibling-only: a bare PATH lookup for `idea64` can land in a different, - * stale install, and it costs a second full PATH walk on Electron's main - * thread. Toolbox script directories ship no exe, so those keep the shim and - * rely on the detached-GUI spawn instead. - * - * Upgrades: - * - `idea.cmd` / `idea.bat` / `idea64.cmd` → colocated `idea64.exe` when present - * - short-name console stubs `idea.exe` → colocated `idea64.exe` when present - * - leaves `idea64.exe` and non-JetBrains launchers unchanged - */ +/** Search only beside the resolved launcher; PATH could select another install or block main again. */ export function resolveColocatedJetBrainsGuiExecutable( resolvedCommand: string, fileExists: (path: string) => boolean @@ -59,7 +42,6 @@ export function resolveColocatedJetBrainsGuiExecutable( return null } - // .exe: only short console names (idea.exe), never the GUI binary itself. if (isExe) { const guiName = JETBRAINS_WINDOWS_CONSOLE_TO_GUI[baseName] if (!guiName) { @@ -69,7 +51,6 @@ export function resolveColocatedJetBrainsGuiExecutable( return fileExists(candidate) ? candidate : null } - // .cmd/.bat: idea.cmd and idea64.cmd both prefer idea64.exe when colocated. const guiName = getGuiExecutableName(baseName) if (!guiName) { return null @@ -78,10 +59,7 @@ export function resolveColocatedJetBrainsGuiExecutable( return fileExists(candidate) ? candidate : null } -// Why: only JetBrains batch shims chain through console helpers that outlive -// the batch and re-open a prompt. Other GUI shims (code.cmd, cursor.cmd) exit -// on their own, and running them under `start` would re-parse their quoted -// WSL/SSH remote argv. detachedGui only rewrites batch targets. +// Detach JetBrains shims with persistent console helpers; other GUI shims must retain quoted argv. export function isJetBrainsConsoleShim(command: string, platform: NodeJS.Platform): boolean { return ( platform === 'win32' &&