From e2fa5e354c3fda30653830f92af7e71ba2740a6e Mon Sep 17 00:00:00 2001 From: "buf0-bot[bot]" <252831055+buf0-bot[bot]@users.noreply.github.com> Date: Mon, 18 May 2026 22:43:58 -0700 Subject: [PATCH] fix: pr-bug-scan validated finding from #2270 (#2298) * fix: address pr-bug-scan validated finding from #2270 Restored legacy local hook fallback at hooks.ts:286-307 when commandSourcePolicy is undefined and persisted scripts.setup/archive are non-empty; also patched getSetupCommandSource and updated the regr * Resolve direct PR launch start point correctly - Prevent direct "Use PR" launches from skipping PR head resolution - Avoid legacy local hook fallback whenever an orca.yaml file exists --------- Co-authored-by: orca-bug-scan-bot Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com> --- src/main/hooks.test.ts | 81 ++++++++++++++++++- src/main/hooks.ts | 47 +++++++++-- .../src/lib/launch-work-item-direct.ts | 41 +++++++++- 3 files changed, 158 insertions(+), 11 deletions(-) diff --git a/src/main/hooks.test.ts b/src/main/hooks.test.ts index f8744474f..a6ce1718c 100644 --- a/src/main/hooks.test.ts +++ b/src/main/hooks.test.ts @@ -346,7 +346,7 @@ describe('getEffectiveHooks', () => { expect(result?.scripts.setup).not.toContain('old-version') }) - it('does not fall back to local settings hooks by default when yaml is missing', async () => { + it('falls back to legacy local hooks when policy is unset and yaml is missing', async () => { const fs = await import('fs') vi.mocked(fs.existsSync).mockReturnValue(false) @@ -357,6 +357,26 @@ describe('getEffectiveHooks', () => { }) const result = getEffectiveHooks(repo) + expect(result).toEqual({ + scripts: { + setup: 'echo "local setup"', + archive: 'echo "local archive"' + } + }) + }) + + it('does not fall back to local hooks when policy is explicitly shared-only', async () => { + const fs = await import('fs') + vi.mocked(fs.existsSync).mockReturnValue(false) + + const { getEffectiveHooks } = await import('./hooks') + const repo = makeRepo({ + mode: 'override', + commandSourcePolicy: 'shared-only', + scripts: { setup: 'echo "local setup"', archive: 'echo "local archive"' } + }) + const result = getEffectiveHooks(repo) + expect(result).toBeNull() }) @@ -438,6 +458,21 @@ describe('getEffectiveHooks', () => { }) }) + it('does not fall back to legacy local hooks when yaml exists without supported hooks', async () => { + const fs = await import('fs') + vi.mocked(fs.existsSync).mockReturnValue(true) + vi.mocked(fs.readFileSync).mockReturnValue('futureFeature: enabled\n') + + const { getEffectiveHooks } = await import('./hooks') + const repo = makeRepo({ + mode: 'override', + scripts: { setup: 'echo "legacy setup"', archive: 'echo "legacy archive"' } + }) + const result = getEffectiveHooks(repo) + + expect(result).toBeNull() + }) + it('treats legacy shared-first policy as orca.yaml only', async () => { const fs = await import('fs') vi.mocked(fs.existsSync).mockReturnValue(true) @@ -468,6 +503,50 @@ describe('getEffectiveHooks', () => { expect(result).toBeNull() }) + + it('falls back to legacy local setup source only when yaml is missing', async () => { + const fs = await import('fs') + vi.mocked(fs.existsSync).mockReturnValue(false) + + const { getSetupCommandSource } = await import('./hooks') + const repo = makeRepo({ + mode: 'override', + scripts: { setup: 'echo "legacy setup"', archive: '' } + }) + const result = getSetupCommandSource(repo) + + expect(result).toEqual({ source: 'local', command: 'echo "legacy setup"' }) + }) + + it('does not use legacy local setup source when yaml omits setup', async () => { + const fs = await import('fs') + vi.mocked(fs.existsSync).mockReturnValue(true) + vi.mocked(fs.readFileSync).mockReturnValue('scripts:\n archive: |\n echo "yaml archive"\n') + + const { getSetupCommandSource } = await import('./hooks') + const repo = makeRepo({ + mode: 'override', + scripts: { setup: 'echo "legacy setup"', archive: '' } + }) + const result = getSetupCommandSource(repo) + + expect(result).toBeNull() + }) + + it('does not use legacy local setup source when yaml exists without supported hooks', async () => { + const fs = await import('fs') + vi.mocked(fs.existsSync).mockReturnValue(true) + vi.mocked(fs.readFileSync).mockReturnValue('futureFeature: enabled\n') + + const { getSetupCommandSource } = await import('./hooks') + const repo = makeRepo({ + mode: 'override', + scripts: { setup: 'echo "legacy setup"', archive: '' } + }) + const result = getSetupCommandSource(repo) + + expect(result).toBeNull() + }) }) describe('runHook', () => { diff --git a/src/main/hooks.ts b/src/main/hooks.ts index 32c9eb540..08bf04695 100644 --- a/src/main/hooks.ts +++ b/src/main/hooks.ts @@ -267,7 +267,8 @@ function ensureOrcaDirIgnored(repoPath: string): void { function getEffectiveHookScript( yamlScript: string | undefined, localScript: string | undefined, - policy: HookCommandSourcePolicy + policy: HookCommandSourcePolicy, + legacyFallback: boolean ): string | undefined { const shared = yamlScript?.trim() const local = localScript?.trim() @@ -280,16 +281,38 @@ function getEffectiveHookScript( return [shared, local].filter(Boolean).join('\n') || undefined } + // Why: existing users persisted local setup/archive scripts before + // commandSourcePolicy existed. Without a fallback the new default + // 'shared-only' would silently drop those scripts when orca.yaml is missing. + if (legacyFallback) { + return shared || local || undefined + } + return shared || undefined } +function hasLegacyLocalScripts(repo: Repo): boolean { + return Boolean( + repo.hookSettings?.scripts.setup?.trim() || repo.hookSettings?.scripts.archive?.trim() + ) +} + export function getEffectiveHooks(repo: Repo, worktreePath?: string): OrcaHooks | null { - const yamlHooks = loadHooks(worktreePath ?? repo.path) + const hooksRoot = worktreePath ?? repo.path + const yamlHooks = loadHooks(hooksRoot) + const yamlFileExists = hasHooksFile(hooksRoot) const localSetup = repo.hookSettings?.scripts.setup const localArchive = repo.hookSettings?.scripts.archive - const policy = normalizeHookCommandSourcePolicy(repo.hookSettings?.commandSourcePolicy) - const setup = getEffectiveHookScript(yamlHooks?.scripts.setup, localSetup, policy) - const archive = getEffectiveHookScript(yamlHooks?.scripts.archive, localArchive, policy) + const rawPolicy = repo.hookSettings?.commandSourcePolicy + const policy = normalizeHookCommandSourcePolicy(rawPolicy) + const legacyFallback = rawPolicy === undefined && !yamlFileExists && hasLegacyLocalScripts(repo) + const setup = getEffectiveHookScript(yamlHooks?.scripts.setup, localSetup, policy, legacyFallback) + const archive = getEffectiveHookScript( + yamlHooks?.scripts.archive, + localArchive, + policy, + legacyFallback + ) if (!setup && !archive) { return null @@ -330,9 +353,13 @@ export function getSetupCommandSource( repo: Repo, worktreePath?: string ): { source: 'yaml' | 'local' | 'both'; command: string } | null { - const yamlSetup = loadHooks(worktreePath ?? repo.path)?.scripts.setup?.trim() + const hooksRoot = worktreePath ?? repo.path + const yamlHooks = loadHooks(hooksRoot) + const yamlFileExists = hasHooksFile(hooksRoot) + const yamlSetup = yamlHooks?.scripts.setup?.trim() const localSetup = repo.hookSettings?.scripts.setup?.trim() - const policy = normalizeHookCommandSourcePolicy(repo.hookSettings?.commandSourcePolicy) + const rawPolicy = repo.hookSettings?.commandSourcePolicy + const policy = normalizeHookCommandSourcePolicy(rawPolicy) if (policy === 'local-only') { return localSetup ? { source: 'local', command: localSetup } : null @@ -346,6 +373,12 @@ export function getSetupCommandSource( return { source: 'yaml', command: yamlSetup } } + // Why: pre-policy persisted local setup scripts must keep working when + // orca.yaml is missing; otherwise upgrading silently drops the legacy hook. + if (rawPolicy === undefined && !yamlFileExists && localSetup && hasLegacyLocalScripts(repo)) { + return { source: 'local', command: localSetup } + } + return null } diff --git a/src/renderer/src/lib/launch-work-item-direct.ts b/src/renderer/src/lib/launch-work-item-direct.ts index 1914f8d78..cbff5b379 100644 --- a/src/renderer/src/lib/launch-work-item-direct.ts +++ b/src/renderer/src/lib/launch-work-item-direct.ts @@ -1,10 +1,11 @@ import { toast } from 'sonner' -import { useAppStore } from '@/store' +import { useAppStore, type AppState } from '@/store' import { AGENT_CATALOG } from '@/lib/agent-catalog' import { pasteDraftWhenAgentReady } from '@/lib/agent-paste-draft' import { buildAgentDraftLaunchPlan, buildAgentStartupPlan } from '@/lib/tui-agent-startup' import { TUI_AGENT_CONFIG } from '../../../shared/tui-agent-config' import { activateAndRevealWorktree, type AgentStartedTelemetry } from '@/lib/worktree-activation' +import { callRuntimeRpc, getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' import { CLIENT_PLATFORM, getLinkedWorkItemSuggestedName, @@ -15,6 +16,7 @@ import { ensureHooksConfirmed } from '@/lib/ensure-hooks-confirmed' import { checkRuntimeHooks } from '@/runtime/runtime-hooks-client' import { track, tuiAgentToAgentKind } from '@/lib/telemetry' import type { + GitPushTarget, OrcaHooks, RepoHookSettings, SetupDecision, @@ -86,6 +88,24 @@ function pickAgent( return null } +async function resolveDirectPrStartPoint( + repoId: string, + prNumber: number, + settings: AppState['settings'] +): Promise<{ baseBranch: string; pushTarget?: GitPushTarget }> { + const target = getActiveRuntimeTarget(settings) + const result = + target.kind === 'local' + ? await window.api.worktrees.resolvePrBase({ repoId, prNumber }) + : await callRuntimeRpc< + { baseBranch: string; pushTarget?: GitPushTarget } | { error: string } + >(target, 'worktree.resolvePrBase', { repo: repoId, prNumber }, { timeoutMs: 30_000 }) + if ('error' in result) { + throw new Error(result.error) + } + return result +} + async function resolveSetupDecision( repoId: string, repo: { hookSettings?: RepoHookSettings } @@ -207,6 +227,21 @@ export async function launchWorkItemDirect(args: LaunchWorkItemDirectArgs): Prom linkedIssueNumber: item.type === 'issue' ? (item.number ?? null) : null, linkedPR: item.type === 'pr' ? (item.number ?? null) : null }) + let resolvedBaseBranch = baseBranch + let resolvedPushTarget: GitPushTarget | undefined + if (!resolvedBaseBranch && item.type === 'pr' && item.number) { + try { + // Why: direct "Use PR" launches bypass the Start-from picker, so they + // must still resolve the PR head before `git worktree add`. + const result = await resolveDirectPrStartPoint(repoId, item.number, settings) + resolvedBaseBranch = result.baseBranch + resolvedPushTarget = result.pushTarget + } catch (error) { + toast.error(error instanceof Error ? error.message : 'Failed to resolve PR head.') + openModalFallback() + return + } + } let worktreeId: string let primaryTabId: string | null @@ -217,14 +252,14 @@ export async function launchWorkItemDirect(args: LaunchWorkItemDirectArgs): Prom const result = await store.createWorktree( repoId, workspaceName, - baseBranch, + resolvedBaseBranch, finalSetupDecision, undefined, telemetrySource, item.title, item.type === 'issue' && item.number ? item.number : undefined, item.type === 'pr' && item.number ? item.number : undefined, - undefined, + resolvedPushTarget, undefined, item.linearIdentifier )