From ccd72f5909ac87dec5a42e523b15935d9cfebcb4 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 20 Jul 2026 11:11:23 -0700 Subject: [PATCH 1/5] Add unified mobile onboarding for session view and notifications (#9478) * Add a mobile native-chat opt-in so users pick terminal vs chat once Mirror the notifications one-time opt-in for the native-chat default view. After pairing, a full-screen modal (modeled on notification-opt-in) lets the user choose whether supported agent sessions open in the terminal or in native chat, then persists the choice to the existing orca:defaultSessionView key. - Expose readDefaultSessionViewPreference() (tri-state; absent key = undecided) so the gate can prompt exactly once; loadDefaultSessionView() is unchanged. - shouldPresentSessionViewOptIn() gates the screen; the home focus effect shows it after the notification opt-in. - Settings -> Native chat toggle (already shipped) remains the recovery path. * fix(mobile): preserve onboarding flow after pairing * refine mobile session view opt-in copy * Unify mobile onboarding prompts --- mobile/app/_layout.tsx | 2 +- mobile/app/index.tsx | 22 +- mobile/app/mobile-onboarding.tsx | 215 ++++++++++++++++ mobile/app/notification-opt-in.tsx | 236 ------------------ mobile/app/pair-confirm.tsx | 13 +- mobile/app/pair-scan.tsx | 13 +- .../onboarding/MobileOnboardingPage.test.ts | 103 ++++++++ .../src/onboarding/MobileOnboardingPage.tsx | 173 +++++++++++++ .../onboarding/mobile-onboarding-plan.test.ts | 75 ++++++ .../src/onboarding/mobile-onboarding-plan.ts | 51 ++++ .../mobile-onboarding-screen.test.ts | 165 ++++++++++++ .../onboarding/mobile-onboarding-styles.ts | 132 ++++++++++ .../session/session-view-opt-in-gate.test.ts | 49 ++++ .../src/session/session-view-opt-in-gate.ts | 10 + mobile/src/storage/preferences.test.ts | 34 +++ .../src/storage/session-view-preferences.ts | 23 +- 16 files changed, 1054 insertions(+), 262 deletions(-) create mode 100644 mobile/app/mobile-onboarding.tsx delete mode 100644 mobile/app/notification-opt-in.tsx create mode 100644 mobile/src/onboarding/MobileOnboardingPage.test.ts create mode 100644 mobile/src/onboarding/MobileOnboardingPage.tsx create mode 100644 mobile/src/onboarding/mobile-onboarding-plan.test.ts create mode 100644 mobile/src/onboarding/mobile-onboarding-plan.ts create mode 100644 mobile/src/onboarding/mobile-onboarding-screen.test.ts create mode 100644 mobile/src/onboarding/mobile-onboarding-styles.ts create mode 100644 mobile/src/session/session-view-opt-in-gate.test.ts create mode 100644 mobile/src/session/session-view-opt-in-gate.ts diff --git a/mobile/app/_layout.tsx b/mobile/app/_layout.tsx index fe89a82c4a0..d5cbd0e0f1b 100644 --- a/mobile/app/_layout.tsx +++ b/mobile/app/_layout.tsx @@ -179,7 +179,7 @@ export default function RootLayout() { diff --git a/mobile/app/index.tsx b/mobile/app/index.tsx index 265e3413c42..a8909c372b7 100644 --- a/mobile/app/index.tsx +++ b/mobile/app/index.tsx @@ -36,7 +36,10 @@ import { } from '../src/transport/client-context' import { classifyConnection } from '../src/transport/connection-health' import { subscribeToDesktopNotifications } from '../src/notifications/mobile-notifications' -import { shouldPresentNotificationOptIn } from '../src/notifications/notification-opt-in-gate' +import { + loadMobileOnboardingSteps, + mobileOnboardingDestination +} from '../src/onboarding/mobile-onboarding-plan' import type { ConnectionState, HostProfile } from '../src/transport/types' import { triggerMediumImpact } from '../src/platform/haptics' import { OrcaLogo } from '../src/components/OrcaLogo' @@ -306,7 +309,9 @@ export default function HomeScreen() { const [lastVisited, setLastVisited] = useState<{ hostId: string; worktreeId: string } | null>( null ) - const notificationOptInCheckedRef = useRef(false) + // Why: focus can fire repeatedly while an async gate is pending; one probe per + // mount avoids duplicate storage/permission reads and competing navigation. + const onboardingOptInCheckedRef = useRef(false) // Why: shared clients from the per-host store, not N independent WebSockets. See docs/mobile-shared-client-per-host.md. const hostIds = useMemo(() => hosts.map((h) => h.id), [hosts]) @@ -378,13 +383,16 @@ export default function HomeScreen() { return } setHosts(h) - if (h.length === 0 || notificationOptInCheckedRef.current) { + if (h.length === 0 || onboardingOptInCheckedRef.current) { return } - notificationOptInCheckedRef.current = true - const showNotificationOptIn = await shouldPresentNotificationOptIn() - if (!stale && showNotificationOptIn) { - router.replace('/notification-opt-in') + onboardingOptInCheckedRef.current = true + const onboardingSteps = await loadMobileOnboardingSteps() + if (stale) { + return + } + if (onboardingSteps.length > 0) { + router.replace(mobileOnboardingDestination(onboardingSteps)) } }) void AsyncStorage.getItem('orca:last-visited-worktree').then((raw) => { diff --git a/mobile/app/mobile-onboarding.tsx b/mobile/app/mobile-onboarding.tsx new file mode 100644 index 00000000000..50957a465fc --- /dev/null +++ b/mobile/app/mobile-onboarding.tsx @@ -0,0 +1,215 @@ +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' +import { + AccessibilityInfo, + Animated, + BackHandler, + Text, + useWindowDimensions, + View +} from 'react-native' +import { useFocusEffect, useLocalSearchParams, useRouter } from 'expo-router' +import { SafeAreaView } from 'react-native-safe-area-context' +import { OrcaLogo } from '../src/components/OrcaLogo' +import { ensureNotificationPermissions } from '../src/notifications/mobile-notifications' +import { + MobileOnboardingPage, + type MobileOnboardingBusyChoice, + type NotificationOnboardingChoice +} from '../src/onboarding/MobileOnboardingPage' +import { parseMobileOnboardingSteps } from '../src/onboarding/mobile-onboarding-plan' +import { mobileOnboardingStyles as styles } from '../src/onboarding/mobile-onboarding-styles' +import { + saveDefaultSessionView, + type MobileSessionView +} from '../src/storage/session-view-preferences' +import { savePushNotificationsEnabled } from '../src/storage/preferences' + +const SLIDE_DURATION_MS = 280 + +export default function MobileOnboardingScreen() { + const params = useLocalSearchParams<{ + hostId?: string | string[] + steps?: string | string[] + }>() + const hostId = firstParam(params.hostId) + const rawSteps = firstParam(params.steps) + + return ( + + ) +} + +function MobileOnboardingFlow({ + hostId, + rawSteps +}: { + hostId: string | undefined + rawSteps: string | undefined +}) { + const router = useRouter() + const steps = useMemo(() => parseMobileOnboardingSteps(rawSteps), [rawSteps]) + const { width } = useWindowDimensions() + const [activeIndex, setActiveIndex] = useState(0) + const [busyChoice, setBusyChoice] = useState(null) + const [error, setError] = useState(null) + const choiceInFlightRef = useRef(false) + const slideProgress = useRef(new Animated.Value(0)).current + const reducedMotionEnabled = useReducedMotionEnabled() + + // Why: onboarding requires an explicit choice for every planned step; disabling + // stack gestures alone still leaves Android hardware Back able to skip the flow. + useFocusEffect( + useCallback(() => { + const subscription = BackHandler.addEventListener('hardwareBackPress', () => true) + return () => subscription.remove() + }, []) + ) + + const continueToApp = useCallback(() => { + router.replace(hostId ? `/h/${hostId}` : '/') + }, [hostId, router]) + + const advanceOrContinue = useCallback(() => { + const nextIndex = activeIndex + 1 + if (nextIndex >= steps.length) { + continueToApp() + return + } + setActiveIndex(nextIndex) + setError(null) + Animated.timing(slideProgress, { + toValue: nextIndex, + // Why: the carousel should preserve continuity without overriding the + // device's reduced-motion preference. + duration: reducedMotionEnabled ? 0 : SLIDE_DURATION_MS, + useNativeDriver: true + }).start(() => { + // Why: a cancelled cosmetic transition must not leave the next decision + // permanently disabled after the previous choice was already persisted. + setBusyChoice(null) + choiceInFlightRef.current = false + }) + }, [activeIndex, continueToApp, reducedMotionEnabled, slideProgress, steps.length]) + + const chooseSessionView = useCallback( + async (view: MobileSessionView) => { + // Why: state does not disable both buttons synchronously; the ref prevents + // rapid taps from persisting conflicting choices or advancing twice. + if (choiceInFlightRef.current) { + return + } + choiceInFlightRef.current = true + setBusyChoice(view) + setError(null) + try { + await saveDefaultSessionView(view) + advanceOrContinue() + } catch { + setError('Your choice could not be saved. Try again.') + setBusyChoice(null) + choiceInFlightRef.current = false + } + }, + [advanceOrContinue] + ) + + const chooseNotifications = useCallback( + async (choice: NotificationOnboardingChoice) => { + if (choiceInFlightRef.current) { + return + } + choiceInFlightRef.current = true + setBusyChoice(choice) + setError(null) + try { + const enabled = choice === 'enable' ? await ensureNotificationPermissions() : false + await savePushNotificationsEnabled(enabled) + advanceOrContinue() + } catch { + setError('Notification settings could not be updated. Try again.') + setBusyChoice(null) + choiceInFlightRef.current = false + } + }, + [advanceOrContinue] + ) + + const translateX = useMemo(() => Animated.multiply(slideProgress, -width), [slideProgress, width]) + + return ( + + + + Orca + {steps.length > 1 ? ( + + {steps.map((step, index) => ( + + ))} + + ) : null} + + + + + {steps.map((step, index) => ( + void chooseSessionView(view)} + onNotificationChoice={(choice) => void chooseNotifications(choice)} + /> + ))} + + + + ) +} + +function firstParam(value: string | string[] | undefined): string | undefined { + return Array.isArray(value) ? value[0] : value +} + +function useReducedMotionEnabled(): boolean { + const [enabled, setEnabled] = useState(false) + + useEffect(() => { + let mounted = true + void AccessibilityInfo.isReduceMotionEnabled() + .then((nextEnabled) => { + if (mounted) { + setEnabled(nextEnabled) + } + }) + .catch(() => undefined) + const subscription = AccessibilityInfo.addEventListener('reduceMotionChanged', setEnabled) + return () => { + mounted = false + subscription.remove() + } + }, []) + + return enabled +} diff --git a/mobile/app/notification-opt-in.tsx b/mobile/app/notification-opt-in.tsx deleted file mode 100644 index a4476687e09..00000000000 --- a/mobile/app/notification-opt-in.tsx +++ /dev/null @@ -1,236 +0,0 @@ -import { useCallback, useState } from 'react' -import { - ActivityIndicator, - BackHandler, - Pressable, - ScrollView, - StyleSheet, - Text, - View -} from 'react-native' -import { useFocusEffect, useLocalSearchParams, useRouter } from 'expo-router' -import { SafeAreaView } from 'react-native-safe-area-context' -import { BellRing } from 'lucide-react-native' -import { OrcaLogo } from '../src/components/OrcaLogo' -import { ensureNotificationPermissions } from '../src/notifications/mobile-notifications' -import { savePushNotificationsEnabled } from '../src/storage/preferences' -import { colors, radii, spacing, typography } from '../src/theme/mobile-theme' - -export default function NotificationOptInScreen() { - const router = useRouter() - const params = useLocalSearchParams<{ hostId?: string | string[] }>() - const hostId = Array.isArray(params.hostId) ? params.hostId[0] : params.hostId - const [busyChoice, setBusyChoice] = useState<'enable' | 'skip' | null>(null) - const [error, setError] = useState(null) - - // Why: this one-time screen requires an explicit Enable or Not now choice; - // disabling back gestures alone would still leave Android hardware back open. - useFocusEffect( - useCallback(() => { - const subscription = BackHandler.addEventListener('hardwareBackPress', () => true) - return () => subscription.remove() - }, []) - ) - - const continueToApp = useCallback(() => { - router.replace(hostId ? `/h/${hostId}` : '/') - }, [hostId, router]) - - const choose = useCallback( - async (choice: 'enable' | 'skip') => { - if (busyChoice) { - return - } - setBusyChoice(choice) - setError(null) - try { - const enabled = choice === 'enable' ? await ensureNotificationPermissions() : false - await savePushNotificationsEnabled(enabled) - continueToApp() - } catch { - setError('Notification settings could not be updated. Try again.') - setBusyChoice(null) - } - }, - [busyChoice, continueToApp] - ) - - return ( - - - - - Orca - - - - - - - Notifications - Stay updated while away - - Get notified on this device when an agent needs your input or finishes a task. - - - - - {error ? ( - - {error} - - ) : null} - [ - styles.primaryButton, - pressed && styles.buttonPressed, - busyChoice !== null && styles.buttonDisabled - ]} - onPress={() => void choose('enable')} - > - {busyChoice === 'enable' ? ( - - ) : ( - Enable notifications - )} - - [ - styles.secondaryButton, - pressed && styles.buttonPressed, - busyChoice !== null && styles.buttonDisabled - ]} - onPress={() => void choose('skip')} - > - {busyChoice === 'skip' ? ( - - ) : ( - Not now - )} - - You can change this any time in Settings. - - - - ) -} - -const styles = StyleSheet.create({ - container: { - flex: 1, - backgroundColor: colors.bgBase, - paddingHorizontal: spacing.xl - }, - // Why: this decision screen cannot be dismissed with Back, so every action - // must remain reachable in landscape and with accessibility text scaling. - scrollContent: { - flexGrow: 1 - }, - brandRow: { - minHeight: 52, - flexDirection: 'row', - alignItems: 'center', - gap: spacing.sm - }, - brandName: { - color: colors.textPrimary, - fontSize: 17, - fontWeight: '700' - }, - content: { - flexGrow: 1, - alignItems: 'center', - justifyContent: 'center', - paddingVertical: spacing.xl - }, - iconSurface: { - width: 64, - height: 64, - borderRadius: radii.card, - alignItems: 'center', - justifyContent: 'center', - backgroundColor: colors.bgRaised, - marginBottom: spacing.xl - }, - eyebrow: { - color: colors.textMuted, - fontSize: 11, - fontWeight: '600', - letterSpacing: 0.55, - textTransform: 'uppercase', - marginBottom: spacing.sm - }, - title: { - maxWidth: 420, - color: colors.textPrimary, - fontSize: 26, - fontWeight: '700', - letterSpacing: -0.3, - textAlign: 'center' - }, - body: { - maxWidth: 420, - color: colors.textSecondary, - fontSize: typography.bodySize, - lineHeight: 21, - textAlign: 'center', - marginTop: spacing.md - }, - footer: { - width: '100%', - maxWidth: 420, - alignSelf: 'center', - paddingBottom: spacing.lg - }, - primaryButton: { - minHeight: 44, - alignItems: 'center', - justifyContent: 'center', - borderRadius: radii.button, - backgroundColor: colors.surfaceBright, - paddingVertical: spacing.sm - }, - primaryButtonText: { - color: colors.bgBase, - fontSize: typography.bodySize, - fontWeight: '600' - }, - secondaryButton: { - minHeight: 44, - alignItems: 'center', - justifyContent: 'center', - borderRadius: radii.button, - marginTop: spacing.xs, - paddingVertical: spacing.sm - }, - secondaryButtonText: { - color: colors.textSecondary, - fontSize: typography.bodySize, - fontWeight: '500' - }, - buttonPressed: { - opacity: 0.72 - }, - buttonDisabled: { - opacity: 0.58 - }, - footerNote: { - color: colors.textMuted, - fontSize: typography.metaSize, - lineHeight: 18, - textAlign: 'center', - marginTop: spacing.sm - }, - error: { - color: colors.statusRed, - fontSize: typography.metaSize, - lineHeight: 18, - textAlign: 'center', - marginBottom: spacing.sm - } -}) diff --git a/mobile/app/pair-confirm.tsx b/mobile/app/pair-confirm.tsx index d4505b82411..0621caa4f01 100644 --- a/mobile/app/pair-confirm.tsx +++ b/mobile/app/pair-confirm.tsx @@ -12,7 +12,10 @@ import type { ConnectionLogEntry } from '../src/transport/types' import { useCloseHost } from '../src/transport/client-context' import { colors, spacing, radii, typography } from '../src/theme/mobile-theme' import { ConnectionLog } from '../src/components/ConnectionLog' -import { shouldPresentNotificationOptIn } from '../src/notifications/notification-opt-in-gate' +import { + loadMobileOnboardingSteps, + mobileOnboardingDestination +} from '../src/onboarding/mobile-onboarding-plan' type Status = 'awaiting-confirm' | 'connecting' | 'error' @@ -113,15 +116,11 @@ export default function PairConfirmScreen() { // profile — the removeHost() path already refreshes on re-pair, and a // brand-new host has no cached entry so this is a no-op. closeHost(hostId) - const showNotificationOptIn = await shouldPresentNotificationOptIn() + const onboardingSteps = await loadMobileOnboardingSteps() if (!mountedRef.current) { return } - router.replace( - showNotificationOptIn - ? { pathname: '/notification-opt-in', params: { hostId } } - : `/h/${hostId}` - ) + router.replace(mobileOnboardingDestination(onboardingSteps, hostId)) } catch (err) { const timedOut = attempt.timedOut const attemptIsCurrent = activePairingAttemptRef.current === attempt diff --git a/mobile/app/pair-scan.tsx b/mobile/app/pair-scan.tsx index 6b980ee0be6..892c1b359a1 100644 --- a/mobile/app/pair-scan.tsx +++ b/mobile/app/pair-scan.tsx @@ -22,7 +22,10 @@ import { useCloseHost } from '../src/transport/client-context' import { colors, spacing, radii, typography } from '../src/theme/mobile-theme' import { TextInputModal } from '../src/components/TextInputModal' import { ConnectionLog } from '../src/components/ConnectionLog' -import { shouldPresentNotificationOptIn } from '../src/notifications/notification-opt-in-gate' +import { + loadMobileOnboardingSteps, + mobileOnboardingDestination +} from '../src/onboarding/mobile-onboarding-plan' // Why: see pair-confirm.tsx — cap initial-pair "Connecting…" so a broken // route surfaces as a real error with the log visible instead of a @@ -157,15 +160,11 @@ export default function PairScanScreen() { // profile — the removeHost() path already refreshes on re-pair, and a // brand-new host has no cached entry so this is a no-op. closeHost(hostId) - const showNotificationOptIn = await shouldPresentNotificationOptIn() + const onboardingSteps = await loadMobileOnboardingSteps() if (!mountedRef.current) { return } - router.replace( - showNotificationOptIn - ? { pathname: '/notification-opt-in', params: { hostId } } - : `/h/${hostId}` - ) + router.replace(mobileOnboardingDestination(onboardingSteps, hostId)) } catch (err) { const timedOut = attempt.timedOut const attemptIsCurrent = activePairingAttemptRef.current === attempt diff --git a/mobile/src/onboarding/MobileOnboardingPage.test.ts b/mobile/src/onboarding/MobileOnboardingPage.test.ts new file mode 100644 index 00000000000..f3683105160 --- /dev/null +++ b/mobile/src/onboarding/MobileOnboardingPage.test.ts @@ -0,0 +1,103 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { MobileOnboardingPage } from './MobileOnboardingPage' + +vi.mock('react-native', async () => { + const React = await import('react') + return { + ActivityIndicator: 'ActivityIndicator', + Pressable: 'Pressable', + ScrollView: ({ children, ...props }: { children?: unknown }) => + React.createElement('ScrollView', props, children), + StyleSheet: { create: (styles: unknown) => styles }, + Text: 'Text', + View: 'View' + } +}) + +vi.mock('lucide-react-native', () => ({ + BellRing: 'BellRing', + MessageSquare: 'MessageSquare' +})) + +describe('MobileOnboardingPage', () => { + let renderer: ReactTestRenderer | null = null + + beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + }) + + afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + vi.restoreAllMocks() + }) + + async function renderPage( + step: 'session-view' | 'notifications', + options: { active?: boolean; busyChoice?: 'chat' | 'enable' | null } = {} + ) { + const onSessionChoice = vi.fn() + const onNotificationChoice = vi.fn() + const consoleError = vi.spyOn(console, 'error').mockImplementation((...args) => { + if (typeof args[0] !== 'string' || !args[0].includes('react-test-renderer is deprecated')) { + throw new Error(String(args[0])) + } + }) + await act(async () => { + renderer = create( + createElement(MobileOnboardingPage, { + step, + width: 390, + active: options.active ?? true, + busyChoice: options.busyChoice ?? null, + error: null, + onSessionChoice, + onNotificationChoice + }) + ) + }) + consoleError.mockRestore() + return { onSessionChoice, onNotificationChoice } + } + + function button(label: string) { + return renderer!.root.find( + (node) => node.type === 'Pressable' && node.props.accessibilityLabel === label + ) + } + + it('renders the session choices and sends exactly one selected view', async () => { + const callbacks = await renderPage('session-view') + + act(() => button('Open sessions in native chat').props.onPress()) + expect(callbacks.onSessionChoice).toHaveBeenCalledWith('chat') + expect(callbacks.onNotificationChoice).not.toHaveBeenCalled() + }) + + it('renders the notification choices and sends the selected option', async () => { + const callbacks = await renderPage('notifications') + act(() => button('Skip notifications for now').props.onPress()) + + expect(callbacks.onNotificationChoice).toHaveBeenCalledWith('skip') + expect(callbacks.onSessionChoice).not.toHaveBeenCalled() + }) + + it('disables both notification choices while permission is pending', async () => { + await renderPage('notifications', { busyChoice: 'enable' }) + const enable = button('Enable agent notifications') + const secondary = button('Skip notifications for now') + + expect(enable.props.disabled).toBe(true) + expect(secondary.props.disabled).toBe(true) + }) + + it('hides an off-screen page from assistive technology', async () => { + await renderPage('notifications', { active: false }) + const scrollView = renderer!.root.findByType('ScrollView') + + expect(scrollView.props.accessibilityElementsHidden).toBe(true) + expect(scrollView.props.importantForAccessibility).toBe('no-hide-descendants') + }) +}) diff --git a/mobile/src/onboarding/MobileOnboardingPage.tsx b/mobile/src/onboarding/MobileOnboardingPage.tsx new file mode 100644 index 00000000000..f7bbff0cf0d --- /dev/null +++ b/mobile/src/onboarding/MobileOnboardingPage.tsx @@ -0,0 +1,173 @@ +import { ActivityIndicator, Pressable, ScrollView, Text, View } from 'react-native' +import { BellRing, MessageSquare } from 'lucide-react-native' +import type { MobileOnboardingStep } from './mobile-onboarding-plan' +import { mobileOnboardingStyles as styles } from './mobile-onboarding-styles' +import type { MobileSessionView } from '../storage/session-view-preferences' +import { colors } from '../theme/mobile-theme' + +export type NotificationOnboardingChoice = 'enable' | 'skip' +export type MobileOnboardingBusyChoice = MobileSessionView | NotificationOnboardingChoice | null + +type Props = { + step: MobileOnboardingStep + width: number + active: boolean + busyChoice: MobileOnboardingBusyChoice + error: string | null + onSessionChoice: (view: MobileSessionView) => void + onNotificationChoice: (choice: NotificationOnboardingChoice) => void +} + +export function MobileOnboardingPage({ + step, + width, + active, + busyChoice, + error, + onSessionChoice, + onNotificationChoice +}: Props) { + const busy = busyChoice !== null + const isSessionView = step === 'session-view' + + return ( + + + + {isSessionView ? ( + + ) : ( + + )} + + + {isSessionView ? 'How should sessions open?' : 'Stay updated while away'} + + + {isSessionView + ? 'Choose whether supported agent sessions open in the terminal or native chat on this device. Press and hold a session tab to switch its view, or change the default later in Settings.' + : 'Get notified on this device when an agent needs your input or finishes a task.'} + + + + + {error ? ( + + {error} + + ) : null} + {isSessionView ? ( + + ) : ( + + )} + + + ) +} + +function SessionViewChoices({ + busyChoice, + disabled, + onChoice +}: { + busyChoice: MobileOnboardingBusyChoice + disabled: boolean + onChoice: (view: MobileSessionView) => void +}) { + return ( + <> + onChoice('chat')} + /> + onChoice('terminal')} + /> + > + ) +} + +function NotificationChoices({ + busyChoice, + disabled, + onChoice +}: { + busyChoice: MobileOnboardingBusyChoice + disabled: boolean + onChoice: (choice: NotificationOnboardingChoice) => void +}) { + return ( + <> + onChoice('enable')} + /> + onChoice('skip')} + /> + > + ) +} + +function ChoiceButton({ + label, + accessibilityLabel, + primary = false, + busy, + disabled, + onPress +}: { + label: string + accessibilityLabel?: string + primary?: boolean + busy: boolean + disabled: boolean + onPress: () => void +}) { + return ( + [ + primary ? styles.primaryButton : styles.secondaryButton, + pressed && styles.buttonPressed, + disabled && styles.buttonDisabled + ]} + onPress={onPress} + > + {busy ? ( + + ) : ( + {label} + )} + + ) +} diff --git a/mobile/src/onboarding/mobile-onboarding-plan.test.ts b/mobile/src/onboarding/mobile-onboarding-plan.test.ts new file mode 100644 index 00000000000..d04396bdef7 --- /dev/null +++ b/mobile/src/onboarding/mobile-onboarding-plan.test.ts @@ -0,0 +1,75 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { shouldPresentNotificationOptIn } from '../notifications/notification-opt-in-gate' +import { shouldPresentSessionViewOptIn } from '../session/session-view-opt-in-gate' +import { + loadMobileOnboardingSteps, + mobileOnboardingDestination, + parseMobileOnboardingSteps +} from './mobile-onboarding-plan' + +vi.mock('../notifications/notification-opt-in-gate', () => ({ + shouldPresentNotificationOptIn: vi.fn() +})) +vi.mock('../session/session-view-opt-in-gate', () => ({ + shouldPresentSessionViewOptIn: vi.fn() +})) + +describe('mobile onboarding plan', () => { + beforeEach(() => { + vi.mocked(shouldPresentNotificationOptIn).mockReset().mockResolvedValue(false) + vi.mocked(shouldPresentSessionViewOptIn).mockReset().mockResolvedValue(false) + }) + + it.each([ + [true, true, ['session-view', 'notifications']], + [true, false, ['session-view']], + [false, true, ['notifications']], + [false, false, []] + ] as const)( + 'builds the exact plan for session=%s notifications=%s', + async (showSession, showNotifications, expected) => { + vi.mocked(shouldPresentSessionViewOptIn).mockResolvedValue(showSession) + vi.mocked(shouldPresentNotificationOptIn).mockResolvedValue(showNotifications) + + await expect(loadMobileOnboardingSteps()).resolves.toEqual(expected) + expect(shouldPresentSessionViewOptIn).toHaveBeenCalledOnce() + expect(shouldPresentNotificationOptIn).toHaveBeenCalledOnce() + } + ) + + it.each([ + [[], undefined, '/'], + [[], 'paired-host', '/h/paired-host'], + [ + ['session-view', 'notifications'], + undefined, + { + pathname: '/mobile-onboarding', + params: { steps: 'session-view,notifications' } + } + ], + [ + ['notifications'], + 'paired-host', + { + pathname: '/mobile-onboarding', + params: { steps: 'notifications', hostId: 'paired-host' } + } + ] + ] as const)('maps %j with host %s to the correct destination', (steps, hostId, destination) => { + expect(mobileOnboardingDestination(steps, hostId)).toEqual(destination) + }) + + it('parses route steps in canonical order without duplicates', () => { + expect(parseMobileOnboardingSteps('notifications,unknown,session-view,notifications')).toEqual([ + 'session-view', + 'notifications' + ]) + expect(parseMobileOnboardingSteps('notifications')).toEqual(['notifications']) + }) + + it('defaults missing or invalid route state to the complete wizard', () => { + expect(parseMobileOnboardingSteps(undefined)).toEqual(['session-view', 'notifications']) + expect(parseMobileOnboardingSteps('unknown')).toEqual(['session-view', 'notifications']) + }) +}) diff --git a/mobile/src/onboarding/mobile-onboarding-plan.ts b/mobile/src/onboarding/mobile-onboarding-plan.ts new file mode 100644 index 00000000000..3435034c467 --- /dev/null +++ b/mobile/src/onboarding/mobile-onboarding-plan.ts @@ -0,0 +1,51 @@ +import { shouldPresentNotificationOptIn } from '../notifications/notification-opt-in-gate' +import { shouldPresentSessionViewOptIn } from '../session/session-view-opt-in-gate' + +export const MOBILE_ONBOARDING_STEPS = ['session-view', 'notifications'] as const +export type MobileOnboardingStep = (typeof MOBILE_ONBOARDING_STEPS)[number] +export type MobileOnboardingDestination = + | '/' + | `/h/${string}` + | { + pathname: '/mobile-onboarding' + params: { steps: string; hostId?: string } + } + +/** Loads every outstanding decision in the order the wizard presents them. */ +export async function loadMobileOnboardingSteps(): Promise { + // Why: the wizard needs the complete plan for accurate progress dots; run the + // independent gates together so adding the second decision does not add latency. + const [showSessionView, showNotifications] = await Promise.all([ + shouldPresentSessionViewOptIn(), + shouldPresentNotificationOptIn() + ]) + return MOBILE_ONBOARDING_STEPS.filter( + (step) => + (step === 'session-view' && showSessionView) || + (step === 'notifications' && showNotifications) + ) +} + +/** Preserves a paired host while routing through outstanding decisions. */ +export function mobileOnboardingDestination( + steps: readonly MobileOnboardingStep[], + hostId?: string +): MobileOnboardingDestination { + if (steps.length === 0) { + return hostId ? `/h/${hostId}` : '/' + } + return { + pathname: '/mobile-onboarding', + params: { steps: steps.join(','), ...(hostId ? { hostId } : {}) } + } +} + +/** Restores the canonical order and ignores duplicate or unknown route values. */ +export function parseMobileOnboardingSteps(raw: string | undefined): MobileOnboardingStep[] { + if (!raw) { + return [...MOBILE_ONBOARDING_STEPS] + } + const requested = new Set(raw.split(',')) + const steps = MOBILE_ONBOARDING_STEPS.filter((step) => requested.has(step)) + return steps.length > 0 ? steps : [...MOBILE_ONBOARDING_STEPS] +} diff --git a/mobile/src/onboarding/mobile-onboarding-screen.test.ts b/mobile/src/onboarding/mobile-onboarding-screen.test.ts new file mode 100644 index 00000000000..d571ce28a01 --- /dev/null +++ b/mobile/src/onboarding/mobile-onboarding-screen.test.ts @@ -0,0 +1,165 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import MobileOnboardingScreen from '../../app/mobile-onboarding' + +const mocks = vi.hoisted(() => ({ + params: { hostId: 'paired-host', steps: 'session-view,notifications' }, + replace: vi.fn(), + reducedMotionEnabled: false, + animatedTiming: vi.fn(), + ensureNotificationPermissions: vi.fn(), + saveDefaultSessionView: vi.fn(), + savePushNotificationsEnabled: vi.fn() +})) + +vi.mock('react-native', () => ({ + AccessibilityInfo: { + addEventListener: vi.fn(() => ({ remove: vi.fn() })), + isReduceMotionEnabled: vi.fn(() => Promise.resolve(mocks.reducedMotionEnabled)) + }, + Animated: { + Value: class {}, + View: 'AnimatedView', + multiply: vi.fn(() => 0), + timing: mocks.animatedTiming + }, + BackHandler: { + addEventListener: vi.fn(() => ({ remove: vi.fn() })) + }, + StyleSheet: { create: (styles: unknown) => styles }, + Text: 'Text', + View: 'View', + useWindowDimensions: () => ({ width: 390, height: 844 }) +})) + +vi.mock('expo-router', () => ({ + useFocusEffect: vi.fn(), + useLocalSearchParams: () => mocks.params, + useRouter: () => ({ replace: mocks.replace }) +})) + +vi.mock('react-native-safe-area-context', () => ({ SafeAreaView: 'SafeAreaView' })) +vi.mock('../components/OrcaLogo', () => ({ OrcaLogo: 'OrcaLogo' })) +vi.mock('./MobileOnboardingPage', () => ({ MobileOnboardingPage: 'MobileOnboardingPage' })) +vi.mock('../notifications/mobile-notifications', () => ({ + ensureNotificationPermissions: mocks.ensureNotificationPermissions +})) +vi.mock('../storage/session-view-preferences', () => ({ + saveDefaultSessionView: mocks.saveDefaultSessionView +})) +vi.mock('../storage/preferences', () => ({ + savePushNotificationsEnabled: mocks.savePushNotificationsEnabled +})) + +describe('MobileOnboardingScreen', () => { + let renderer: ReactTestRenderer | null = null + + beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + mocks.params = { hostId: 'paired-host', steps: 'session-view,notifications' } + mocks.replace.mockReset() + mocks.reducedMotionEnabled = false + mocks.animatedTiming.mockReset().mockReturnValue({ + start: (callback: (result: { finished: boolean }) => void) => callback({ finished: true }) + }) + mocks.ensureNotificationPermissions.mockReset().mockResolvedValue(true) + mocks.saveDefaultSessionView.mockReset().mockResolvedValue(undefined) + mocks.savePushNotificationsEnabled.mockReset().mockResolvedValue(undefined) + }) + + afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + vi.restoreAllMocks() + }) + + async function renderScreen() { + const consoleError = vi.spyOn(console, 'error').mockImplementation((...args) => { + if (typeof args[0] !== 'string' || !args[0].includes('react-test-renderer is deprecated')) { + throw new Error(String(args[0])) + } + }) + await act(async () => { + renderer = create(createElement(MobileOnboardingScreen)) + }) + consoleError.mockRestore() + } + + function pages() { + return renderer!.root.findAllByType('MobileOnboardingPage') + } + + it('advances from session view to notifications before opening the paired host', async () => { + await renderScreen() + expect(pages().map((page) => page.props.active)).toEqual([true, false]) + + await act(async () => pages()[0].props.onSessionChoice('chat')) + expect(mocks.saveDefaultSessionView).toHaveBeenCalledWith('chat') + expect(pages().map((page) => page.props.active)).toEqual([false, true]) + expect(mocks.replace).not.toHaveBeenCalled() + + await act(async () => pages()[1].props.onNotificationChoice('skip')) + expect(mocks.ensureNotificationPermissions).not.toHaveBeenCalled() + expect(mocks.savePushNotificationsEnabled).toHaveBeenCalledWith(false) + expect(mocks.replace).toHaveBeenCalledWith('/h/paired-host') + }) + + it('finishes immediately when the plan contains only one outstanding step', async () => { + mocks.params = { hostId: 'paired-host', steps: 'session-view' } + await renderScreen() + + await act(async () => pages()[0].props.onSessionChoice('terminal')) + expect(mocks.replace).toHaveBeenCalledWith('/h/paired-host') + }) + + it('keeps the current step retryable when persistence fails', async () => { + mocks.params = { hostId: 'paired-host', steps: 'session-view' } + mocks.saveDefaultSessionView + .mockRejectedValueOnce(new Error('storage unavailable')) + .mockResolvedValueOnce(undefined) + await renderScreen() + + await act(async () => pages()[0].props.onSessionChoice('chat')) + expect(pages()[0].props.error).toBe('Your choice could not be saved. Try again.') + + await act(async () => pages()[0].props.onSessionChoice('chat')) + expect(mocks.saveDefaultSessionView).toHaveBeenCalledTimes(2) + expect(mocks.replace).toHaveBeenCalledWith('/h/paired-host') + }) + + it('resets carousel state when the route supplies a new onboarding plan', async () => { + await renderScreen() + await act(async () => pages()[0].props.onSessionChoice('chat')) + expect(pages().map((page) => page.props.active)).toEqual([false, true]) + + mocks.params = { hostId: 'paired-host', steps: 'notifications' } + await act(async () => renderer!.update(createElement(MobileOnboardingScreen))) + + expect(pages()).toHaveLength(1) + expect(pages()[0].props).toMatchObject({ step: 'notifications', active: true }) + }) + + it('skips the slide animation when the device requests reduced motion', async () => { + mocks.reducedMotionEnabled = true + await renderScreen() + + await act(async () => pages()[0].props.onSessionChoice('chat')) + + expect(mocks.animatedTiming).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ duration: 0, useNativeDriver: true }) + ) + }) + + it('keeps the next decision available if the cosmetic transition is interrupted', async () => { + mocks.animatedTiming.mockReturnValue({ + start: (callback: (result: { finished: boolean }) => void) => callback({ finished: false }) + }) + await renderScreen() + + await act(async () => pages()[0].props.onSessionChoice('chat')) + + expect(pages()[1].props).toMatchObject({ active: true, busyChoice: null }) + }) +}) diff --git a/mobile/src/onboarding/mobile-onboarding-styles.ts b/mobile/src/onboarding/mobile-onboarding-styles.ts new file mode 100644 index 00000000000..20f36ec0f5b --- /dev/null +++ b/mobile/src/onboarding/mobile-onboarding-styles.ts @@ -0,0 +1,132 @@ +import { StyleSheet } from 'react-native' +import { colors, radii, spacing, typography } from '../theme/mobile-theme' + +export const mobileOnboardingStyles = StyleSheet.create({ + container: { + flex: 1, + backgroundColor: colors.bgBase + }, + brandRow: { + minHeight: 52, + flexDirection: 'row', + alignItems: 'center', + gap: spacing.sm, + paddingHorizontal: spacing.xl + }, + brandName: { + color: colors.textPrimary, + fontSize: 17, + fontWeight: '700' + }, + progress: { + position: 'absolute', + left: '50%', + flexDirection: 'row', + alignItems: 'center', + gap: 7, + transform: [{ translateX: -18 }] + }, + progressDot: { + width: 7, + height: 7, + borderRadius: 999, + backgroundColor: colors.borderSubtle + }, + progressDotActive: { + width: 22, + backgroundColor: colors.textPrimary + }, + carouselViewport: { + flex: 1, + overflow: 'hidden' + }, + carouselTrack: { + height: '100%', + flexDirection: 'row' + }, + page: { + height: '100%' + }, + // Why: every decision remains reachable in landscape and with accessibility + // text scaling even though Back and swipe-to-skip are intentionally disabled. + pageContent: { + flexGrow: 1, + paddingHorizontal: spacing.xl + }, + content: { + flexGrow: 1, + alignItems: 'center', + justifyContent: 'center', + paddingVertical: spacing.xl + }, + iconSurface: { + width: 64, + height: 64, + borderRadius: radii.card, + alignItems: 'center', + justifyContent: 'center', + backgroundColor: colors.bgRaised, + marginBottom: spacing.xl + }, + title: { + maxWidth: 420, + color: colors.textPrimary, + fontSize: 26, + fontWeight: '700', + letterSpacing: -0.3, + textAlign: 'center' + }, + body: { + maxWidth: 420, + color: colors.textSecondary, + fontSize: typography.bodySize, + lineHeight: 21, + textAlign: 'center', + marginTop: spacing.md + }, + footer: { + width: '100%', + maxWidth: 420, + alignSelf: 'center', + paddingBottom: spacing.lg + }, + primaryButton: { + minHeight: 44, + alignItems: 'center', + justifyContent: 'center', + borderRadius: radii.button, + backgroundColor: colors.surfaceBright, + paddingVertical: spacing.sm + }, + primaryButtonText: { + color: colors.bgBase, + fontSize: typography.bodySize, + fontWeight: '600' + }, + secondaryButton: { + minHeight: 44, + alignItems: 'center', + justifyContent: 'center', + borderRadius: radii.button, + marginTop: spacing.xs, + paddingVertical: spacing.sm + }, + secondaryButtonText: { + color: colors.textSecondary, + fontSize: typography.bodySize, + fontWeight: '500' + }, + buttonPressed: { + opacity: 0.72 + }, + buttonDisabled: { + opacity: 0.58 + }, + error: { + color: colors.statusRed, + fontSize: typography.metaSize, + lineHeight: 18, + textAlign: 'center', + marginBottom: spacing.sm + } +}) diff --git a/mobile/src/session/session-view-opt-in-gate.test.ts b/mobile/src/session/session-view-opt-in-gate.test.ts new file mode 100644 index 00000000000..8a31f466e8a --- /dev/null +++ b/mobile/src/session/session-view-opt-in-gate.test.ts @@ -0,0 +1,49 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { readDefaultSessionViewPreference } from '../storage/session-view-preferences' +import { shouldPresentSessionViewOptIn } from './session-view-opt-in-gate' + +vi.mock('../storage/session-view-preferences', () => ({ + readDefaultSessionViewPreference: vi.fn() +})) + +describe('session view opt-in gate', () => { + beforeEach(() => { + vi.mocked(readDefaultSessionViewPreference).mockReset() + }) + + it('presents when no default has ever been saved', async () => { + vi.mocked(readDefaultSessionViewPreference).mockResolvedValue({ + value: null, + loaded: true, + hasStoredValue: false + }) + await expect(shouldPresentSessionViewOptIn()).resolves.toBe(true) + }) + + it.each(['terminal', 'chat'] as const)('preserves an existing %s choice', async (value) => { + vi.mocked(readDefaultSessionViewPreference).mockResolvedValue({ + value, + loaded: true, + hasStoredValue: true + }) + await expect(shouldPresentSessionViewOptIn()).resolves.toBe(false) + }) + + it('does not overwrite an unknown stored value', async () => { + vi.mocked(readDefaultSessionViewPreference).mockResolvedValue({ + value: null, + loaded: true, + hasStoredValue: true + }) + await expect(shouldPresentSessionViewOptIn()).resolves.toBe(false) + }) + + it('does not block startup when storage is unreadable', async () => { + vi.mocked(readDefaultSessionViewPreference).mockResolvedValue({ + value: null, + loaded: false, + hasStoredValue: false + }) + await expect(shouldPresentSessionViewOptIn()).resolves.toBe(false) + }) +}) diff --git a/mobile/src/session/session-view-opt-in-gate.ts b/mobile/src/session/session-view-opt-in-gate.ts new file mode 100644 index 00000000000..b7e6cb1a036 --- /dev/null +++ b/mobile/src/session/session-view-opt-in-gate.ts @@ -0,0 +1,10 @@ +import { readDefaultSessionViewPreference } from '../storage/session-view-preferences' + +/** Whether to show the one-time screen that lets the user pick terminal vs native + * chat. Present it only when the read succeeded and no default was ever saved, so + * people who already chose (in the opt-in or Settings) are never prompted again, + * and a transient storage failure doesn't trap startup behind the screen. */ +export async function shouldPresentSessionViewOptIn(): Promise { + const preference = await readDefaultSessionViewPreference() + return preference.loaded && !preference.hasStoredValue +} diff --git a/mobile/src/storage/preferences.test.ts b/mobile/src/storage/preferences.test.ts index 13e43d45b43..b636ea12d3e 100644 --- a/mobile/src/storage/preferences.test.ts +++ b/mobile/src/storage/preferences.test.ts @@ -24,6 +24,7 @@ import { import { loadDefaultSessionView, loadSessionViewOverrides, + readDefaultSessionViewPreference, readSessionViewOverridesPreference, saveDefaultSessionView, updateSessionViewOverride @@ -62,6 +63,39 @@ describe('session view preference', () => { await expect(loadDefaultSessionView()).resolves.toBe('terminal') }) + it('reports an absent default as an undecided (null) preference', async () => { + vi.mocked(AsyncStorage.getItem).mockResolvedValue(null) + await expect(readDefaultSessionViewPreference()).resolves.toEqual({ + value: null, + loaded: true, + hasStoredValue: false + }) + + vi.mocked(AsyncStorage.getItem).mockResolvedValue('chat') + await expect(readDefaultSessionViewPreference()).resolves.toEqual({ + value: 'chat', + loaded: true, + hasStoredValue: true + }) + + vi.mocked(AsyncStorage.getItem).mockResolvedValue('bogus') + await expect(readDefaultSessionViewPreference()).resolves.toEqual({ + value: null, + loaded: true, + hasStoredValue: true + }) + }) + + it('marks an unreadable default as not loaded so the opt-in gate can bail', async () => { + vi.mocked(AsyncStorage.getItem).mockRejectedValue(new Error('storage unavailable')) + await expect(readDefaultSessionViewPreference()).resolves.toEqual({ + value: null, + loaded: false, + hasStoredValue: false + }) + await expect(loadDefaultSessionView()).resolves.toBe('terminal') + }) + it('loads and updates per-tab overrides under a host-and-worktree scoped key', async () => { vi.mocked(AsyncStorage.getItem).mockResolvedValue( JSON.stringify({ 'tab-1': 'chat', 'tab-2': 'terminal', 'tab-3': 'bogus' }) diff --git a/mobile/src/storage/session-view-preferences.ts b/mobile/src/storage/session-view-preferences.ts index 750167e3d53..cd9597d6f77 100644 --- a/mobile/src/storage/session-view-preferences.ts +++ b/mobile/src/storage/session-view-preferences.ts @@ -22,17 +22,32 @@ function clearDefaultViewWriteBarrier(barrier: Promise): void { } } -/** Global (per-device) default for how supported agent sessions open. */ -export async function loadDefaultSessionView(): Promise { +export type DefaultSessionViewPreference = { + readonly value: MobileSessionView | null + readonly loaded: boolean + readonly hasStoredValue: boolean +} + +/** Reads the raw per-device default and whether its storage key exists. */ +export async function readDefaultSessionViewPreference(): Promise { await defaultViewWriteBarrier try { const raw = await AsyncStorage.getItem(DEFAULT_SESSION_VIEW_KEY) - return raw === 'chat' || raw === 'terminal' ? raw : DEFAULT_SESSION_VIEW + return { + value: raw === 'chat' || raw === 'terminal' ? raw : null, + loaded: true, + hasStoredValue: raw !== null + } } catch { - return DEFAULT_SESSION_VIEW + return { value: null, loaded: false, hasStoredValue: false } } } +/** Global (per-device) default for how supported agent sessions open. */ +export async function loadDefaultSessionView(): Promise { + return (await readDefaultSessionViewPreference()).value ?? DEFAULT_SESSION_VIEW +} + export function saveDefaultSessionView(view: MobileSessionView): Promise { // Why: callers can outlive their route; a shared barrier keeps remounted // Settings screens from letting an older write land after a newer choice. From 99df4158b950cd2ebd4514e66644e0245de0a03a Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 20 Jul 2026 11:40:17 -0700 Subject: [PATCH 2/5] docs(readme): drop Windows RC download notice Stable v1.4.147 includes the Windows fixes, so point users at latest again. --- README.md | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/README.md b/README.md index 98a1e21b4a1..1a434350bd0 100644 --- a/README.md +++ b/README.md @@ -22,10 +22,6 @@ Download Orca - - Windows? Grab the latest RC — has Windows fixes. - - @@ -216,8 +212,7 @@ Works with **any CLI agent** — if it runs in a terminal, it runs in Orca. ### Desktop — macOS, Windows, Linux - **[Download from onOrca.dev](https://onorca.dev/download)** -- Or grab a build directly: [macOS Apple Silicon](https://github.com/stablyai/orca/releases/latest/download/orca-macos-arm64.dmg) · [macOS Intel](https://github.com/stablyai/orca/releases/latest/download/orca-macos-x64.dmg) · [Windows (.exe)](https://github.com/stablyai/orca/releases/download/v1.4.147-rc.4/orca-windows-setup.exe) · [Linux AppImage](https://github.com/stablyai/orca/releases/latest/download/orca-linux.AppImage) · [All builds](https://github.com/stablyai/orca/releases/latest) -- **Windows users:** use the [latest RC (`v1.4.147-rc.4`)](https://github.com/stablyai/orca/releases#release-v1.4.147-rc.4) — it includes Windows-specific bug fixes not yet in stable. +- Or grab a build directly: [macOS Apple Silicon](https://github.com/stablyai/orca/releases/latest/download/orca-macos-arm64.dmg) · [macOS Intel](https://github.com/stablyai/orca/releases/latest/download/orca-macos-x64.dmg) · [Windows (.exe)](https://github.com/stablyai/orca/releases/latest/download/orca-windows-setup.exe) · [Linux AppImage](https://github.com/stablyai/orca/releases/latest/download/orca-linux.AppImage) · [All builds](https://github.com/stablyai/orca/releases/latest) - Running `orca serve` on a headless Linux server? See the [headless Linux server guide](docs/reference/headless-linux-server.md). _Or via a package manager:_ From aca7d50bab78bddabb44d4971170ac61f4a077ed Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 20 Jul 2026 11:54:30 -0700 Subject: [PATCH 3/5] Stop attaching stale closed PRs/MRs to default-branch checkouts (#9469) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Stop attaching stale closed PRs/MRs to default-branch checkouts On the repo default branch, the implicit head-branch PR lookup (state=all) could attach a historical closed/merged PR whose head ref was the default branch name and show its wrong diffs and checks (#9171). Add a shared default-branch guard: an implicit branch-name match on the repository's default branch never surfaces a non-open review. Applied at the branch-lookup choke point of all five provider clients (GitHub, GitLab, Bitbucket, Azure DevOps, Gitea). Explicitly linked reviews are exempt; open reviews from the trunk stay visible; resolution is lazy (zero git calls unless a non-open candidate appears), TTL-cached, transport-aware (local/WSL/SSH), probe-time-bounded, and fails open. * Treat stuck-locked GitLab MRs as non-open in the default-branch guard Three code-review lanes flagged (one reproduced) that 'locked' — normally a seconds-long merge transition, but a known GitLab wedge state — leaked past the closed/merged-only check and would re-create the #9171 symptom for a stuck-locked MR whose source branch is the trunk. * Bound default-branch lookup to one refresh budget * Coalesce default-branch resolution probes --- src/main/azure-devops/client.test.ts | 143 +++++++ src/main/azure-devops/client.ts | 17 +- src/main/bitbucket/client.test.ts | 71 ++++ src/main/bitbucket/client.ts | 17 +- src/main/gitea/client.test.ts | 81 ++++ src/main/gitea/client.ts | 17 +- src/main/github/client.test.ts | 13 +- src/main/github/client.ts | 18 + .../github/default-branch-stale-pr.test.ts | 388 ++++++++++++++++++ src/main/gitlab/client-mr.test.ts | 155 ++++++- src/main/gitlab/client.ts | 17 +- .../repo-default-branch.test.ts | 272 ++++++++++++ .../source-control/repo-default-branch.ts | 165 ++++++++ 13 files changed, 1367 insertions(+), 7 deletions(-) create mode 100644 src/main/github/default-branch-stale-pr.test.ts create mode 100644 src/main/source-control/repo-default-branch.test.ts create mode 100644 src/main/source-control/repo-default-branch.ts diff --git a/src/main/azure-devops/client.test.ts b/src/main/azure-devops/client.test.ts index 8c993a9db0d..b1f857d1bb1 100644 --- a/src/main/azure-devops/client.test.ts +++ b/src/main/azure-devops/client.test.ts @@ -7,6 +7,7 @@ import { normalizeAzureDevOpsApiBaseUrl } from './client' import { _resetAzureDevOpsRepoRefCache } from './repository-ref' +import { __resetRepoDefaultBranchCacheForTests } from '../source-control/repo-default-branch' const gitExecFileAsyncMock = vi.hoisted(() => vi.fn()) @@ -14,6 +15,22 @@ vi.mock('../git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock })) +/** Serve the remote URL plus the #9171 default-branch resolver probes. */ +function primeGitExecWithDefaultBranch(defaultRef = 'refs/remotes/origin/main'): void { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'https://dev.azure.com/acme/Project/_git/repo\n', stderr: '' } + } + if (args[0] === 'symbolic-ref' && args.includes('refs/remotes/origin/HEAD')) { + return { stdout: `${defaultRef}\n`, stderr: '' } + } + if (args[0] === 'rev-parse' && args[1] === '--verify' && args.includes(defaultRef)) { + return { stdout: 'default-oid\n', stderr: '' } + } + throw new Error(`unexpected git call: ${args.join(' ')}`) + }) +} + const OLD_ENV = process.env const OLD_FETCH = globalThis.fetch @@ -22,6 +39,7 @@ describe('Azure DevOps client', () => { process.env = { ...OLD_ENV, ORCA_AZURE_DEVOPS_TOKEN: 'pat-token' } gitExecFileAsyncMock.mockReset() _resetAzureDevOpsRepoRefCache() + __resetRepoDefaultBranchCacheForTests() }) afterEach(() => { @@ -210,6 +228,131 @@ describe('Azure DevOps client', () => { }) }) + it('hides a stale completed/abandoned PR whose source branch is the repo default branch (#9171)', async () => { + primeGitExecWithDefaultBranch() + globalThis.fetch = vi.fn(async (input: string | URL | Request) => { + const url = new URL(String(input)) + const response = (body: unknown): Response => + new Response(JSON.stringify(body), { + status: 200, + headers: { 'Content-Type': 'application/json' } + }) + + if (url.pathname === '/acme/Project/_apis/git/repositories/repo') { + return response({ id: 'repo-guid' }) + } + if (url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests') { + return response({ + value: [ + { + pullRequestId: 30, + title: 'Accidental PR from main', + status: 'abandoned', + creationDate: '2026-05-10T00:00:00Z', + closedDate: '2026-05-11T00:00:00Z', + lastMergeSourceCommit: { commitId: 'stale-main-oid' } + } + ] + }) + } + return new Response(JSON.stringify({ message: 'not found' }), { status: 404 }) + }) as never + + await expect(getAzureDevOpsPullRequestForBranch('/repo', 'refs/heads/main')).resolves.toBeNull() + }) + + it('discards a completed default-branch shadow and refetches the linked PR (#9171)', async () => { + primeGitExecWithDefaultBranch() + globalThis.fetch = vi.fn(async (input: string | URL | Request) => { + const url = new URL(String(input)) + const response = (body: unknown): Response => + new Response(JSON.stringify(body), { + status: 200, + headers: { 'Content-Type': 'application/json' } + }) + + if (url.pathname === '/acme/Project/_apis/git/repositories/repo') { + return response({ id: 'repo-guid' }) + } + if (url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests') { + return response({ + value: [ + { + pullRequestId: 32, + title: 'Completed PR from main', + status: 'completed', + creationDate: '2026-05-10T00:00:00Z', + closedDate: '2026-05-12T00:00:00Z', + lastMergeSourceCommit: { commitId: 'shadow-oid' } + } + ] + }) + } + if (url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests/40') { + return response({ + pullRequestId: 40, + title: 'Linked PR', + status: 'active', + creationDate: '2026-05-13T00:00:00Z', + mergeStatus: 'succeeded', + lastMergeSourceCommit: { commitId: 'linked-oid' } + }) + } + if ( + url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests/40/statuses' + ) { + return response({ value: [{ state: 'succeeded' }] }) + } + return new Response(JSON.stringify({ message: 'not found' }), { status: 404 }) + }) as never + + await expect( + getAzureDevOpsPullRequestForBranch('/repo', 'refs/heads/main', 40) + ).resolves.toMatchObject({ number: 40, state: 'open' }) + }) + + it('keeps an active PR whose source branch is the repo default branch', async () => { + gitExecFileAsyncMock.mockResolvedValue({ + stdout: 'https://dev.azure.com/acme/Project/_git/repo\n' + }) + globalThis.fetch = vi.fn(async (input: string | URL | Request) => { + const url = new URL(String(input)) + const response = (body: unknown): Response => + new Response(JSON.stringify(body), { + status: 200, + headers: { 'Content-Type': 'application/json' } + }) + + if (url.pathname === '/acme/Project/_apis/git/repositories/repo') { + return response({ id: 'repo-guid' }) + } + if (url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests') { + return response({ + value: [ + { + pullRequestId: 31, + title: 'main → release', + status: 'active', + creationDate: '2026-05-10T00:00:00Z', + mergeStatus: 'succeeded', + lastMergeSourceCommit: { commitId: 'main-head-oid' } + } + ] + }) + } + if ( + url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests/31/statuses' + ) { + return response({ value: [{ state: 'succeeded' }] }) + } + return new Response(JSON.stringify({ message: 'not found' }), { status: 404 }) + }) as never + + await expect( + getAzureDevOpsPullRequestForBranch('/repo', 'refs/heads/main') + ).resolves.toMatchObject({ number: 31, state: 'open' }) + }) + it('cancels unread error-response bodies so bundled undici cannot crash on socket close', async () => { gitExecFileAsyncMock.mockResolvedValue({ stdout: 'https://dev.azure.com/acme/Project/_git/repo\n' diff --git a/src/main/azure-devops/client.ts b/src/main/azure-devops/client.ts index 25390ba4bfa..9ddae9ffe45 100644 --- a/src/main/azure-devops/client.ts +++ b/src/main/azure-devops/client.ts @@ -1,10 +1,12 @@ import { deriveAzureDevOpsStatus, mapAzureDevOpsPullRequest, + mapAzureDevOpsPullRequestState, type AzureDevOpsPullRequestInfo, type RawAzureDevOpsPullRequest, type RawAzureDevOpsStatus } from './pull-request-mappers' +import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-default-branch' import { getAzureDevOpsRepoRef, type AzureDevOpsRepoRef } from './repository-ref' import { getHostedReviewLocalGitOptions, @@ -230,7 +232,20 @@ export async function getAzureDevOpsPullRequestForBranch( ) const raw = (list?.value ?? []).sort(sortPullRequestsForBranch)[0] if (raw) { - return normalizePullRequest(repo, repository.idOrName, repository.webBaseUrl, raw) + // Why (#9171): discard a non-open implicit branch match on the repo + // default branch and fall through to the linked-number fallback below. + const hideOnDefaultBranch = await shouldHideNonOpenReviewOnDefaultBranch({ + state: mapAzureDevOpsPullRequestState(raw), + reviewNumber: raw.pullRequestId ?? null, + linkedReviewNumber: linkedPRNumber, + branchName, + repoPath, + connectionId, + localGitOptions: getHostedReviewLocalGitOptions(options) + }) + if (!hideOnDefaultBranch) { + return normalizePullRequest(repo, repository.idOrName, repository.webBaseUrl, raw) + } } } diff --git a/src/main/bitbucket/client.test.ts b/src/main/bitbucket/client.test.ts index 7ecfeb64151..dd55bfad6d2 100644 --- a/src/main/bitbucket/client.test.ts +++ b/src/main/bitbucket/client.test.ts @@ -15,6 +15,23 @@ import { getBitbucketPullRequestForBranchOrThrow } from './client' import { _resetBitbucketRepoRefCache } from './repository-ref' +import { __resetRepoDefaultBranchCacheForTests } from '../source-control/repo-default-branch' + +/** Serve the remote URL plus the #9171 default-branch resolver probes. */ +function primeGitExecWithDefaultBranch(defaultRef = 'refs/remotes/origin/main'): void { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'git@bitbucket.org:team/repo.git\n', stderr: '' } + } + if (args[0] === 'symbolic-ref' && args.includes('refs/remotes/origin/HEAD')) { + return { stdout: `${defaultRef}\n`, stderr: '' } + } + if (args[0] === 'rev-parse' && args[1] === '--verify' && args.includes(defaultRef)) { + return { stdout: 'default-oid\n', stderr: '' } + } + throw new Error(`unexpected git call: ${args.join(' ')}`) + }) +} const OLD_ENV = process.env @@ -50,9 +67,63 @@ describe('Bitbucket client', () => { stderr: '' }) _resetBitbucketRepoRefCache() + __resetRepoDefaultBranchCacheForTests() vi.unstubAllGlobals() }) + it('hides a stale DECLINED PR whose source branch is the repo default branch (#9171)', async () => { + primeGitExecWithDefaultBranch() + const fetchMock = vi.fn(async (url: string) => { + if (url.includes('/statuses/build')) { + return Response.json({ values: [] }) + } + return Response.json({ + values: [{ ...bitbucketPr(7), state: 'DECLINED', source: { branch: { name: 'main' } } }] + }) + }) + vi.stubGlobal('fetch', fetchMock) + + await expect(getBitbucketPullRequestForBranch('/repo', 'refs/heads/main')).resolves.toBeNull() + }) + + it('keeps an OPEN PR whose source branch is the repo default branch', async () => { + const fetchMock = vi.fn(async (url: string) => { + if (url.includes('/statuses/build')) { + return Response.json({ values: [{ state: 'SUCCESSFUL' }] }) + } + return Response.json({ + values: [ + { ...bitbucketPr(8), source: { branch: { name: 'main' }, commit: { hash: 'abc123' } } } + ] + }) + }) + vi.stubGlobal('fetch', fetchMock) + + await expect( + getBitbucketPullRequestForBranch('/repo', 'refs/heads/main') + ).resolves.toMatchObject({ number: 8, state: 'open' }) + }) + + it('discards a MERGED default-branch shadow and refetches the linked PR via the fallback (#9171)', async () => { + primeGitExecWithDefaultBranch() + const fetchMock = vi.fn(async (url: string) => { + if (url.includes('/statuses/build')) { + return Response.json({ values: [] }) + } + if (url.endsWith('/pullrequests/42')) { + return Response.json(bitbucketPr(42)) + } + return Response.json({ + values: [{ ...bitbucketPr(7), state: 'MERGED', source: { branch: { name: 'main' } } }] + }) + }) + vi.stubGlobal('fetch', fetchMock) + + await expect( + getBitbucketPullRequestForBranch('/repo', 'refs/heads/main', 42) + ).resolves.toMatchObject({ number: 42 }) + }) + it('fetches a branch pull request and commit build status', async () => { const fetchMock = vi.fn(async (url: string, _init?: RequestInit) => { if (url.includes('/statuses/build')) { diff --git a/src/main/bitbucket/client.ts b/src/main/bitbucket/client.ts index ae4158fa180..e5163bd525a 100644 --- a/src/main/bitbucket/client.ts +++ b/src/main/bitbucket/client.ts @@ -3,10 +3,12 @@ import type { CheckStatus } from '../../shared/types' import { deriveBitbucketBuildStatus, mapBitbucketPullRequest, + mapBitbucketPullRequestState, type BitbucketPullRequestInfo, type RawBitbucketBuildStatus, type RawBitbucketPullRequest } from './pull-request-mappers' +import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-default-branch' import { getBitbucketRepoRef, type BitbucketRepoRef } from './repository-ref' import { getHostedReviewLocalGitOptions, @@ -233,7 +235,20 @@ export async function getBitbucketPullRequestForBranch( ) const raw = list?.values?.[0] if (raw) { - return normalizePullRequest(repo, raw) + // Why (#9171): discard a non-open implicit branch match on the repo + // default branch and fall through to the linked-number fallback below. + const hideOnDefaultBranch = await shouldHideNonOpenReviewOnDefaultBranch({ + state: mapBitbucketPullRequestState(raw.state), + reviewNumber: raw.id ?? null, + linkedReviewNumber: linkedPRNumber, + branchName, + repoPath, + connectionId, + localGitOptions: getHostedReviewLocalGitOptions(options) + }) + if (!hideOnDefaultBranch) { + return normalizePullRequest(repo, raw) + } } } diff --git a/src/main/gitea/client.test.ts b/src/main/gitea/client.test.ts index 60f3441263c..eada3d6cd95 100644 --- a/src/main/gitea/client.test.ts +++ b/src/main/gitea/client.test.ts @@ -21,9 +21,26 @@ import { _resetGiteaPullRequestScanCache, scanGiteaPullRequests } from './pull-request-scan-cache' +import { __resetRepoDefaultBranchCacheForTests } from '../source-control/repo-default-branch' const OLD_ENV = process.env +/** Serve the remote URL plus the #9171 default-branch resolver probes. */ +function primeGitExecWithDefaultBranch(defaultRef = 'refs/remotes/origin/main'): void { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'https://git.example.com/team/repo.git\n', stderr: '' } + } + if (args[0] === 'symbolic-ref' && args.includes('refs/remotes/origin/HEAD')) { + return { stdout: `${defaultRef}\n`, stderr: '' } + } + if (args[0] === 'rev-parse' && args[1] === '--verify' && args.includes(defaultRef)) { + return { stdout: 'default-oid\n', stderr: '' } + } + throw new Error(`unexpected git call: ${args.join(' ')}`) + }) +} + function giteaPr(index = 7, branch = 'feature/gitea') { return { number: index, @@ -52,9 +69,73 @@ describe('Gitea client', () => { }) _resetGiteaRepoRefCache() _resetGiteaPullRequestScanCache() + __resetRepoDefaultBranchCacheForTests() vi.unstubAllGlobals() }) + it('hides a stale closed PR whose source branch is the repo default branch (#9171)', async () => { + primeGitExecWithDefaultBranch() + const fetchMock = vi.fn(async (url: string) => { + const parsed = new URL(url) + if (parsed.pathname.endsWith('/status')) { + return Response.json({ state: 'success' }) + } + return Response.json([{ ...giteaPr(7, 'main'), state: 'closed' }]) + }) + vi.stubGlobal('fetch', fetchMock) + + await expect(getGiteaPullRequestForBranch('/repo', 'refs/heads/main')).resolves.toBeNull() + }) + + it('hides a stale merged PR on the default branch but keeps an open one', async () => { + primeGitExecWithDefaultBranch() + const fetchMock = vi.fn(async (url: string) => { + const parsed = new URL(url) + if (parsed.pathname.endsWith('/status')) { + return Response.json({ state: 'success' }) + } + return Response.json([{ ...giteaPr(9, 'main'), merged: true }]) + }) + vi.stubGlobal('fetch', fetchMock) + + await expect(getGiteaPullRequestForBranch('/repo', 'refs/heads/main')).resolves.toBeNull() + + _resetGiteaPullRequestScanCache() + __resetRepoDefaultBranchCacheForTests() + const openFetchMock = vi.fn(async (url: string) => { + const parsed = new URL(url) + if (parsed.pathname.endsWith('/status')) { + return Response.json({ state: 'success' }) + } + return Response.json([giteaPr(10, 'main')]) + }) + vi.stubGlobal('fetch', openFetchMock) + + await expect(getGiteaPullRequestForBranch('/repo', 'refs/heads/main')).resolves.toMatchObject({ + number: 10, + state: 'open' + }) + }) + + it('discards a closed default-branch shadow and refetches the linked PR via the fallback (#9171)', async () => { + primeGitExecWithDefaultBranch() + const fetchMock = vi.fn(async (url: string) => { + const parsed = new URL(url) + if (parsed.pathname.endsWith('/status')) { + return Response.json({ state: 'success' }) + } + if (parsed.pathname.endsWith('/pulls/42')) { + return Response.json(giteaPr(42, 'main')) + } + return Response.json([{ ...giteaPr(7, 'main'), state: 'closed' }]) + }) + vi.stubGlobal('fetch', fetchMock) + + await expect( + getGiteaPullRequestForBranch('/repo', 'refs/heads/main', 42) + ).resolves.toMatchObject({ number: 42 }) + }) + it('normalizes Gitea API base URLs', () => { expect(normalizeGiteaApiBaseUrl('https://git.example.com')).toBe( 'https://git.example.com/api/v1' diff --git a/src/main/gitea/client.ts b/src/main/gitea/client.ts index da6c498d8b7..ba9e7c3ed8f 100644 --- a/src/main/gitea/client.ts +++ b/src/main/gitea/client.ts @@ -1,10 +1,12 @@ import { deriveGiteaCommitStatus, mapGiteaPullRequest, + mapGiteaPullRequestState, type GiteaPullRequestInfo, type RawGiteaCombinedStatus, type RawGiteaPullRequest } from './pull-request-mappers' +import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-default-branch' import { getGiteaRepoRef, type GiteaRepoRef } from './repository-ref' import { invalidateGiteaPullRequestScan, scanGiteaPullRequests } from './pull-request-scan-cache' import { @@ -281,7 +283,20 @@ export async function getGiteaPullRequestForBranch( ) const raw = pullRequests.find((item) => matchesBranch(item, branchName)) if (raw) { - return normalizePullRequest(repo, raw) + // Why (#9171): discard a non-open implicit branch match on the repo + // default branch and fall through to the linked-number fallback below. + const hideOnDefaultBranch = await shouldHideNonOpenReviewOnDefaultBranch({ + state: mapGiteaPullRequestState(raw), + reviewNumber: raw.number ?? null, + linkedReviewNumber: linkedPRNumber, + branchName, + repoPath, + connectionId, + localGitOptions: getHostedReviewLocalGitOptions(options) + }) + if (!hideOnDefaultBranch) { + return normalizePullRequest(repo, raw) + } } } diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 07a007f7f1e..21fc7cc60b8 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -126,6 +126,7 @@ import { } from './client' import { __resetPRConflictSummaryCachesForTests } from './conflict-summary' import { resetMergedPRCommitMembershipCacheForTest } from './merged-pr-commit-membership' +import { __resetRepoDefaultBranchCacheForTests } from '../source-control/repo-default-branch' describe('checkOrcaStarred', () => { beforeEach(() => { @@ -198,6 +199,9 @@ describe('getPRForBranch', () => { __resetTrackedUpstreamBranchCacheForTests() __resetPRConflictSummaryCachesForTests() resetMergedPRCommitMembershipCacheForTest() + // Why: the #9171 guard caches default-branch resolutions per repoPath; + // reset so non-open implicit lookups stay order-independent across tests. + __resetRepoDefaultBranchCacheForTests() }) it('queries GitHub by head branch when the remote is on github.com', async () => { @@ -924,7 +928,14 @@ describe('getPRForBranch', () => { } ) - expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + // Why: the merged-implicit head probe must use the supplied oid — no + // `rev-parse HEAD` shell-out. (The #9171 guard may still probe the repo + // default branch for this non-open implicit result; that is unrelated.) + expect( + gitExecFileAsyncMock.mock.calls.some( + (call) => call[0][0] === 'rev-parse' && call[0][1] === 'HEAD' + ) + ).toBe(false) expect(outcome).toMatchObject({ kind: 'found', pr: { diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 53bf26614ff..d67b3e4405b 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -77,6 +77,7 @@ import { getHostedReviewLocalGitOptions, type HostedReviewExecutionOptions } from '../source-control/hosted-review-git-options' +import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-default-branch' import { readLocalGitConfigSignature } from './local-git-config-signature' import { getRememberedGhCwdResolutionFailure, @@ -3039,6 +3040,23 @@ export async function getPRForBranchOutcome( if ((await hideMergedImplicitPR(data, dataRepo)) && !shouldPreserveMergedFallback) { return { kind: 'no-pr', fetchedAt: Date.now() } } + // Why (#9171): on the default branch an implicit branch/fallback match must + // never surface a non-open PR — it overrides the merged-fallback + // preservation and merged-at-head carve-out on the trunk only. An exact + // linked lookup returns the linked number, so linked PRs are exempt. + if ( + await shouldHideNonOpenReviewOnDefaultBranch({ + state: mapPRState(data.state, data.isDraft), + reviewNumber: data.number, + linkedReviewNumber: linkedPRNumber, + branchName, + repoPath, + connectionId, + localGitOptions + }) + ) { + return { kind: 'no-pr', fetchedAt: Date.now() } + } const mergeable = derivePullRequestMergeable(data) const conflictSummary = diff --git a/src/main/github/default-branch-stale-pr.test.ts b/src/main/github/default-branch-stale-pr.test.ts new file mode 100644 index 00000000000..e9a783b75ad --- /dev/null +++ b/src/main/github/default-branch-stale-pr.test.ts @@ -0,0 +1,388 @@ +/* + * Issue #9171 — "Wrong PR diffs displayed when checking out the default branch". + * + * A repo's default branch is `master`. In the past someone opened a PR whose + * HEAD was `master` (accidentally opening a PR *from* the default branch); it + * was closed long ago. getPRForBranch looks a PR up purely by head branch name + * with `state=all`, so without a guard that stale closed PR gets attached to + * the default-branch checkout, surfacing its wrong diffs and checks. + * + * These tests assert the CORRECT behavior: an implicit (non-linked) branch or + * fallback-number match on the repo default branch never surfaces a non-open + * PR, while open PRs on the trunk and closed/merged PRs on feature branches + * keep today's behavior. The default branch is resolved by the REAL + * resolveDefaultBaseRefViaExec over the mocked git exec layer, so the + * symbolic-ref / rev-parse plumbing and the `origin/` strip are exercised. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +type RateLimitGuardResult = + | { blocked: false } + | { blocked: true; remaining: number; limit: number; resetAt: number } + +const { + execFileAsyncMock, + ghExecFileAsyncMock, + getOwnerRepoMock, + getIssueOwnerRepoMock, + getOwnerRepoForRemoteMock, + resolvePRRepositoryCandidatesMock, + getRemoteUrlForRepoMock, + gitExecFileAsyncMock, + getRateLimitMock, + rateLimitGuardMock, + noteRateLimitSpendMock, + ghRepoExecOptionsMock, + githubRepoContextMock, + getSshGitProviderMock, + readLocalGitConfigSignatureMock, + acquireMock, + releaseMock +} = vi.hoisted(() => ({ + execFileAsyncMock: vi.fn(), + ghExecFileAsyncMock: vi.fn(), + getOwnerRepoMock: vi.fn(), + getIssueOwnerRepoMock: vi.fn(), + getOwnerRepoForRemoteMock: vi.fn(), + resolvePRRepositoryCandidatesMock: vi.fn(), + getRemoteUrlForRepoMock: vi.fn(), + gitExecFileAsyncMock: vi.fn(), + getRateLimitMock: vi.fn(), + rateLimitGuardMock: vi.fn<(bucket?: string) => RateLimitGuardResult>(() => ({ + blocked: false + })), + noteRateLimitSpendMock: vi.fn(), + ghRepoExecOptionsMock: vi.fn((context) => + context.connectionId + ? {} + : { + cwd: context.repoPath, + ...(context.wslDistro ? { wslDistro: context.wslDistro } : {}) + } + ), + githubRepoContextMock: vi.fn((repoPath, connectionId, localGitOptions) => ({ + repoPath, + connectionId: connectionId ?? null, + ...localGitOptions + })), + getSshGitProviderMock: vi.fn(), + readLocalGitConfigSignatureMock: vi.fn(), + acquireMock: vi.fn(), + releaseMock: vi.fn() +})) + +vi.mock('./gh-utils', () => ({ + execFileAsync: execFileAsyncMock, + ghExecFileAsync: ghExecFileAsyncMock, + getOwnerRepo: getOwnerRepoMock, + getIssueOwnerRepo: getIssueOwnerRepoMock, + getOwnerRepoForRemote: getOwnerRepoForRemoteMock, + resolvePRRepositoryCandidates: resolvePRRepositoryCandidatesMock, + getRemoteUrlForRepo: getRemoteUrlForRepoMock, + gitExecFileAsync: gitExecFileAsyncMock, + ghRepoExecOptions: ghRepoExecOptionsMock, + githubRepoContext: githubRepoContextMock, + classifyGhError: (stderr: string) => { + const lower = stderr.toLowerCase() + if (lower.includes('not found') || stderr.includes('HTTP 404')) { + return { type: 'not_found', message: stderr } + } + if (lower.includes('rate limit')) { + return { type: 'rate_limited', message: stderr } + } + return { type: 'unknown', message: stderr } + }, + parseGitHubOwnerRepo: (remoteUrl: string) => { + const match = remoteUrl.trim().match(/github\.com[:/]([^/]+)\/([^/]+?)(?:\.git)?$/) + return match ? { owner: match[1], repo: match[2] } : null + }, + acquire: acquireMock, + release: releaseMock, + _resetOwnerRepoCache: vi.fn() +})) + +vi.mock('../git/runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock +})) + +vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock +})) + +vi.mock('./local-git-config-signature', () => ({ + readLocalGitConfigSignature: readLocalGitConfigSignatureMock +})) + +vi.mock('./rate-limit', () => ({ + getRateLimit: getRateLimitMock, + rateLimitGuard: rateLimitGuardMock, + noteRateLimitSpend: noteRateLimitSpendMock +})) + +import { + getPRForBranch, + getPRForBranchOutcome, + _resetOwnerRepoCache, + _resetMergeQueueCacheForTests, + __resetTrackedUpstreamBranchCacheForTests +} from './client' +import { __resetPRConflictSummaryCachesForTests } from './conflict-summary' +import { resetMergedPRCommitMembershipCacheForTest } from './merged-pr-commit-membership' +import { __resetRepoDefaultBranchCacheForTests } from '../source-control/repo-default-branch' + +const DEFAULT_BRANCH_REF = 'refs/remotes/origin/master' + +/** + * Answer the real resolveDefaultBaseRefViaExec probes (origin/HEAD + * symbolic-ref + rev-parse verification), the merged-at-head `rev-parse HEAD` + * probe, and the tracked-upstream `for-each-ref` snapshot. + */ +function primeGitExecForDefaultBranch({ + defaultRef = DEFAULT_BRANCH_REF, + headOid = 'checkout-head-oid' +}: { defaultRef?: string; headOid?: string } = {}): void { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'symbolic-ref' && args.includes('refs/remotes/origin/HEAD')) { + return { stdout: `${defaultRef}\n`, stderr: '' } + } + if (args[0] === 'rev-parse' && args[1] === '--verify') { + if (args.includes(defaultRef)) { + return { stdout: 'default-branch-oid\n', stderr: '' } + } + throw new Error(`fatal: Needed a single revision: ${args.join(' ')}`) + } + if (args[0] === 'rev-parse' && args[1] === 'HEAD') { + return { stdout: `${headOid}\n`, stderr: '' } + } + if (args[0] === 'for-each-ref') { + return { stdout: '', stderr: '' } + } + throw new Error(`unexpected git call: ${args.join(' ')}`) + }) +} + +type RestPRShape = { + number?: number + state?: string + merged_at?: string | null + head_ref?: string + head_sha?: string +} + +function restPR({ + number = 7, + state = 'closed', + merged_at = null, + head_ref = 'master', + head_sha = 'stale-master-oid' +}: RestPRShape = {}): Record { + return { + number, + title: 'Historical PR', + state, + merged_at, + html_url: `https://github.com/acme/widgets/pull/${number}`, + updated_at: '2024-01-01T00:00:00Z', + draft: false, + mergeable: null, + base: { ref: 'old-release', sha: 'base-oid' }, + head: { ref: head_ref, sha: head_sha } + } +} + +/** Route the REST head-branch lookup; every other gh call fails so branch data + * is kept as-is (hydration falls back to the branch payload). */ +function primeGhExecWithBranchList(list: Record[]): void { + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'api' && args[1]?.includes('pulls?head=')) { + return { stdout: JSON.stringify(list) } + } + throw new Error(`gh unavailable: ${args.join(' ')}`) + }) +} + +describe('issue #9171: default-branch checkout must not attach a stale non-open PR', () => { + beforeEach(() => { + execFileAsyncMock.mockReset() + ghExecFileAsyncMock.mockReset() + getOwnerRepoMock.mockReset() + getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' }) + getIssueOwnerRepoMock.mockReset() + getOwnerRepoForRemoteMock.mockReset() + resolvePRRepositoryCandidatesMock.mockReset() + resolvePRRepositoryCandidatesMock.mockImplementation(async (repoPath, connectionId) => { + const origin = await getOwnerRepoMock(repoPath, connectionId) + return { candidates: origin ? [origin] : [], headRepo: origin } + }) + getRemoteUrlForRepoMock.mockReset() + gitExecFileAsyncMock.mockReset() + getRateLimitMock.mockReset() + getRateLimitMock.mockResolvedValue({ resources: {} }) + rateLimitGuardMock.mockReset() + rateLimitGuardMock.mockReturnValue({ blocked: false }) + noteRateLimitSpendMock.mockReset() + ghRepoExecOptionsMock.mockClear() + githubRepoContextMock.mockClear() + getSshGitProviderMock.mockReset() + readLocalGitConfigSignatureMock.mockReset() + readLocalGitConfigSignatureMock.mockResolvedValue(undefined) + acquireMock.mockReset() + releaseMock.mockReset() + acquireMock.mockResolvedValue(undefined) + _resetOwnerRepoCache() + _resetMergeQueueCacheForTests() + __resetTrackedUpstreamBranchCacheForTests() + __resetPRConflictSummaryCachesForTests() + resetMergedPRCommitMembershipCacheForTest() + __resetRepoDefaultBranchCacheForTests() + }) + + it('hides a stale CLOSED PR when checked out on the default branch (master)', async () => { + primeGitExecForDefaultBranch() + primeGhExecWithBranchList([restPR({ number: 7, state: 'closed' })]) + + const pr = await getPRForBranch('/repo-root', 'master') + + // The head-branch lookup still runs (state=all) and sees the closed PR… + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + ['api', 'repos/acme/widgets/pulls?head=acme%3Amaster&state=all&per_page=1'], + { cwd: '/repo-root' } + ) + // …but the default-branch guard discards it: no PR on the trunk (#9171). + expect(pr).toBeNull() + }) + + it('reports kind="no-pr" from getPRForBranchOutcome on the default branch', async () => { + primeGitExecForDefaultBranch() + primeGhExecWithBranchList([restPR({ number: 7, state: 'closed' })]) + + const outcome = await getPRForBranchOutcome('/repo-root', 'master') + + expect(outcome.kind).toBe('no-pr') + }) + + it('keeps a genuinely OPEN PR on the default branch visible', async () => { + primeGitExecForDefaultBranch() + primeGhExecWithBranchList([restPR({ number: 8, state: 'open' })]) + + const pr = await getPRForBranch('/repo-root', 'master') + + expect(pr?.number).toBe(8) + expect(pr?.state).toBe('open') + // Open results never consult git for the default branch (lazy resolution). + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('keeps a CLOSED PR on a feature branch visible (behavior preserved)', async () => { + primeGitExecForDefaultBranch() + primeGhExecWithBranchList([ + restPR({ number: 9, state: 'closed', head_ref: 'feature-x', head_sha: 'feature-oid' }) + ]) + + const pr = await getPRForBranch('/repo-root', 'feature-x') + + expect(pr?.number).toBe(9) + expect(pr?.state).toBe('closed') + }) + + it('preserves the merged-at-head carve-out on a feature branch', async () => { + // Worktree HEAD is exactly the merged PR's final head commit. + primeGitExecForDefaultBranch({ headOid: 'merged-head-oid' }) + primeGhExecWithBranchList([ + restPR({ + number: 10, + state: 'closed', + merged_at: '2024-02-02T00:00:00Z', + head_ref: 'feature-x', + head_sha: 'merged-head-oid' + }) + ]) + + const pr = await getPRForBranch('/repo-root', 'feature-x') + + expect(pr?.number).toBe(10) + expect(pr?.state).toBe('merged') + }) + + it('hides a CLOSED fallback-number PR on the default branch (self-heal for persisted stale PRs)', async () => { + primeGitExecForDefaultBranch() + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'api' && args[1]?.includes('pulls?head=')) { + return { stdout: '[]' } + } + if (args[0] === 'pr' && args[1] === 'view' && args[2] === '7') { + return { + stdout: JSON.stringify({ + number: 7, + title: 'Historical PR', + state: 'CLOSED', + url: 'https://github.com/acme/widgets/pull/7', + statusCheckRollup: [], + updatedAt: '2024-01-01T00:00:00Z', + isDraft: false, + mergeable: 'UNKNOWN', + baseRefName: 'old-release', + headRefName: 'master', + baseRefOid: 'base-oid', + headRefOid: 'stale-master-oid' + }) + } + } + throw new Error(`gh unavailable: ${args.join(' ')}`) + }) + + const outcome = await getPRForBranchOutcome('/repo-root', 'master', null, null, 7) + + expect(outcome.kind).toBe('no-pr') + }) + + it('overrides acceptMergedFallbackPR preservation for a merged fallback PR on the default branch', async () => { + primeGitExecForDefaultBranch() + ghExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'api' && args[1]?.includes('pulls?head=')) { + return { stdout: '[]' } + } + if (args[0] === 'pr' && args[1] === 'view' && args[2] === '7') { + return { + stdout: JSON.stringify({ + number: 7, + title: 'Historical PR', + state: 'MERGED', + url: 'https://github.com/acme/widgets/pull/7', + statusCheckRollup: [], + updatedAt: '2024-01-01T00:00:00Z', + isDraft: false, + mergeable: 'UNKNOWN', + baseRefName: 'old-release', + headRefName: 'master', + baseRefOid: 'base-oid', + headRefOid: 'stale-master-oid' + }) + } + } + throw new Error(`gh unavailable: ${args.join(' ')}`) + }) + + const outcome = await getPRForBranchOutcome('/repo-root', 'master', null, null, 7, { + acceptMergedFallbackPR: true + }) + + expect(outcome.kind).toBe('no-pr') + }) + + it('fails open when the default branch cannot be resolved (stale PR still returned)', async () => { + // Every git probe fails: no origin/HEAD, no main/master refs. + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'for-each-ref') { + return { stdout: '', stderr: '' } + } + throw new Error(`fatal: git unavailable: ${args.join(' ')}`) + }) + primeGhExecWithBranchList([restPR({ number: 7, state: 'closed' })]) + + const pr = await getPRForBranch('/repo-root', 'master') + + expect(pr?.number).toBe(7) + expect(pr?.state).toBe('closed') + }) +}) diff --git a/src/main/gitlab/client-mr.test.ts b/src/main/gitlab/client-mr.test.ts index aac5193c1e3..91273d7a83f 100644 --- a/src/main/gitlab/client-mr.test.ts +++ b/src/main/gitlab/client-mr.test.ts @@ -10,7 +10,8 @@ const { getProjectRefMock, resolveIssueSourceMock, acquireMock, - releaseMock + releaseMock, + gitExecFileAsyncMock } = vi.hoisted(() => ({ glabExecFileAsyncMock: vi.fn(), glabApiWithHeadersMock: vi.fn(), @@ -18,7 +19,14 @@ const { getProjectRefMock: vi.fn(), resolveIssueSourceMock: vi.fn(), acquireMock: vi.fn(), - releaseMock: vi.fn() + releaseMock: vi.fn(), + gitExecFileAsyncMock: vi.fn() +})) + +// Why: the #9171 default-branch guard resolves the repo default branch via +// git; keep those probes hermetic instead of spawning real git processes. +vi.mock('../git/runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock })) vi.mock('./gl-utils', async () => { @@ -56,6 +64,20 @@ import { updateMR, updateMRReviewers } from './client' +import { __resetRepoDefaultBranchCacheForTests } from '../source-control/repo-default-branch' + +/** Answer the real default-branch resolver probes (#9171 guard). */ +function primeGitDefaultBranch(defaultRef = 'refs/remotes/origin/main'): void { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'symbolic-ref' && args.includes('refs/remotes/origin/HEAD')) { + return { stdout: `${defaultRef}\n`, stderr: '' } + } + if (args[0] === 'rev-parse' && args[1] === '--verify' && args.includes(defaultRef)) { + return { stdout: 'default-oid\n', stderr: '' } + } + throw new Error(`unexpected git call: ${args.join(' ')}`) + }) +} describe('gitlab client — MR operations', () => { beforeEach(() => { @@ -67,6 +89,9 @@ describe('gitlab client — MR operations', () => { acquireMock.mockReset() releaseMock.mockReset() acquireMock.mockResolvedValue(undefined) + gitExecFileAsyncMock.mockReset() + primeGitDefaultBranch() + __resetRepoDefaultBranchCacheForTests() _resetGitLabRateLimitCache() getGlabKnownHostsMock.mockResolvedValue(['gitlab.com']) resolveIssueSourceMock.mockResolvedValue({ @@ -446,6 +471,132 @@ describe('gitlab client — MR operations', () => { }) }) + it('hides a stale closed MR whose source branch is the repo default branch (#9171)', async () => { + getProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'g/p' }) + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + iid: 7, + title: 'Accidental MR from main', + state: 'closed', + sha: 'stale-main-oid', + head_pipeline: { status: 'success' } + } + ]) + }) + + await expect(getMergeRequestForBranch('/repo', 'main')).resolves.toBeNull() + }) + + it('hides a stuck-locked MR whose source branch is the repo default branch (#9171)', async () => { + getProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'g/p' }) + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + iid: 9, + title: 'Wedged mid-merge MR from main', + state: 'locked', + sha: 'locked-main-oid', + head_pipeline: { status: 'success' } + } + ]) + }) + + await expect(getMergeRequestForBranch('/repo', 'main')).resolves.toBeNull() + }) + + it('keeps an open MR whose source branch is the repo default branch', async () => { + getProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'g/p' }) + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + iid: 8, + title: 'main → release', + state: 'opened', + sha: 'abc', + head_pipeline: { status: 'success' } + } + ]) + }) + + const mr = await getMergeRequestForBranch('/repo', 'main') + expect(mr?.number).toBe(8) + // Open results never consult git for the default branch (lazy resolution). + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('keeps a closed MR on a feature branch (behavior preserved)', async () => { + getProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'g/p' }) + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + iid: 9, + title: 'Rejected work', + state: 'closed', + sha: 'def', + head_pipeline: { status: 'failed' } + } + ]) + }) + + const mr = await getMergeRequestForBranch('/repo', 'feature/rejected') + expect(mr?.number).toBe(9) + expect(mr?.state).toBe('closed') + }) + + it('discards a closed default-branch shadow and refetches the linked MR via the fallback (#9171)', async () => { + getProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'g/p' }) + glabExecFileAsyncMock + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + iid: 7, + title: 'Accidental MR from main', + state: 'closed', + sha: 'stale-main-oid', + head_pipeline: { status: 'success' } + } + ]) + }) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + iid: 42, + title: 'Linked MR', + state: 'merged', + pipeline: { status: 'success' } + }) + }) + + const mr = await getMergeRequestForBranch('/repo', 'main', 42) + + expect(mr).toMatchObject({ number: 42, state: 'merged' }) + expect(glabExecFileAsyncMock).toHaveBeenLastCalledWith( + ['api', 'projects/g%2Fp/merge_requests/42'], + { cwd: '/repo' } + ) + }) + + it('keeps a non-open branch match on the default branch when it IS the linked MR', async () => { + getProjectRefMock.mockResolvedValueOnce({ host: 'gitlab.com', path: 'g/p' }) + glabExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + iid: 7, + title: 'Linked trunk MR', + state: 'merged', + sha: 'abc', + head_pipeline: { status: 'success' } + } + ]) + }) + + const mr = await getMergeRequestForBranch('/repo', 'main', 7) + + expect(mr).toMatchObject({ number: 7, state: 'merged' }) + // Exempted by linked-number match — no fallback refetch needed. + expect(glabExecFileAsyncMock).toHaveBeenCalledTimes(1) + }) + it('returns null for an empty / detached-HEAD branch arg', async () => { // Why: during a rebase the branch is empty — mirror github/getPRForBranch's // early return without calling glab. diff --git a/src/main/gitlab/client.ts b/src/main/gitlab/client.ts index a179ea4be86..d2f30b2d531 100644 --- a/src/main/gitlab/client.ts +++ b/src/main/gitlab/client.ts @@ -45,6 +45,7 @@ import { getHostedReviewLocalGitOptions, type HostedReviewExecutionOptions } from '../source-control/hosted-review-git-options' +import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-default-branch' // Why: glab REST addresses projects by URL-encoded path; escapes slashes for nested groups. function encodedProject(projectPath: string): string { @@ -327,7 +328,21 @@ export async function getMergeRequestForBranch( const raw = data[0] // Why: older GitLab list payloads expose `pipeline` instead of `head_pipeline`. const pipelineStatus = derivePipelineStatus(raw.head_pipeline ?? raw.pipeline ?? null) - return mapMRInfo(raw, pipelineStatus) + const info = mapMRInfo(raw, pipelineStatus) + // Why (#9171): discard a non-open implicit branch match on the repo + // default branch and fall through to the linked-iid fallback below. + const hideOnDefaultBranch = await shouldHideNonOpenReviewOnDefaultBranch({ + state: info.state, + reviewNumber: info.number, + linkedReviewNumber: linkedMRIid, + branchName, + repoPath, + connectionId, + localGitOptions + }) + if (!hideOnDefaultBranch) { + return info + } } } if (typeof linkedMRIid !== 'number') { diff --git a/src/main/source-control/repo-default-branch.test.ts b/src/main/source-control/repo-default-branch.test.ts new file mode 100644 index 00000000000..0c5984ae9bc --- /dev/null +++ b/src/main/source-control/repo-default-branch.test.ts @@ -0,0 +1,272 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock, getSshGitProviderMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + getSshGitProviderMock: vi.fn() +})) + +vi.mock('../git/runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock +})) + +vi.mock('../providers/ssh-git-dispatch', () => ({ + getSshGitProvider: getSshGitProviderMock +})) + +import { + getRepoDefaultBranchName, + shouldHideNonOpenReviewOnDefaultBranch, + __resetRepoDefaultBranchCacheForTests +} from './repo-default-branch' + +function primeLocalGitExec(defaultRef = 'refs/remotes/origin/master'): void { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'symbolic-ref' && args.includes('refs/remotes/origin/HEAD')) { + return { stdout: `${defaultRef}\n`, stderr: '' } + } + if (args[0] === 'rev-parse' && args[1] === '--verify' && args.includes(defaultRef)) { + return { stdout: 'default-oid\n', stderr: '' } + } + throw new Error(`unexpected git call: ${args.join(' ')}`) + }) +} + +describe('getRepoDefaultBranchName', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + getSshGitProviderMock.mockReset() + __resetRepoDefaultBranchCacheForTests() + }) + + it('resolves the default branch name locally, stripping the origin/ remote prefix', async () => { + primeLocalGitExec('refs/remotes/origin/master') + + await expect(getRepoDefaultBranchName('/repo')).resolves.toBe('master') + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['symbolic-ref', '--quiet', 'refs/remotes/origin/HEAD'], + // Why: the timeout keeps a dead filesystem from wedging the serial PR + // refresh drain — assert it stays armed on the local path. + { cwd: '/repo', timeout: expect.any(Number) } + ) + }) + + it('forwards the WSL distro to the local git exec options', async () => { + primeLocalGitExec('refs/remotes/origin/main') + + await expect(getRepoDefaultBranchName('/repo', null, { wslDistro: 'Ubuntu' })).resolves.toBe( + 'main' + ) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['symbolic-ref', '--quiet', 'refs/remotes/origin/HEAD'], + { cwd: '/repo', wslDistro: 'Ubuntu', timeout: expect.any(Number) } + ) + }) + + it('routes resolution through the SSH provider exec and never local git', async () => { + const provider = { + exec: vi.fn(async (args: string[], repoPath: string) => { + expect(repoPath).toBe('/remote/repo') + if (args[0] === 'symbolic-ref') { + return { stdout: 'refs/remotes/origin/trunk\n' } + } + return { stdout: 'oid\n' } + }) + } + getSshGitProviderMock.mockReturnValue(provider) + + await expect(getRepoDefaultBranchName('/remote/repo', 'ssh-1')).resolves.toBe('trunk') + expect(getSshGitProviderMock).toHaveBeenCalledWith('ssh-1') + expect(provider.exec).toHaveBeenCalledWith( + ['symbolic-ref', '--quiet', 'refs/remotes/origin/HEAD'], + '/remote/repo', + { timeoutMs: expect.any(Number) } + ) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('shares one wall-clock timeout budget across all fallback probes', async () => { + let now = 1_000 + const dateNow = vi.spyOn(Date, 'now').mockImplementation(() => now) + gitExecFileAsyncMock.mockImplementation(async (_args, options: { timeout: number }) => { + now += options.timeout + throw new Error('git timed out.') + }) + + try { + await expect(getRepoDefaultBranchName('/repo')).resolves.toBeNull() + + // Once the first probe consumes the budget, fallback refs fail open + // without spawning four more equally slow git processes. + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(gitExecFileAsyncMock.mock.calls[0]?.[1]).toEqual({ + cwd: '/repo', + timeout: 15_000 + }) + } finally { + dateNow.mockRestore() + } + }) + + it('returns null without running local git when the SSH provider is unavailable', async () => { + getSshGitProviderMock.mockReturnValue(undefined) + + await expect(getRepoDefaultBranchName('/remote/repo', 'ssh-gone')).resolves.toBeNull() + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('returns null when no default branch is resolvable (fail open)', async () => { + gitExecFileAsyncMock.mockRejectedValue(new Error('fatal: not a git repository')) + + await expect(getRepoDefaultBranchName('/repo')).resolves.toBeNull() + }) + + it('serves cached results within the TTL and re-executes after a reset', async () => { + primeLocalGitExec('refs/remotes/origin/master') + + await expect(getRepoDefaultBranchName('/repo')).resolves.toBe('master') + const callsAfterFirst = gitExecFileAsyncMock.mock.calls.length + await expect(getRepoDefaultBranchName('/repo')).resolves.toBe('master') + expect(gitExecFileAsyncMock.mock.calls.length).toBe(callsAfterFirst) + + __resetRepoDefaultBranchCacheForTests() + await expect(getRepoDefaultBranchName('/repo')).resolves.toBe('master') + expect(gitExecFileAsyncMock.mock.calls.length).toBeGreaterThan(callsAfterFirst) + }) + + it('coalesces concurrent resolutions for the same repo and runtime', async () => { + let releaseSymbolicRef: (() => void) | undefined + const symbolicRefGate = new Promise((resolve) => { + releaseSymbolicRef = resolve + }) + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'symbolic-ref') { + await symbolicRefGate + return { stdout: 'refs/remotes/origin/main\n', stderr: '' } + } + if (args[0] === 'rev-parse') { + return { stdout: 'default-oid\n', stderr: '' } + } + throw new Error(`unexpected git call: ${args.join(' ')}`) + }) + + const first = getRepoDefaultBranchName('/repo') + const second = getRepoDefaultBranchName('/repo') + await vi.waitFor(() => expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1)) + releaseSymbolicRef?.() + + await expect(Promise.all([first, second])).resolves.toEqual(['main', 'main']) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + }) + + it('scopes the cache per runtime so WSL and host resolutions do not collide', async () => { + primeLocalGitExec('refs/remotes/origin/master') + await expect(getRepoDefaultBranchName('/repo')).resolves.toBe('master') + + primeLocalGitExec('refs/remotes/origin/main') + // Same repoPath, different runtime key → fresh resolution, not the host's. + await expect(getRepoDefaultBranchName('/repo', null, { wslDistro: 'Ubuntu' })).resolves.toBe( + 'main' + ) + }) +}) + +describe('shouldHideNonOpenReviewOnDefaultBranch', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + getSshGitProviderMock.mockReset() + __resetRepoDefaultBranchCacheForTests() + }) + + it('never resolves the default branch for open or draft reviews (lazy)', async () => { + for (const state of ['open', 'opened', 'draft']) { + await expect( + shouldHideNonOpenReviewOnDefaultBranch({ + state, + reviewNumber: 1, + branchName: 'master', + repoPath: '/repo' + }) + ).resolves.toBe(false) + } + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('hides a closed review whose branch is the repo default branch', async () => { + primeLocalGitExec('refs/remotes/origin/master') + + await expect( + shouldHideNonOpenReviewOnDefaultBranch({ + state: 'closed', + reviewNumber: 7, + branchName: 'master', + repoPath: '/repo' + }) + ).resolves.toBe(true) + }) + + it('hides a stuck-locked review whose branch is the repo default branch', async () => { + primeLocalGitExec('refs/remotes/origin/master') + + await expect( + shouldHideNonOpenReviewOnDefaultBranch({ + state: 'locked', + reviewNumber: 7, + branchName: 'master', + repoPath: '/repo' + }) + ).resolves.toBe(true) + }) + + it('keeps a closed review on a non-default branch', async () => { + primeLocalGitExec('refs/remotes/origin/master') + + await expect( + shouldHideNonOpenReviewOnDefaultBranch({ + state: 'closed', + reviewNumber: 7, + branchName: 'feature-x', + repoPath: '/repo' + }) + ).resolves.toBe(false) + }) + + it('exempts the explicitly linked review without resolving the default branch', async () => { + await expect( + shouldHideNonOpenReviewOnDefaultBranch({ + state: 'merged', + reviewNumber: 7, + linkedReviewNumber: 7, + branchName: 'master', + repoPath: '/repo' + }) + ).resolves.toBe(false) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('still hides a non-open shadow whose number differs from the linked review', async () => { + primeLocalGitExec('refs/remotes/origin/master') + + await expect( + shouldHideNonOpenReviewOnDefaultBranch({ + state: 'merged', + reviewNumber: 7, + linkedReviewNumber: 42, + branchName: 'master', + repoPath: '/repo' + }) + ).resolves.toBe(true) + }) + + it('fails open when the default branch is unresolvable', async () => { + gitExecFileAsyncMock.mockRejectedValue(new Error('fatal: not a git repository')) + + await expect( + shouldHideNonOpenReviewOnDefaultBranch({ + state: 'closed', + reviewNumber: 7, + branchName: 'master', + repoPath: '/repo' + }) + ).resolves.toBe(false) + }) +}) diff --git a/src/main/source-control/repo-default-branch.ts b/src/main/source-control/repo-default-branch.ts new file mode 100644 index 00000000000..b5c4f77d2c8 --- /dev/null +++ b/src/main/source-control/repo-default-branch.ts @@ -0,0 +1,165 @@ +import { resolveDefaultBaseRefViaExec } from '../git/repo' +import { gitExecFileAsync } from '../git/runner' +import { getSshGitProvider } from '../providers/ssh-git-dispatch' +import type { HostedReviewLocalGitOptions } from './hosted-review-git-options' + +// Why: bounded like TRACKED_UPSTREAM_SNAPSHOT_CACHE in github/client.ts — PR +// refresh ticks re-ask per repo, and worktree churn can mint unbounded keys. +const REPO_DEFAULT_BRANCH_CACHE_TTL_MS = 30_000 +const REPO_DEFAULT_BRANCH_CACHE_MAX_ENTRIES = 512 +const REPO_DEFAULT_BRANCH_RESOLUTION_BUDGET_MS = 15_000 + +type RepoDefaultBranchCacheEntry = { + expiresAt: number + branchName: string | null +} + +const repoDefaultBranchCache = new Map() +const repoDefaultBranchInFlight = new Map>() + +function getRepoDefaultBranchCacheKey( + repoPath: string, + connectionId?: string | null, + localGitOptions: HostedReviewLocalGitOptions = {} +): string { + // Why: scope per executing git host (native/WSL/SSH) so hosts with + // different clones of the "same" path cannot cross-contaminate. + const runtimeKey = connectionId + ? `ssh:${connectionId}` + : `local:${localGitOptions.wslDistro ?? 'host'}` + return [runtimeKey, repoPath].join('\0') +} + +function pruneRepoDefaultBranchCache(now: number): void { + for (const [key, entry] of repoDefaultBranchCache) { + if (entry.expiresAt <= now) { + repoDefaultBranchCache.delete(key) + } + } + while (repoDefaultBranchCache.size > REPO_DEFAULT_BRANCH_CACHE_MAX_ENTRIES) { + const oldestKey = repoDefaultBranchCache.keys().next().value + if (oldestKey === undefined) { + break + } + repoDefaultBranchCache.delete(oldestKey) + } +} + +/** + * Resolve the repository's default branch NAME (`main`, `master`, …) over the + * transport the surrounding hosted-review call already uses. Returns null when + * unresolvable so callers fail open (#9171 guard skipped, current behavior). + */ +export async function getRepoDefaultBranchName( + repoPath: string, + connectionId?: string | null, + localGitOptions: HostedReviewLocalGitOptions = {} +): Promise { + const cacheKey = getRepoDefaultBranchCacheKey(repoPath, connectionId, localGitOptions) + const now = Date.now() + const cached = repoDefaultBranchCache.get(cacheKey) + if (cached && cached.expiresAt > now) { + return cached.branchName + } + const pending = repoDefaultBranchInFlight.get(cacheKey) + if (pending) { + return pending + } + + // Why: simultaneous refresh paths for one checkout should share the same + // Git/SSH subprocess chain instead of multiplying cold-cache probes. + const resolution = (async (): Promise => { + let branchName: string | null = null + try { + const provider = connectionId ? getSshGitProvider(connectionId) : null + if (connectionId && !provider) { + // Why: a dropped SSH provider must not fall back to local git — the + // repoPath is remote, so a local run could answer for the wrong repo. + return null + } + const resolutionDeadline = Date.now() + REPO_DEFAULT_BRANCH_RESOLUTION_BUDGET_MS + // Why: the resolver can try five refs; share one deadline so an unhealthy + // local/WSL/SSH host cannot multiply the refresh delay per fallback probe. + const baseRef = await resolveDefaultBaseRefViaExec((argv) => { + const timeoutMs = resolutionDeadline - Date.now() + if (timeoutMs <= 0) { + return Promise.reject(new Error('Default branch resolution timed out.')) + } + return provider + ? provider.exec(argv, repoPath, { timeoutMs }) + : gitExecFileAsync(argv, { + cwd: repoPath, + ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}), + timeout: timeoutMs + }) + }) + // Same base-ref → branch-name normalization as git/repo.ts getRemoteFileUrl. + branchName = baseRef ? baseRef.replace(/^origin\//, '') : null + } catch { + branchName = null + } + const completedAt = Date.now() + // Why: null (failure or genuinely no default) is cached too — the resolver + // cannot tell them apart, and the short TTL bounds the fail-open window + // without re-probing git on every refresh tick. + pruneRepoDefaultBranchCache(completedAt) + repoDefaultBranchCache.set(cacheKey, { + branchName, + expiresAt: completedAt + REPO_DEFAULT_BRANCH_CACHE_TTL_MS + }) + pruneRepoDefaultBranchCache(completedAt) + return branchName + })() + repoDefaultBranchInFlight.set(cacheKey, resolution) + try { + return await resolution + } finally { + if (repoDefaultBranchInFlight.get(cacheKey) === resolution) { + repoDefaultBranchInFlight.delete(cacheKey) + } + } +} + +/** + * #9171 invariant, applied by every provider client at its branch-lookup choke + * point: an implicit branch-name match on the repository's default branch must + * never surface a non-open review. The explicitly linked review is exempt. + * Resolves the default branch lazily — only for non-open, non-linked results — + * so the common refresh path adds zero git calls. + */ +export async function shouldHideNonOpenReviewOnDefaultBranch(input: { + /** Provider-normalized review state; 'closed' / 'merged' / 'locked' count as non-open. */ + state: string + reviewNumber: number | null + linkedReviewNumber?: number | null + branchName: string + repoPath: string + connectionId?: string | null + localGitOptions?: HostedReviewLocalGitOptions +}): Promise { + // Why: GitLab 'locked' is non-open too — normally a seconds-long merge + // transition, but a stuck-locked trunk MR would otherwise attach forever. + if (input.state !== 'closed' && input.state !== 'merged' && input.state !== 'locked') { + return false + } + if ( + typeof input.linkedReviewNumber === 'number' && + input.reviewNumber === input.linkedReviewNumber + ) { + return false + } + if (!input.branchName) { + return false + } + const defaultBranchName = await getRepoDefaultBranchName( + input.repoPath, + input.connectionId, + input.localGitOptions + ) + return defaultBranchName !== null && input.branchName === defaultBranchName +} + +export function __resetRepoDefaultBranchCacheForTests(): void { + repoDefaultBranchCache.clear() + repoDefaultBranchInFlight.clear() +} From 4f81dfc128d746e58e43002190a43e5d1edde950 Mon Sep 17 00:00:00 2001 From: Jinho Choi <83327025+nasagong@users.noreply.github.com> Date: Tue, 21 Jul 2026 04:18:57 +0900 Subject: [PATCH 4/5] perf(ssh): cut warm high-latency connects from 88.7s to 7.7s (#9015) Move managed agent-hook filesystem work behind one relay RPC so high-latency SSH connects pay one WAN round trip instead of hundreds. Keep installers serial, lock shared account config across relay processes, and fence cancelled connection generations from replacement state. Co-authored-by: nasagong Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- config/scripts/build-relay.mjs | 31 +- .../managed-hook-install-lock.test.ts | 397 ++++++++++++++++++ .../agent-hooks/managed-hook-install-lock.ts | 142 +++++++ .../managed-hook-local-filesystem.test.ts | 71 ++++ .../managed-hook-local-filesystem.ts | 61 +++ .../agent-hooks/managed-hook-lock-claims.ts | 216 ++++++++++ .../agent-hooks/managed-hook-lock-records.ts | 274 ++++++++++++ .../managed-hook-owner-identity.test.ts | 181 ++++++++ .../managed-hook-owner-identity.ts | 209 +++++++++ .../agent-hooks/managed-hook-runtime.test.ts | 22 + src/main/agent-hooks/managed-hook-runtime.ts | 106 +++++ .../managed-hook-stdin-lifecycle.test.ts | 26 +- .../agent-hooks/managed-hook-timeout.test.ts | 23 +- .../remote-hook-service-installers.test.ts | 29 ++ .../remote-managed-hook-installers.ts | 6 + src/main/ipc/ssh.test.ts | 43 ++ src/main/ipc/ssh.ts | 79 +++- src/main/ssh/ssh-connection-manager.test.ts | 74 ++++ src/main/ssh/ssh-connection-manager.ts | 25 +- src/main/ssh/ssh-connection.test.ts | 33 ++ src/main/ssh/ssh-connection.ts | 17 + src/main/ssh/ssh-relay-session.test.ts | 139 +++--- src/main/ssh/ssh-relay-session.ts | 109 +---- .../ssh/ssh-relay-versioned-install.test.ts | 3 +- src/main/ssh/ssh-relay-versioned-install.ts | 5 +- src/main/ssh/ssh-remote-commands.test.ts | 8 +- src/main/ssh/ssh-remote-commands.ts | 5 +- .../ssh-system-transport.integration.test.ts | 1 + src/relay/managed-hook-installer.test.ts | 63 +++ src/relay/managed-hook-installer.ts | 56 +++ src/relay/relay.ts | 4 + src/shared/agent-hook-relay.ts | 9 + 32 files changed, 2263 insertions(+), 204 deletions(-) create mode 100644 src/main/agent-hooks/managed-hook-install-lock.test.ts create mode 100644 src/main/agent-hooks/managed-hook-install-lock.ts create mode 100644 src/main/agent-hooks/managed-hook-local-filesystem.test.ts create mode 100644 src/main/agent-hooks/managed-hook-local-filesystem.ts create mode 100644 src/main/agent-hooks/managed-hook-lock-claims.ts create mode 100644 src/main/agent-hooks/managed-hook-lock-records.ts create mode 100644 src/main/agent-hooks/managed-hook-owner-identity.test.ts create mode 100644 src/main/agent-hooks/managed-hook-owner-identity.ts create mode 100644 src/main/agent-hooks/managed-hook-runtime.test.ts create mode 100644 src/main/agent-hooks/managed-hook-runtime.ts create mode 100644 src/main/ssh/ssh-connection-manager.test.ts create mode 100644 src/relay/managed-hook-installer.test.ts create mode 100644 src/relay/managed-hook-installer.ts diff --git a/config/scripts/build-relay.mjs b/config/scripts/build-relay.mjs index cb7e01c9700..6afd517af88 100644 --- a/config/scripts/build-relay.mjs +++ b/config/scripts/build-relay.mjs @@ -18,6 +18,14 @@ const __dirname = import.meta.dirname const ROOT = join(__dirname, '..', '..') const RELAY_ENTRY = join(ROOT, 'src', 'relay', 'relay.ts') const WATCHER_ENTRY = join(ROOT, 'src', 'main', 'ipc', 'parcel-watcher-process-entry.ts') +const MANAGED_HOOK_RUNTIME_ENTRY = join( + ROOT, + 'src', + 'main', + 'agent-hooks', + 'managed-hook-runtime.ts' +) +const JSONC_PARSER_ESM_ENTRY = join(ROOT, 'node_modules', 'jsonc-parser', 'lib', 'esm', 'main.js') const PLATFORMS = [ 'linux-x64', @@ -66,14 +74,33 @@ for (const platform of PLATFORMS) { } }) + await build({ + entryPoints: [MANAGED_HOOK_RUNTIME_ENTRY], + bundle: true, + platform: 'node', + target: 'node18', + format: 'cjs', + outfile: join(outDir, 'managed-hook-runtime.js'), + // Why: jsonc-parser's default UMD build keeps relative dynamic requires + // that break after bundling; its ESM entry is equivalent and self-contained. + alias: { 'jsonc-parser': JSONC_PARSER_ESM_ENTRY }, + sourcemap: false, + minify: true, + define: { + 'process.env.NODE_ENV': '"production"' + } + }) + // Why: include a content hash so the deploy check detects code changes - // even when RELAY_VERSION hasn't been bumped. Hash both process artifacts - // so a watcher-only change always deploys beside the matching relay host. + // even when RELAY_VERSION hasn't been bumped. Hash every executable module + // so a companion-only change always deploys beside the matching relay host. const relayContent = readFileSync(join(outDir, 'relay.js')) const watcherContent = readFileSync(join(outDir, 'relay-watcher.js')) + const managedHookRuntimeContent = readFileSync(join(outDir, 'managed-hook-runtime.js')) const hash = createHash('sha256') .update(relayContent) .update(watcherContent) + .update(managedHookRuntimeContent) .digest('hex') .slice(0, 12) writeFileSync(join(outDir, '.version'), `${RELAY_VERSION}+${hash}`) diff --git a/src/main/agent-hooks/managed-hook-install-lock.test.ts b/src/main/agent-hooks/managed-hook-install-lock.test.ts new file mode 100644 index 00000000000..d6563064f5e --- /dev/null +++ b/src/main/agent-hooks/managed-hook-install-lock.test.ts @@ -0,0 +1,397 @@ +import { + link, + lstat, + mkdir, + mkdtemp, + readFile, + readdir, + rename, + rm, + unlink, + utimes, + writeFile +} from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' +import * as ownerIdentity from './managed-hook-owner-identity' +import { withManagedHookInstallLock } from './managed-hook-install-lock' + +const tempHomes: string[] = [] +const STALE_TOKEN = '00000000-0000-4000-8000-000000000000' +const STALE_CLAIM_TOKEN = '11111111-1111-4111-8111-111111111111' +const fsFailure = vi.hoisted(() => ({ + canonicalUnlinkPath: null as string | null, + contendWithoutLock: null as (() => void) | null +})) + +vi.mock('node:fs/promises', async (importOriginal) => { + const original = await importOriginal<{ link: typeof link; unlink: typeof unlink }>() + return { + ...original, + link: async (...args: Parameters) => { + if (fsFailure.contendWithoutLock && String(args[1]).endsWith('managed-hook-install.lock')) { + fsFailure.contendWithoutLock() + throw Object.assign(new Error('injected publication contention'), { code: 'EEXIST' }) + } + await original.link(...args) + }, + unlink: async (path: Parameters[0]) => { + if (String(path) === fsFailure.canonicalUnlinkPath) { + fsFailure.canonicalUnlinkPath = null + throw Object.assign(new Error('injected unlink failure'), { code: 'EACCES' }) + } + await original.unlink(path) + } + } +}) + +async function createTempHome(): Promise { + const home = await mkdtemp(join(tmpdir(), 'orca-managed-hook-lock-')) + tempHomes.push(home) + return home +} + +async function requireHostIdentity(): Promise { + const hostIdentity = await ownerIdentity.readManagedHookHostIdentity() + if (!hostIdentity) { + throw new Error('test host identity is unavailable') + } + return hostIdentity +} + +async function createOwnedLock( + home: string, + processIdentity: string, + hostIdentity?: string +): Promise { + const lockHostIdentity = hostIdentity ?? (await requireHostIdentity()) + const lockParent = join(home, '.orca') + const ownerPath = join(lockParent, `managed-hook-install.owner-${STALE_TOKEN}.json`) + await mkdir(lockParent, { recursive: true }) + await writeFile( + ownerPath, + JSON.stringify({ + token: STALE_TOKEN, + pid: process.pid, + hostIdentity: lockHostIdentity, + processIdentity + }) + ) + await link(ownerPath, join(lockParent, 'managed-hook-install.lock')) +} + +afterEach(async () => { + fsFailure.canonicalUnlinkPath = null + fsFailure.contendWithoutLock = null + vi.restoreAllMocks() + vi.unstubAllEnvs() + await Promise.all(tempHomes.splice(0).map((home) => rm(home, { recursive: true, force: true }))) +}) + +describe.skipIf(process.platform === 'win32')('withManagedHookInstallLock', () => { + it('enforces the deadline when publication contention observes a missing lock', async () => { + const home = await createTempHome() + const controller = new AbortController() + const run = vi.fn() + let now = 0 + fsFailure.contendWithoutLock = () => controller.abort() + vi.spyOn(Date, 'now').mockImplementation(() => (now += 10_000)) + + await expect(withManagedHookInstallLock(home, controller.signal, run)).rejects.toThrow( + 'Timed out waiting for another managed-hook install to finish' + ) + expect(run).not.toHaveBeenCalled() + }) + + it('serializes installers across relay runtime instances', async () => { + const home = await createTempHome() + let releaseFirst!: () => void + let markFirstStarted!: () => void + const firstStarted = new Promise((resolve) => (markFirstStarted = resolve)) + const secondRun = vi.fn(async () => 'second') + const first = withManagedHookInstallLock(home, undefined, () => { + markFirstStarted() + return new Promise((resolve) => (releaseFirst = () => resolve('first'))) + }) + await firstStarted + const second = withManagedHookInstallLock(home, undefined, secondRun) + + await new Promise((resolve) => setTimeout(resolve, 40)) + expect(secondRun).not.toHaveBeenCalled() + releaseFirst() + + await expect(first).resolves.toBe('first') + await expect(second).resolves.toBe('second') + }) + + it('atomically publishes a complete owner record before entering the installer', async () => { + const home = await createTempHome() + const lockParent = join(home, '.orca') + const lockPath = join(lockParent, 'managed-hook-install.lock') + + await withManagedHookInstallLock(home, undefined, async () => { + const entries = await readdir(lockParent) + const ownerEntry = entries.find((entry) => entry.startsWith('managed-hook-install.owner-')) + expect(entries).toHaveLength(2) + expect(ownerEntry).toBeDefined() + if (!ownerEntry) { + throw new Error('owner entry was not published') + } + const ownerPath = join(lockParent, ownerEntry) + const owner = JSON.parse(await readFile(lockPath, 'utf8')) as { + token: string + pid: number + hostIdentity: string + processIdentity: string + } + expect(owner).toMatchObject({ pid: process.pid }) + expect(owner.token).toMatch(/^[\da-f-]{36}$/) + expect(owner.hostIdentity.length).toBeGreaterThan(0) + expect(owner.processIdentity.length).toBeGreaterThan(0) + const [lockStats, ownerStats] = await Promise.all([lstat(lockPath), lstat(ownerPath)]) + expect(lockStats.ino).toBe(ownerStats.ino) + expect(lockStats.nlink).toBe(2) + }) + }) + + it('fails fast instead of stealing an unverifiable legacy directory lock', async () => { + const home = await createTempHome() + await mkdir(join(home, '.orca', 'managed-hook-install.lock'), { recursive: true }) + const run = vi.fn() + + await expect(withManagedHookInstallLock(home, undefined, run)).rejects.toThrow( + 'unverifiable owner' + ) + expect(run).not.toHaveBeenCalled() + }) + + it('recovers after the same SSH host reboots even if its PID and start time are reused', async () => { + const home = await createTempHome() + await createOwnedLock(home, 'stale-process-incarnation') + + await expect( + withManagedHookInstallLock(home, undefined, async () => 'installed') + ).resolves.toBe('installed') + }) + + it('does not compare PID identities or steal a lock owned by another SSH host', async () => { + const home = await createTempHome() + const lockPath = join(home, '.orca', 'managed-hook-install.lock') + await createOwnedLock(home, 'stale-process-incarnation', 'another-host-boot') + const originalLock = await readFile(lockPath, 'utf8') + + await expect(withManagedHookInstallLock(home, undefined, vi.fn())).rejects.toThrow( + 'belongs to another host' + ) + expect(await readFile(lockPath, 'utf8')).toBe(originalLock) + }) + + it('does not steal a live lock when its owner process probe becomes unavailable', async () => { + const home = await createTempHome() + const lockPath = join(home, '.orca', 'managed-hook-install.lock') + await createOwnedLock(home, 'another-process-incarnation') + const originalLock = await readFile(lockPath, 'utf8') + const selfIdentity = await ownerIdentity.readManagedHookProcessIdentity(process.pid) + expect(selfIdentity).toBeTypeOf('string') + vi.spyOn(ownerIdentity, 'readManagedHookProcessIdentity') + .mockResolvedValueOnce(selfIdentity) + .mockResolvedValueOnce(undefined) + + await expect(withManagedHookInstallLock(home, undefined, vi.fn())).rejects.toThrow( + 'Could not verify the managed-hook lock owner process' + ) + expect(await readFile(lockPath, 'utf8')).toBe(originalLock) + }) + + it('serializes two contenders that concurrently recover the same dead owner', async () => { + const home = await createTempHome() + await createOwnedLock(home, 'stale-process-incarnation') + + let releaseFirst!: () => void + let activeRuns = 0 + let maxActiveRuns = 0 + const run = vi.fn(async () => { + activeRuns += 1 + maxActiveRuns = Math.max(maxActiveRuns, activeRuns) + if (run.mock.calls.length === 1) { + await new Promise((resolve) => (releaseFirst = resolve)) + } + activeRuns -= 1 + }) + const first = withManagedHookInstallLock(home, undefined, run) + const second = withManagedHookInstallLock(home, undefined, run) + + await vi.waitFor(() => expect(run).toHaveBeenCalledTimes(1)) + expect(maxActiveRuns).toBe(1) + releaseFirst() + await Promise.all([first, second]) + expect(run).toHaveBeenCalledTimes(2) + expect(maxActiveRuns).toBe(1) + }) + + it('recovers a canonical lock after its previous recovery claimant crashes', async () => { + const home = await createTempHome() + const lockParent = join(home, '.orca') + const ownerPath = join(lockParent, `managed-hook-install.owner-${STALE_TOKEN}.json`) + const claimedOwnerPath = join( + lockParent, + `managed-hook-install.claimed-${STALE_TOKEN}-${STALE_CLAIM_TOKEN}.json` + ) + const claimRecordPath = join( + lockParent, + `managed-hook-install.claim-${STALE_TOKEN}-${STALE_CLAIM_TOKEN}.json` + ) + const hostIdentity = await requireHostIdentity() + await createOwnedLock(home, 'stale-owner-incarnation', hostIdentity) + await writeFile( + claimRecordPath, + JSON.stringify({ + ownerToken: STALE_TOKEN, + claimToken: STALE_CLAIM_TOKEN, + pid: process.pid, + hostIdentity, + processIdentity: 'stale-claimant-incarnation' + }) + ) + await rename(ownerPath, claimedOwnerPath) + + await expect( + withManagedHookInstallLock(home, undefined, async () => 'installed') + ).resolves.toBe('installed') + expect(await readdir(lockParent)).toEqual([]) + }) + + it('fails fast when a recovery claimant disappears before removing the canonical lock', async () => { + const home = await createTempHome() + const lockParent = join(home, '.orca') + const lockPath = join(lockParent, 'managed-hook-install.lock') + const ownerPath = join(lockParent, `managed-hook-install.owner-${STALE_TOKEN}.json`) + await createOwnedLock(home, 'stale-process-incarnation') + await unlink(ownerPath) + + const startedAt = Date.now() + await expect(withManagedHookInstallLock(home, undefined, vi.fn())).rejects.toThrow( + 'unverifiable recovery claim' + ) + expect(Date.now() - startedAt).toBeLessThan(500) + expect(await readFile(lockPath, 'utf8')).toContain(STALE_TOKEN) + }) + + it('cleans an unlinked owner draft left by a crashed acquisition', async () => { + const home = await createTempHome() + const lockParent = join(home, '.orca') + const staleOwnerEntry = `managed-hook-install.owner-${STALE_TOKEN}.json` + const ownerPath = join(lockParent, staleOwnerEntry) + await mkdir(lockParent, { recursive: true }) + const hostIdentity = await requireHostIdentity() + await writeFile( + ownerPath, + JSON.stringify({ + token: STALE_TOKEN, + pid: process.pid, + hostIdentity, + processIdentity: 'stale-process-incarnation' + }) + ) + + await withManagedHookInstallLock(home, undefined, async () => { + expect(await readdir(lockParent)).not.toContain(staleOwnerEntry) + }) + expect(await readdir(lockParent)).toEqual([]) + }) + + it('does not delete a malformed final owner record that an older relay may still publish', async () => { + const home = await createTempHome() + const lockParent = join(home, '.orca') + const staleOwnerEntry = `managed-hook-install.owner-${STALE_TOKEN}.json` + const ownerPath = join(lockParent, staleOwnerEntry) + await mkdir(lockParent, { recursive: true }) + await writeFile(ownerPath, '{"token":') + const staleTime = new Date(Date.now() - 2_000) + await utimes(ownerPath, staleTime, staleTime) + + await withManagedHookInstallLock(home, undefined, async () => { + expect(await readdir(lockParent)).toContain(staleOwnerEntry) + }) + expect(await readdir(lockParent)).toEqual([staleOwnerEntry]) + }) + + it('does not let a late release delete a replacement owner lock', async () => { + const home = await createTempHome() + const lockPath = join(home, '.orca', 'managed-hook-install.lock') + let releaseFirst!: () => void + let markFirstStarted!: () => void + const firstStarted = new Promise((resolve) => (markFirstStarted = resolve)) + const first = withManagedHookInstallLock(home, undefined, () => { + markFirstStarted() + return new Promise((resolve) => (releaseFirst = resolve)) + }) + await firstStarted + await rename(lockPath, `${lockPath}.displaced`) + + let releaseSecond!: () => void + let markSecondStarted!: () => void + const secondStarted = new Promise((resolve) => (markSecondStarted = resolve)) + const second = withManagedHookInstallLock(home, undefined, () => { + markSecondStarted() + return new Promise((resolve) => (releaseSecond = resolve)) + }) + await secondStarted + + releaseFirst() + await first + const thirdRun = vi.fn() + const controller = new AbortController() + const third = withManagedHookInstallLock(home, controller.signal, thirdRun) + await new Promise((resolve) => setTimeout(resolve, 40)) + expect(thirdRun).not.toHaveBeenCalled() + controller.abort() + await expect(third).rejects.toMatchObject({ name: 'AbortError' }) + + releaseSecond() + await second + }) + + it('finishes an abandoned same-process release before the next install', async () => { + const home = await createTempHome() + const lockParent = join(home, '.orca') + const lockPath = join(lockParent, 'managed-hook-install.lock') + fsFailure.canonicalUnlinkPath = lockPath + + await expect(withManagedHookInstallLock(home, undefined, async () => 'first')).resolves.toBe( + 'first' + ) + expect(await readdir(lockParent)).toContain('managed-hook-install.lock') + + const startedAt = Date.now() + await expect(withManagedHookInstallLock(home, undefined, async () => 'second')).resolves.toBe( + 'second' + ) + expect(Date.now() - startedAt).toBeLessThan(500) + expect(await readdir(lockParent)).toEqual([]) + }) + + it('cancels a request waiting behind another relay runtime', async () => { + const home = await createTempHome() + let releaseFirst!: () => void + let markFirstStarted!: () => void + const firstStarted = new Promise((resolve) => (markFirstStarted = resolve)) + const first = withManagedHookInstallLock(home, undefined, () => { + markFirstStarted() + return new Promise((resolve) => (releaseFirst = resolve)) + }) + await firstStarted + const controller = new AbortController() + const secondRun = vi.fn() + const second = withManagedHookInstallLock(home, controller.signal, secondRun) + await new Promise((resolve) => setTimeout(resolve, 40)) + + controller.abort() + + await expect(second).rejects.toMatchObject({ name: 'AbortError' }) + expect(secondRun).not.toHaveBeenCalled() + releaseFirst() + await first + }) +}) diff --git a/src/main/agent-hooks/managed-hook-install-lock.ts b/src/main/agent-hooks/managed-hook-install-lock.ts new file mode 100644 index 00000000000..c283b99c41b --- /dev/null +++ b/src/main/agent-hooks/managed-hook-install-lock.ts @@ -0,0 +1,142 @@ +import { mkdir } from 'node:fs/promises' +import { join } from 'node:path' +import { setTimeout as delay } from 'node:timers/promises' +import { removeManagedHookLock } from './managed-hook-lock-claims' +import { + cleanupManagedHookLockFiles, + deactivateManagedHookLockOwner, + inspectManagedHookLock, + isManagedHookLockOwnerActive, + tryCreateManagedHookLock, + type ManagedHookLockOwner +} from './managed-hook-lock-records' +import { + readManagedHookHostIdentity, + readManagedHookProcessIdentity +} from './managed-hook-owner-identity' + +const LOCK_WAIT_TIMEOUT_MS = 10_000 +const LOCK_RETRY_MS = 20 + +async function releaseInstallLock( + lockPath: string, + lockParent: string, + owner: ManagedHookLockOwner, + hostIdentity: string, + processIdentity: string +): Promise { + try { + const removal = await removeManagedHookLock( + lockPath, + lockParent, + owner, + hostIdentity, + processIdentity + ) + if (removal !== 'removed') { + console.warn(`[agent-hooks] Failed to release managed-hook install lock: ${removal}`) + } + } catch (error) { + console.warn('[agent-hooks] Failed to release managed-hook install lock', error) + } finally { + deactivateManagedHookLockOwner(owner.token) + } +} + +async function acquireInstallLock( + home: string, + signal?: AbortSignal, + suppliedHostIdentity?: string +): Promise<() => Promise> { + const lockParent = join(home, '.orca') + const lockPath = join(lockParent, 'managed-hook-install.lock') + await mkdir(lockParent, { recursive: true }) + const hostIdentity = suppliedHostIdentity ?? (await readManagedHookHostIdentity()) + await cleanupManagedHookLockFiles(lockParent, hostIdentity) + const processIdentity = await readManagedHookProcessIdentity(process.pid) + if (typeof processIdentity !== 'string') { + throw new Error('Could not identify the managed-hook installer process') + } + const deadline = Date.now() + LOCK_WAIT_TIMEOUT_MS + + while (true) { + signal?.throwIfAborted() + const owner = await tryCreateManagedHookLock( + lockParent, + lockPath, + hostIdentity, + processIdentity + ) + if (owner) { + return async () => + await releaseInstallLock(lockPath, lockParent, owner, hostIdentity, processIdentity) + } + + const state = await inspectManagedHookLock(lockPath) + if (state.kind === 'unknown') { + // Why: unverifiable legacy locks cannot be stolen safely; hooks are best-effort, + // so fail fast instead of adding a recurring connection timeout. + throw new Error('Managed-hook install lock has an unverifiable owner') + } + if (state.kind === 'owned' && state.owner.hostIdentity !== hostIdentity) { + // Why: homes can be shared across SSH hosts, whose PID namespaces are unrelated. + throw new Error('Managed-hook install lock belongs to another host') + } + + if (state.kind === 'owned') { + const currentIdentity = await readManagedHookProcessIdentity(state.owner.pid) + if (currentIdentity === undefined) { + throw new Error('Could not verify the managed-hook lock owner process') + } + const abandonedOwnLock = + state.owner.pid === process.pid && + currentIdentity === processIdentity && + !isManagedHookLockOwnerActive(state.owner.token) + if ( + currentIdentity === null || + currentIdentity !== state.owner.processIdentity || + abandonedOwnLock + ) { + const removal = await removeManagedHookLock( + lockPath, + lockParent, + state.owner, + hostIdentity, + processIdentity + ) + if (removal === 'removed') { + if (Date.now() >= deadline) { + throw new Error('Timed out waiting for another managed-hook install to finish') + } + continue + } + if (removal === 'foreign') { + throw new Error('Managed-hook install lock recovery belongs to another host') + } + if (removal === 'unverifiable') { + throw new Error('Managed-hook install lock has an unverifiable recovery claim') + } + } + } + if (Date.now() >= deadline) { + throw new Error('Timed out waiting for another managed-hook install to finish') + } + await delay(LOCK_RETRY_MS, undefined, { signal }) + } +} + +/** Serialize config merges across relay daemons that target the same account. */ +export async function withManagedHookInstallLock( + home: string, + signal: AbortSignal | undefined, + run: () => Promise, + hostIdentity?: string +): Promise { + const release = await acquireInstallLock(home, signal, hostIdentity) + try { + signal?.throwIfAborted() + return await run() + } finally { + await release() + } +} diff --git a/src/main/agent-hooks/managed-hook-local-filesystem.test.ts b/src/main/agent-hooks/managed-hook-local-filesystem.test.ts new file mode 100644 index 00000000000..91a1ca62eb6 --- /dev/null +++ b/src/main/agent-hooks/managed-hook-local-filesystem.test.ts @@ -0,0 +1,71 @@ +import { mkdtemp, mkdir, readFile, readdir, rm, stat, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { installRemoteManagedAgentHooks } from './remote-managed-hook-installers' +import { createManagedHookLocalFilesystem } from './managed-hook-local-filesystem' + +const tempHomes: string[] = [] + +async function createTempHome(): Promise { + const home = await mkdtemp(join(tmpdir(), 'orca-managed-hooks-')) + tempHomes.push(home) + return home +} + +async function listFiles(root: string): Promise { + const entries = await readdir(root, { withFileTypes: true }) + const nested = await Promise.all( + entries.map(async (entry) => { + const path = join(root, entry.name) + return entry.isDirectory() ? await listFiles(path) : [path] + }) + ) + return nested.flat() +} + +afterEach(async () => { + await Promise.all(tempHomes.splice(0).map((home) => rm(home, { recursive: true, force: true }))) +}) + +describe('managed-hook local filesystem', () => { + it('supports cold and warm aggregate installs without SFTP or temp-file residue', async () => { + const home = await createTempHome() + const filesystem = createManagedHookLocalFilesystem() + const options = { grokHomeDir: join(home, '.grok') } + + const cold = await installRemoteManagedAgentHooks(filesystem, home, options) + const warm = await installRemoteManagedAgentHooks(filesystem, home, options) + + expect(cold).toHaveLength(14) + expect(cold.filter((result) => result.state === 'error')).toEqual([]) + expect(warm).toHaveLength(14) + expect(warm.filter((result) => result.state === 'error')).toEqual([]) + const files = await listFiles(home) + expect(files.filter((path) => path.endsWith('.tmp'))).toEqual([]) + const scripts = files.filter((path) => path.includes(join('.orca', 'agent-hooks'))) + expect(scripts.length).toBeGreaterThanOrEqual(10) + if (process.platform !== 'win32') { + for (const script of scripts) { + expect((await stat(script)).mode & 0o777).toBe(0o755) + } + } + }) + + it('isolates a malformed config while installing the remaining agents', async () => { + const home = await createTempHome() + const claudeConfig = join(home, '.claude', 'settings.json') + await mkdir(join(home, '.claude'), { recursive: true }) + await writeFile(claudeConfig, '{"hooks": }', 'utf8') + + const results = await installRemoteManagedAgentHooks(createManagedHookLocalFilesystem(), home, { + grokHomeDir: join(home, '.grok') + }) + + expect(results).toHaveLength(14) + expect(results.find((result) => result.agent === 'claude')?.state).toBe('error') + expect(results.find((result) => result.agent === 'openclaude')?.state).toBe('installed') + expect(results.find((result) => result.agent === 'kimi')?.state).toBe('installed') + expect(await readFile(claudeConfig, 'utf8')).toBe('{"hooks": }') + }) +}) diff --git a/src/main/agent-hooks/managed-hook-local-filesystem.ts b/src/main/agent-hooks/managed-hook-local-filesystem.ts new file mode 100644 index 00000000000..1a104a3a3b1 --- /dev/null +++ b/src/main/agent-hooks/managed-hook-local-filesystem.ts @@ -0,0 +1,61 @@ +import { chmod, mkdir, readFile, readdir, rename, stat, unlink, writeFile } from 'node:fs' +import type { SFTPWrapper } from 'ssh2' + +type Callback = (error: Error | null, value?: T) => void + +function asSftpError(error: NodeJS.ErrnoException): Error & { code: number } { + const translated = new Error(error.message, { cause: error }) as Error & { code: number } + translated.code = + error.code === 'ENOENT' ? 2 : error.code === 'ENOSYS' || error.code === 'ENOTSUP' ? 8 : 4 + return translated +} + +function finish(callback: Callback, error: NodeJS.ErrnoException | null, value?: T): void { + if (error) { + callback(asSftpError(error)) + return + } + callback(null, value) +} + +/** The managed installers only need this small callback-style SFTP surface. + * On the remote host it turns hundreds of WAN round trips into local fs calls. */ +export function createManagedHookLocalFilesystem(): SFTPWrapper { + const adapter = { + readFile(path: string, _encoding: string, callback: Callback): void { + readFile(path, 'utf8', (error, contents) => finish(callback, error, contents)) + }, + writeFile( + path: string, + contents: string, + options: { encoding?: BufferEncoding; mode?: number }, + callback: Callback + ): void { + writeFile(path, contents, options, (error) => finish(callback, error)) + }, + stat(path: string, callback: Callback<{ mode: number }>): void { + stat(path, (error, stats) => finish(callback, error, stats)) + }, + readdir(path: string, callback: Callback<[]>): void { + // Why: installers use readdir only as an existence check; names and + // attrs would allocate work that no caller consumes. + readdir(path, (error) => finish(callback, error, [])) + }, + mkdir(path: string, callback: Callback): void { + mkdir(path, (error) => finish(callback, error)) + }, + rename(source: string, destination: string, callback: Callback): void { + rename(source, destination, (error) => finish(callback, error)) + }, + ext_openssh_rename(source: string, destination: string, callback: Callback): void { + rename(source, destination, (error) => finish(callback, error)) + }, + unlink(path: string, callback: Callback): void { + unlink(path, (error) => finish(callback, error)) + }, + chmod(path: string, mode: number, callback: Callback): void { + chmod(path, mode, (error) => finish(callback, error)) + } + } + return adapter as unknown as SFTPWrapper +} diff --git a/src/main/agent-hooks/managed-hook-lock-claims.ts b/src/main/agent-hooks/managed-hook-lock-claims.ts new file mode 100644 index 00000000000..d3299c7bfd8 --- /dev/null +++ b/src/main/agent-hooks/managed-hook-lock-claims.ts @@ -0,0 +1,216 @@ +import { randomUUID } from 'node:crypto' +import { readdir, readFile, rename, unlink, writeFile } from 'node:fs/promises' +import { join } from 'node:path' +import { + CLAIMED_OWNER_PATTERN, + claimedOwnerFileName, + claimRecordFileName, + hasCode, + inspectManagedHookLock, + ownerFileName, + parseClaim, + parseOwner, + removeFileIfPresent, + type ManagedHookLockClaim, + type ManagedHookLockOwner +} from './managed-hook-lock-records' +import { readManagedHookProcessIdentity } from './managed-hook-owner-identity' + +export type ManagedHookLockRemoval = 'removed' | 'active' | 'foreign' | 'unverifiable' + +type AcquiredClaim = { + kind: 'claimed' + claim: ManagedHookLockClaim + claimRecordPath: string + claimedOwnerPath: string +} + +type ClaimResult = AcquiredClaim | { kind: 'active' | 'foreign' | 'unverifiable' | 'contended' } + +const activeClaimTokens = new Set() + +async function findClaimedOwner( + lockParent: string, + ownerToken: string +): Promise<{ claimToken: string; path: string } | null | undefined> { + const matches = (await readdir(lockParent)).flatMap((entry) => { + const match = CLAIMED_OWNER_PATTERN.exec(entry) + return match?.[1] === ownerToken && match[2] + ? [{ claimToken: match[2], path: join(lockParent, entry) }] + : [] + }) + return matches.length === 1 ? matches[0] : matches.length === 0 ? null : undefined +} + +async function resolveClaimSource( + lockParent: string, + owner: ManagedHookLockOwner, + hostIdentity: string, + processIdentity: string +): Promise< + | { kind: 'ready'; sourcePath: string; priorClaimRecordPath?: string } + | { kind: 'active' | 'foreign' | 'unverifiable' } +> { + const ownerPath = join(lockParent, ownerFileName(owner.token)) + try { + return parseOwner(await readFile(ownerPath, 'utf8'), owner.token) + ? { kind: 'ready', sourcePath: ownerPath } + : { kind: 'unverifiable' } + } catch (error) { + if (!hasCode(error, 'ENOENT')) { + throw error + } + } + + const claimedOwner = await findClaimedOwner(lockParent, owner.token) + if (claimedOwner === undefined || claimedOwner === null) { + return { kind: 'unverifiable' } + } + const priorClaimRecordPath = join( + lockParent, + claimRecordFileName(owner.token, claimedOwner.claimToken) + ) + let priorClaim: ManagedHookLockClaim | null + try { + priorClaim = parseClaim( + await readFile(priorClaimRecordPath, 'utf8'), + owner.token, + claimedOwner.claimToken + ) + } catch (error) { + if (hasCode(error, 'ENOENT')) { + return { kind: 'unverifiable' } + } + throw error + } + if (!priorClaim) { + return { kind: 'unverifiable' } + } + if (priorClaim.hostIdentity !== hostIdentity) { + return { kind: 'foreign' } + } + const currentIdentity = await readManagedHookProcessIdentity(priorClaim.pid) + const recoverOwnInactiveClaim = + priorClaim.pid === process.pid && + currentIdentity === processIdentity && + !activeClaimTokens.has(priorClaim.claimToken) + if (currentIdentity === undefined) { + return { kind: 'unverifiable' } + } + if (currentIdentity === priorClaim.processIdentity && !recoverOwnInactiveClaim) { + return { kind: 'active' } + } + return { kind: 'ready', sourcePath: claimedOwner.path, priorClaimRecordPath } +} + +async function claimManagedHookLock( + lockParent: string, + owner: ManagedHookLockOwner, + hostIdentity: string, + processIdentity: string +): Promise { + const source = await resolveClaimSource(lockParent, owner, hostIdentity, processIdentity) + if (source.kind !== 'ready') { + return source + } + const claimToken = randomUUID() + const claim = { + ownerToken: owner.token, + claimToken, + pid: process.pid, + hostIdentity, + processIdentity + } + const claimRecordPath = join(lockParent, claimRecordFileName(owner.token, claimToken)) + const claimedOwnerPath = join(lockParent, claimedOwnerFileName(owner.token, claimToken)) + await writeFile(claimRecordPath, JSON.stringify(claim), { + encoding: 'utf8', + flag: 'wx', + mode: 0o600 + }) + try { + try { + // Why: renaming the witness elects one claimant without ever dropping the hard link. + await rename(source.sourcePath, claimedOwnerPath) + } catch (error) { + if (hasCode(error, 'ENOENT')) { + return { kind: 'contended' } + } + throw error + } + activeClaimTokens.add(claimToken) + if (source.priorClaimRecordPath) { + try { + await removeFileIfPresent(source.priorClaimRecordPath) + } catch (error) { + console.warn('[agent-hooks] Failed to clean prior managed-hook claim record', error) + } + } + return { kind: 'claimed', claim, claimRecordPath, claimedOwnerPath } + } finally { + if (!activeClaimTokens.has(claimToken)) { + await removeFileIfPresent(claimRecordPath) + } + } +} + +async function cleanAcquiredClaim(claimed: AcquiredClaim): Promise { + await Promise.all([ + removeFileIfPresent(claimed.claimedOwnerPath), + removeFileIfPresent(claimed.claimRecordPath) + ]) +} + +export async function removeManagedHookLock( + lockPath: string, + lockParent: string, + owner: ManagedHookLockOwner, + hostIdentity: string, + processIdentity: string +): Promise { + const claimed = await claimManagedHookLock(lockParent, owner, hostIdentity, processIdentity) + if (claimed.kind === 'contended') { + return 'active' + } + if (claimed.kind === 'unverifiable') { + const state = await inspectManagedHookLock(lockPath) + if (state.kind === 'missing' || (state.kind === 'owned' && state.owner.token !== owner.token)) { + return 'removed' + } + } + if (claimed.kind !== 'claimed') { + return claimed.kind + } + + let removed = false + try { + const state = await inspectManagedHookLock(lockPath) + if (state.kind === 'unknown') { + return 'unverifiable' + } + if (state.kind === 'missing' || state.owner.token !== owner.token) { + removed = true + return 'removed' + } + try { + await unlink(lockPath) + } catch (error) { + if (!hasCode(error, 'ENOENT')) { + return 'unverifiable' + } + } + removed = true + return 'removed' + } finally { + activeClaimTokens.delete(claimed.claim.claimToken) + if (removed) { + try { + await cleanAcquiredClaim(claimed) + } catch (error) { + // Why: the canonical lock is already gone; residue cleanup must not + // turn a successful release into a failed installation. + console.warn('[agent-hooks] Failed to clean managed-hook recovery claim', error) + } + } + } +} diff --git a/src/main/agent-hooks/managed-hook-lock-records.ts b/src/main/agent-hooks/managed-hook-lock-records.ts new file mode 100644 index 00000000000..555c2a1a521 --- /dev/null +++ b/src/main/agent-hooks/managed-hook-lock-records.ts @@ -0,0 +1,274 @@ +import { randomUUID } from 'node:crypto' +import { link, lstat, readdir, readFile, rename, unlink, writeFile } from 'node:fs/promises' +import { join } from 'node:path' +import { readManagedHookProcessIdentity } from './managed-hook-owner-identity' + +const UUID_PATTERN = '[\\da-f]{8}-[\\da-f]{4}-[1-5][\\da-f]{3}-[89ab][\\da-f]{3}-[\\da-f]{12}' +const OWNER_FILE_PATTERN = new RegExp( + `^managed-hook-install\\.owner-(${UUID_PATTERN})\\.json$`, + 'i' +) +const OWNER_DRAFT_PATTERN = new RegExp( + `^managed-hook-install\\.owner-draft-(${UUID_PATTERN})\\.json$`, + 'i' +) +export const CLAIMED_OWNER_PATTERN = new RegExp( + `^managed-hook-install\\.claimed-(${UUID_PATTERN})-(${UUID_PATTERN})\\.json$`, + 'i' +) +const CLAIM_RECORD_PATTERN = new RegExp( + `^managed-hook-install\\.claim-(${UUID_PATTERN})-(${UUID_PATTERN})\\.json$`, + 'i' +) +const activeOwnerTokens = new Set() + +export type ManagedHookLockOwner = { + token: string + pid: number + hostIdentity: string + processIdentity: string +} + +export type ManagedHookLockClaim = { + ownerToken: string + claimToken: string + pid: number + hostIdentity: string + processIdentity: string +} + +export type ManagedHookLockState = + | { kind: 'missing' } + | { kind: 'owned'; owner: ManagedHookLockOwner } + | { kind: 'unknown' } + +export function hasCode(error: unknown, code: string): boolean { + return error instanceof Error && 'code' in error && error.code === code +} + +export function ownerFileName(token: string): string { + return `managed-hook-install.owner-${token}.json` +} + +function ownerDraftFileName(token: string): string { + return `managed-hook-install.owner-draft-${token}.json` +} + +export function claimedOwnerFileName(ownerToken: string, claimToken: string): string { + return `managed-hook-install.claimed-${ownerToken}-${claimToken}.json` +} + +export function claimRecordFileName(ownerToken: string, claimToken: string): string { + return `managed-hook-install.claim-${ownerToken}-${claimToken}.json` +} + +export function deactivateManagedHookLockOwner(ownerToken: string): void { + activeOwnerTokens.delete(ownerToken) +} + +export function isManagedHookLockOwnerActive(ownerToken: string): boolean { + return activeOwnerTokens.has(ownerToken) +} + +export function parseOwner(value: string, token: string): ManagedHookLockOwner | null { + try { + const owner = JSON.parse(value) as Partial + if ( + owner.token !== token || + !Number.isSafeInteger(owner.pid) || + (owner.pid ?? 0) <= 0 || + typeof owner.hostIdentity !== 'string' || + owner.hostIdentity.length === 0 || + typeof owner.processIdentity !== 'string' || + owner.processIdentity.length === 0 + ) { + return null + } + return owner as ManagedHookLockOwner + } catch { + return null + } +} + +export function parseClaim( + value: string, + ownerToken: string, + claimToken: string +): ManagedHookLockClaim | null { + try { + const claim = JSON.parse(value) as Partial + if ( + claim.ownerToken !== ownerToken || + claim.claimToken !== claimToken || + !Number.isSafeInteger(claim.pid) || + (claim.pid ?? 0) <= 0 || + typeof claim.hostIdentity !== 'string' || + claim.hostIdentity.length === 0 || + typeof claim.processIdentity !== 'string' || + claim.processIdentity.length === 0 + ) { + return null + } + return claim as ManagedHookLockClaim + } catch { + return null + } +} + +export async function removeFileIfPresent(path: string): Promise { + try { + await unlink(path) + return true + } catch (error) { + if (hasCode(error, 'ENOENT')) { + return false + } + throw error + } +} + +export async function inspectManagedHookLock(lockPath: string): Promise { + let rawOwner: string + try { + rawOwner = await readFile(lockPath, 'utf8') + } catch (error) { + if (hasCode(error, 'ENOENT')) { + return { kind: 'missing' } + } + // Why: an older relay may own the canonical path as a directory. + return { kind: 'unknown' } + } + try { + const parsed = JSON.parse(rawOwner) as Partial + const owner = typeof parsed.token === 'string' ? parseOwner(rawOwner, parsed.token) : null + return owner ? { kind: 'owned', owner } : { kind: 'unknown' } + } catch { + return { kind: 'unknown' } + } +} + +export async function tryCreateManagedHookLock( + lockParent: string, + lockPath: string, + hostIdentity: string, + processIdentity: string +): Promise { + const token = randomUUID() + const owner = { token, pid: process.pid, hostIdentity, processIdentity } + const ownerPath = join(lockParent, ownerFileName(token)) + const draftPath = join(lockParent, ownerDraftFileName(token)) + await writeFile(draftPath, JSON.stringify(owner), { encoding: 'utf8', flag: 'wx', mode: 0o600 }) + try { + // Why: the final owner name appears only after its complete record is durable. + await rename(draftPath, ownerPath) + try { + // Why: hard-link creation publishes metadata atomically and never replaces a lock. + await link(ownerPath, lockPath) + // Why: publish process-local activity before another same-process contender + // can mistake the newly linked owner for an abandoned release. + activeOwnerTokens.add(token) + return owner + } catch (error) { + if (hasCode(error, 'EEXIST')) { + return null + } + throw error + } + } finally { + await removeFileIfPresent(draftPath) + try { + if ((await lstat(ownerPath)).nlink === 1) { + await unlink(ownerPath) + } + } catch (error) { + if (!hasCode(error, 'ENOENT')) { + console.warn('[agent-hooks] Failed to clean managed-hook owner file', error) + } + } + } +} + +async function cleanOwnerEntry(path: string, token: string, hostIdentity: string): Promise { + const stats = await lstat(path) + if (stats.nlink !== 1) { + return + } + const owner = parseOwner(await readFile(path, 'utf8'), token) + if (!owner || owner.hostIdentity !== hostIdentity) { + return + } + const currentIdentity = await readManagedHookProcessIdentity(owner.pid) + if ( + currentIdentity === null || + (typeof currentIdentity === 'string' && currentIdentity !== owner.processIdentity) + ) { + await unlink(path) + } +} + +async function cleanLockEntry( + entry: string, + lockParent: string, + hostIdentity: string +): Promise { + const path = join(lockParent, entry) + const ownerToken = OWNER_FILE_PATTERN.exec(entry)?.[1] ?? OWNER_DRAFT_PATTERN.exec(entry)?.[1] + if (ownerToken) { + await cleanOwnerEntry(path, ownerToken, hostIdentity) + return + } + const claimedMatch = CLAIMED_OWNER_PATTERN.exec(entry) + if (claimedMatch?.[1] && claimedMatch[2] && (await lstat(path)).nlink === 1) { + await unlink(path) + await removeFileIfPresent( + join(lockParent, claimRecordFileName(claimedMatch[1], claimedMatch[2])) + ) + return + } + const claimMatch = CLAIM_RECORD_PATTERN.exec(entry) + if (!claimMatch?.[1] || !claimMatch[2]) { + return + } + const claim = parseClaim(await readFile(path, 'utf8'), claimMatch[1], claimMatch[2]) + if (!claim || claim.hostIdentity !== hostIdentity) { + return + } + try { + await lstat(join(lockParent, claimedOwnerFileName(claim.ownerToken, claim.claimToken))) + return + } catch (error) { + if (!hasCode(error, 'ENOENT')) { + throw error + } + } + const currentIdentity = await readManagedHookProcessIdentity(claim.pid) + if ( + currentIdentity === null || + (typeof currentIdentity === 'string' && currentIdentity !== claim.processIdentity) + ) { + await unlink(path) + } +} + +export async function cleanupManagedHookLockFiles( + lockParent: string, + hostIdentity: string +): Promise { + let entries: string[] + try { + entries = await readdir(lockParent) + } catch { + return + } + await Promise.all( + entries.map(async (entry) => { + try { + await cleanLockEntry(entry, lockParent, hostIdentity) + } catch (error) { + if (!hasCode(error, 'ENOENT')) { + console.warn('[agent-hooks] Failed to clean managed-hook lock file', error) + } + } + }) + ) +} diff --git a/src/main/agent-hooks/managed-hook-owner-identity.test.ts b/src/main/agent-hooks/managed-hook-owner-identity.test.ts new file mode 100644 index 00000000000..ae8ac15dd12 --- /dev/null +++ b/src/main/agent-hooks/managed-hook-owner-identity.test.ts @@ -0,0 +1,181 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' + +const originalPlatform = process.platform +const originalGetuidDescriptor = Object.getOwnPropertyDescriptor(process, 'getuid') + +type LinuxIdentityFixture = { + hostToken?: string + bootId?: string + pidNamespace?: string + statErrorCode?: string +} + +async function loadLinuxIdentity(fixture: LinuxIdentityFixture) { + Object.defineProperty(process, 'platform', { configurable: true, value: 'linux' }) + Object.defineProperty(process, 'getuid', { configurable: true, value: () => 1000 }) + const statFields = ['S', ...Array.from({ length: 18 }, () => '0'), '4242'] + const readFile = vi.fn(async (path: string | URL) => { + if (String(path).endsWith('host-id') && fixture.hostToken) { + return fixture.hostToken + } + switch (String(path)) { + case '/proc/sys/kernel/random/boot_id': + if (fixture.bootId) { + return `${fixture.bootId}\n` + } + break + case `/proc/${process.pid}/stat`: + case '/proc/123/stat': + if (!fixture.statErrorCode) { + return `123 (node relay) ${statFields.join(' ')}` + } + break + default: + throw new Error(`unexpected path: ${String(path)}`) + } + throw Object.assign(new Error(`unavailable path: ${String(path)}`), { + code: fixture.statErrorCode ?? 'ENOENT' + }) + }) + const readlink = vi.fn(async (path: string | URL) => { + if (fixture.pidNamespace) { + return fixture.pidNamespace + } + throw Object.assign(new Error(`unavailable path: ${String(path)}`), { code: 'EACCES' }) + }) + const mkdir = vi.fn(async () => { + if (!fixture.hostToken) { + throw Object.assign(new Error('host-local storage unavailable'), { code: 'EACCES' }) + } + }) + const lstat = vi.fn(async (path: string | URL) => { + const isToken = String(path).endsWith('host-id') + return { + isDirectory: () => !isToken, + isFile: () => isToken, + mode: isToken ? 0o100600 : 0o040700, + uid: process.getuid?.() ?? 0 + } + }) + vi.doMock('node:fs/promises', async (importOriginal) => ({ + ...(await importOriginal>()), + lstat, + mkdir, + readFile, + readlink + })) + return { identity: await import('./managed-hook-owner-identity'), readFile } +} + +afterEach(() => { + Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) + if (originalGetuidDescriptor) { + Object.defineProperty(process, 'getuid', originalGetuidDescriptor) + } else { + Reflect.deleteProperty(process, 'getuid') + } + vi.unstubAllEnvs() + vi.doUnmock('node:fs/promises') + vi.resetModules() +}) + +describe('managed hook owner identity', () => { + it('uses durable host-local identity without requiring Linux machine identity files', async () => { + vi.stubEnv('SSH_CONNECTION', '198.51.100.8 53100 10.0.0.7 2222') + const { identity, readFile } = await loadLinuxIdentity({ + hostToken: '00000000-0000-4000-8000-000000000001', + bootId: 'current-boot-id', + pidNamespace: 'pid:[4026533001]' + }) + + await expect(identity.readManagedHookHostIdentity()).resolves.toBe( + 'host-token:00000000-0000-4000-8000-000000000001' + ) + expect(readFile).not.toHaveBeenCalledWith('/etc/machine-id', 'utf8') + }) + + it('separates SSH backends that share a key, endpoint, and boot metadata', async () => { + vi.stubEnv('SSH_CONNECTION', '198.51.100.8 53100 10.0.0.7 2222') + const first = await loadLinuxIdentity({ + hostToken: '00000000-0000-4000-8000-000000000001', + bootId: 'shared-kernel-boot-id', + pidNamespace: 'pid:[4026533001]' + }) + const firstHost = await first.identity.readManagedHookHostIdentity() + const firstProcess = await first.identity.readManagedHookProcessIdentity(123) + const fingerprint = 'SHA256:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA' + const firstScopedHost = first.identity.scopeManagedHookHostIdentity(firstHost, fingerprint) + + vi.doUnmock('node:fs/promises') + vi.resetModules() + const second = await loadLinuxIdentity({ + hostToken: '00000000-0000-4000-8000-000000000002', + bootId: 'shared-kernel-boot-id', + pidNamespace: 'pid:[4026533002]' + }) + + const secondHost = await second.identity.readManagedHookHostIdentity() + expect(secondHost).not.toBe(firstHost) + expect(second.identity.scopeManagedHookHostIdentity(secondHost, fingerprint)).not.toBe( + firstScopedHost + ) + await expect(second.identity.readManagedHookProcessIdentity(123)).resolves.not.toBe( + firstProcess + ) + }) + + it('keeps one host scope stable across reboot while changing its process incarnation', async () => { + const fixture = { + hostToken: '00000000-0000-4000-8000-000000000001', + bootId: 'first-boot-id', + pidNamespace: 'pid:[4026533001]' + } + const first = await loadLinuxIdentity(fixture) + const firstHost = await first.identity.readManagedHookHostIdentity() + const firstProcess = await first.identity.readManagedHookProcessIdentity(123) + + vi.doUnmock('node:fs/promises') + vi.resetModules() + const second = await loadLinuxIdentity({ + ...fixture, + bootId: 'second-boot-id', + pidNamespace: 'pid:[4026534001]' + }) + + await expect(second.identity.readManagedHookHostIdentity()).resolves.toBe(firstHost) + await expect(second.identity.readManagedHookProcessIdentity(123)).resolves.not.toBe( + firstProcess + ) + }) + + it('falls back safely when machine, boot, and namespace probes are unavailable', async () => { + vi.stubEnv('SSH_CONNECTION', '') + const { identity } = await loadLinuxIdentity({ statErrorCode: 'EACCES' }) + + const hostIdentity = await identity.readManagedHookHostIdentity() + const processIdentity = await identity.readManagedHookProcessIdentity(process.pid) + expect(hostIdentity).toMatch(/^runtime:/) + expect(processIdentity).toMatch(/^runtime:/) + await expect(identity.readManagedHookHostIdentity()).resolves.toBe(hostIdentity) + await expect(identity.readManagedHookProcessIdentity(process.pid)).resolves.toBe( + processIdentity + ) + await expect(identity.readManagedHookProcessIdentity(123)).resolves.toBeUndefined() + }) + + it('includes namespace, boot, and start ticks when Linux exposes them', async () => { + vi.stubEnv('SSH_CONNECTION', '') + const { identity } = await loadLinuxIdentity({ + hostToken: '00000000-0000-4000-8000-000000000001', + bootId: 'current-boot-id', + pidNamespace: 'pid:[4026533001]' + }) + + await expect(identity.readManagedHookHostIdentity()).resolves.toBe( + 'host-token:00000000-0000-4000-8000-000000000001' + ) + await expect(identity.readManagedHookProcessIdentity(123)).resolves.toBe( + 'linux:pid:[4026533001]:current-boot-id:4242' + ) + }) +}) diff --git a/src/main/agent-hooks/managed-hook-owner-identity.ts b/src/main/agent-hooks/managed-hook-owner-identity.ts new file mode 100644 index 00000000000..e9801c3c489 --- /dev/null +++ b/src/main/agent-hooks/managed-hook-owner-identity.ts @@ -0,0 +1,209 @@ +import { execFile } from 'node:child_process' +import { randomUUID } from 'node:crypto' +import { link, lstat, mkdir, readFile, readlink, unlink, writeFile } from 'node:fs/promises' +import { join } from 'node:path' +import { promisify } from 'node:util' + +const execFileAsync = promisify(execFile) +const runtimeHostIdentity = `runtime:${randomUUID()}` +const runtimeProcessIdentity = `runtime:${randomUUID()}` +let hostIdentityPromise: Promise | undefined +let bootIdentityPromise: Promise | undefined +const HOST_TOKEN_PATTERN = /^[\da-f]{8}-[\da-f]{4}-4[\da-f]{3}-[89ab][\da-f]{3}-[\da-f]{12}$/i + +function hasCode(error: unknown, code: string): boolean { + return error instanceof Error && 'code' in error && error.code === code +} + +function parseLinuxStartTicks(statLine: string): string | null { + const commandEnd = statLine.lastIndexOf(') ') + if (commandEnd < 0) { + return null + } + // Field 22 is index 19 after removing pid and the parenthesized command. + const startTicks = statLine + .slice(commandEnd + 2) + .trim() + .split(/\s+/)[19] + return startTicks ?? null +} + +async function readLinuxPidNamespace(pid: number): Promise { + try { + const namespace = await readlink(`/proc/${pid}/ns/pid`) + return namespace.trim() || undefined + } catch { + return undefined + } +} + +async function readPublishedHostToken(path: string, uid: number): Promise { + try { + const stats = await lstat(path) + if (!stats.isFile() || stats.uid !== uid || (stats.mode & 0o077) !== 0) { + return undefined + } + const token = (await readFile(path, 'utf8')).trim() + return HOST_TOKEN_PATTERN.test(token) ? token : undefined + } catch { + return undefined + } +} + +async function readDurableHostToken(): Promise { + const uid = process.getuid?.() + if ((process.platform !== 'linux' && process.platform !== 'darwin') || uid === undefined) { + return undefined + } + const directory = join('/var', 'tmp', `orca-managed-hooks-${uid}`) + const tokenPath = join(directory, 'host-id') + try { + await mkdir(directory, { recursive: true, mode: 0o700 }) + const directoryStats = await lstat(directory) + if ( + !directoryStats.isDirectory() || + directoryStats.uid !== uid || + (directoryStats.mode & 0o077) !== 0 + ) { + return undefined + } + const existing = await readPublishedHostToken(tokenPath, uid) + if (existing) { + return existing + } + const token = randomUUID() + const draftPath = join(directory, `host-id-draft-${token}`) + await writeFile(draftPath, token, { encoding: 'utf8', flag: 'wx', mode: 0o600 }) + try { + // Why: the hard link publishes a complete token atomically across relay processes. + await link(draftPath, tokenPath).catch((error) => { + if (!hasCode(error, 'EEXIST')) { + throw error + } + }) + } finally { + await unlink(draftPath).catch(() => {}) + } + return await readPublishedHostToken(tokenPath, uid) + } catch { + return undefined + } +} + +async function readHostIdentity(): Promise { + const durableToken = await readDurableHostToken() + if (durableToken) { + return `host-token:${durableToken}` + } + if (process.platform === 'linux') { + // Why: an unverifiable host scope may acquire and release its own clean lock, + // but a later process gets a different identity and cannot steal its residue. + return runtimeHostIdentity + } + if (process.platform === 'darwin') { + try { + const { stdout } = await execFileAsync('ioreg', ['-rd1', '-c', 'IOPlatformExpertDevice'], { + encoding: 'utf8', + timeout: 1_000 + }) + const platformId = /"IOPlatformUUID"\s*=\s*"([^"]+)"/.exec(stdout)?.[1] + return platformId ? `darwin:${platformId}` : runtimeHostIdentity + } catch { + return runtimeHostIdentity + } + } + return runtimeHostIdentity +} + +async function readBootIdentity(): Promise { + if (process.platform === 'linux') { + try { + const bootId = (await readFile('/proc/sys/kernel/random/boot_id', 'utf8')).trim() + return bootId || undefined + } catch { + return undefined + } + } + + if (process.platform === 'darwin') { + try { + const { stdout } = await execFileAsync('sysctl', ['-n', 'kern.bootsessionuuid'], { + encoding: 'utf8', + timeout: 1_000 + }) + const bootSessionId = stdout.trim() + return bootSessionId || undefined + } catch { + return undefined + } + } + + return undefined +} + +export async function readManagedHookHostIdentity(): Promise { + hostIdentityPromise ??= readHostIdentity() + return await hostIdentityPromise +} + +/** Bind a stable SSH identity to one execution runtime before comparing its PIDs. */ +export function scopeManagedHookHostIdentity( + executionIdentity: string, + hostKeyFingerprint?: string +): string { + return hostKeyFingerprint + ? `ssh-host-key:${hostKeyFingerprint}:execution:${executionIdentity}` + : executionIdentity +} + +/** null means confirmed missing; undefined means the platform probe was unavailable. */ +export async function readManagedHookProcessIdentity( + pid: number +): Promise { + if (process.platform === 'linux') { + try { + const [statLine, pidNamespace, bootIdentity] = await Promise.all([ + readFile(`/proc/${pid}/stat`, 'utf8'), + readLinuxPidNamespace(pid), + (bootIdentityPromise ??= readBootIdentity()) + ]) + const startTicks = parseLinuxStartTicks(statLine) + return startTicks + ? `linux:${pidNamespace ?? 'unknown-namespace'}:${bootIdentity ?? 'unknown-boot'}:${startTicks}` + : undefined + } catch (error) { + if (hasCode(error, 'ENOENT')) { + return null + } + return pid === process.pid ? runtimeProcessIdentity : undefined + } + } + + bootIdentityPromise ??= readBootIdentity() + const bootIdentity = await bootIdentityPromise + try { + const { stdout } = await execFileAsync( + 'ps', + ['-o', 'lstart=', '-o', 'command=', '-p', String(pid)], + { + encoding: 'utf8', + timeout: 1_000, + // Why: lstart is locale-sensitive; a fixed locale keeps the identity + // stable across relay processes for the same remote account. + env: { ...process.env, LC_ALL: 'C', LANG: 'C' } + } + ) + const startedAt = stdout.trim() + return startedAt ? `${process.platform}:${bootIdentity ?? 'unknown-boot'}:${startedAt}` : null + } catch { + try { + process.kill(pid, 0) + return undefined + } catch (error) { + if (hasCode(error, 'ESRCH')) { + return null + } + return pid === process.pid ? runtimeProcessIdentity : undefined + } + } +} diff --git a/src/main/agent-hooks/managed-hook-runtime.test.ts b/src/main/agent-hooks/managed-hook-runtime.test.ts new file mode 100644 index 00000000000..456a741df87 --- /dev/null +++ b/src/main/agent-hooks/managed-hook-runtime.test.ts @@ -0,0 +1,22 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { resolveRelayGrokHome } from './managed-hook-runtime' + +afterEach(() => { + vi.unstubAllEnvs() +}) + +describe.runIf(process.platform !== 'win32')('resolveRelayGrokHome', () => { + it('uses the login-shell GROK_HOME and normalizes trailing separators', async () => { + vi.stubEnv('SHELL', '/bin/sh') + vi.stubEnv('GROK_HOME', '/srv/grok///') + + await expect(resolveRelayGrokHome('/home/orca')).resolves.toBe('/srv/grok') + }) + + it('falls back when the login-shell GROK_HOME is not an absolute POSIX path', async () => { + vi.stubEnv('SHELL', '/bin/sh') + vi.stubEnv('GROK_HOME', '../relative') + + await expect(resolveRelayGrokHome('/home/orca')).resolves.toBe('/home/orca/.grok') + }) +}) diff --git a/src/main/agent-hooks/managed-hook-runtime.ts b/src/main/agent-hooks/managed-hook-runtime.ts new file mode 100644 index 00000000000..b79c3b2186d --- /dev/null +++ b/src/main/agent-hooks/managed-hook-runtime.ts @@ -0,0 +1,106 @@ +import { execFile } from 'node:child_process' +import { basename } from 'node:path' +import { homedir, userInfo } from 'node:os' +import { promisify } from 'node:util' +import { installRemoteManagedAgentHooks } from './remote-managed-hook-installers' +import { createManagedHookLocalFilesystem } from './managed-hook-local-filesystem' +import { withManagedHookInstallLock } from './managed-hook-install-lock' +import { + readManagedHookHostIdentity, + scopeManagedHookHostIdentity +} from './managed-hook-owner-identity' + +const execFileAsync = promisify(execFile) +const GROK_HOME_MAX_LENGTH = 4096 +const GROK_HOME_PROBE_TIMEOUT_MS = 8_000 + +export type ManagedHookInstallSummary = { + installers: number + errors: number +} + +function defaultGrokHome(home: string): string { + return `${home.replace(/\/+$/, '') || home}/.grok` +} + +function hasControlCharacter(value: string): boolean { + return Array.from(value).some((character) => { + const code = character.charCodeAt(0) + return code <= 0x1f || code === 0x7f + }) +} + +function normalizeGrokHome(candidate: string): string | null { + if ( + candidate.length === 0 || + candidate.length > GROK_HOME_MAX_LENGTH || + candidate !== candidate.trim() || + !candidate.startsWith('/') || + candidate.includes('\\') || + hasControlCharacter(candidate) + ) { + return null + } + return candidate.replace(/\/+$/, '') || '/' +} + +function resolveLoginShell(): string { + const candidate = process.env.SHELL || userInfo().shell || '/bin/sh' + if (!candidate.startsWith('/') || candidate.includes('\\') || hasControlCharacter(candidate)) { + return '/bin/sh' + } + return candidate +} + +export async function resolveRelayGrokHome(home: string, signal?: AbortSignal): Promise { + const fallback = defaultGrokHome(home) + try { + const shell = resolveLoginShell() + const shellName = basename(shell) + const mode = shellName === 'sh' || shellName === 'dash' ? '-c' : '-lc' + // Why: agent PTYs start login shells, so read the same profile-derived + // GROK_HOME without opening two additional SSH exec channels. + const { stdout } = await execFileAsync( + shell, + [mode, `printenv GROK_HOME | head -c ${GROK_HOME_MAX_LENGTH + 1}`], + { encoding: 'utf8', timeout: GROK_HOME_PROBE_TIMEOUT_MS, signal } + ) + return normalizeGrokHome(stdout.split(/\r?\n/, 1)[0] ?? '') ?? fallback + } catch { + signal?.throwIfAborted() + return fallback + } +} + +export async function installManagedHooks(options?: { + signal?: AbortSignal + hostKeyFingerprint?: string +}): Promise { + options?.signal?.throwIfAborted() + const home = homedir() + const grokHomeDir = await resolveRelayGrokHome(home, options?.signal) + options?.signal?.throwIfAborted() + const hostIdentity = scopeManagedHookHostIdentity( + await readManagedHookHostIdentity(), + options?.hostKeyFingerprint + ) + return await withManagedHookInstallLock( + home, + options?.signal, + async () => { + const results = await installRemoteManagedAgentHooks( + createManagedHookLocalFilesystem(), + home, + { + grokHomeDir, + signal: options?.signal + } + ) + return { + installers: results.length, + errors: results.filter((result) => result.state === 'error').length + } + }, + hostIdentity + ) +} diff --git a/src/main/agent-hooks/managed-hook-stdin-lifecycle.test.ts b/src/main/agent-hooks/managed-hook-stdin-lifecycle.test.ts index 39639a36b49..bc11aba30f7 100644 --- a/src/main/agent-hooks/managed-hook-stdin-lifecycle.test.ts +++ b/src/main/agent-hooks/managed-hook-stdin-lifecycle.test.ts @@ -2,12 +2,31 @@ // matrix catches an unread early exit without duplicating template assertions. import { describe, expect, it, vi } from 'vitest' import { spawn } from 'node:child_process' -import { mkdtempSync, readFileSync, readdirSync, rmSync } from 'node:fs' +import { existsSync, mkdtempSync, readFileSync, readdirSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import type { SFTPWrapper } from 'ssh2' import type * as osModule from 'node:os' +function findGitBash(): string { + if (process.env.KIMI_SHELL_PATH) { + return process.env.KIMI_SHELL_PATH + } + const candidates = [ + process.env.ProgramFiles && join(process.env.ProgramFiles, 'Git', 'bin', 'bash.exe'), + process.env['ProgramFiles(x86)'] && + join(process.env['ProgramFiles(x86)'], 'Git', 'bin', 'bash.exe'), + process.env.LOCALAPPDATA && join(process.env.LOCALAPPDATA, 'Programs', 'Git', 'bin', 'bash.exe') + ] + const bash = candidates.find((candidate): candidate is string => + Boolean(candidate && existsSync(candidate)) + ) + if (!bash) { + throw new Error('Git Bash is required for the Windows Kimi hook lifecycle test') + } + return bash +} + const { homedirMock } = vi.hoisted(() => ({ homedirMock: vi.fn<() => string>() })) @@ -255,6 +274,7 @@ describe('Windows managed hook stdin structure', () => { const home = mkdtempSync(join(tmpdir(), 'orca-hook-stdin-windows-live-')) homedirMock.mockReturnValue(home) try { + const gitBash = findGitBash() for (const entry of LOCAL_INSTALLERS) { expect(entry.install().state, `${entry.agent} install status`).toBe('installed') } @@ -279,7 +299,7 @@ describe('Windows managed hook stdin structure', () => { 'v1.0', 'powershell.exe' ) - : process.env.KIMI_SHELL_PATH || 'bash.exe' + : gitBash const args = fileName.endsWith('.cmd') ? ['/d', '/c', scriptPath] : fileName.endsWith('.ps1') @@ -306,7 +326,7 @@ describe('Windows managed hook stdin structure', () => { }, { name: 'Git Bash fast path', - executable: process.env.KIMI_SHELL_PATH || 'bash.exe', + executable: gitBash, args: ['-lc', wrapWindowsGitBashHookCommand(missingScript)] } ] diff --git a/src/main/agent-hooks/managed-hook-timeout.test.ts b/src/main/agent-hooks/managed-hook-timeout.test.ts index a215210a577..c7d374573ff 100644 --- a/src/main/agent-hooks/managed-hook-timeout.test.ts +++ b/src/main/agent-hooks/managed-hook-timeout.test.ts @@ -118,10 +118,16 @@ const MANAGED_HOOKS_DIR_NEEDLE = '/.orca/agent-hooks/' // dir) has a positive config-level timeout sibling (`timeout` or the // provider-specific `timeoutSec`). Returns the count of managed carriers found // so callers can assert the scan was not vacuous. -function countManagedCarriersWithTimeout(node: unknown, expectedTimeout: number): number { +function countManagedCarriersWithTimeout( + node: unknown, + expectedTimeout: number, + isManagedCarrier = (value: string): boolean => + value.replaceAll('\\', '/').includes(MANAGED_HOOKS_DIR_NEEDLE) +): number { if (Array.isArray(node)) { return node.reduce( - (sum, child) => sum + countManagedCarriersWithTimeout(child, expectedTimeout), + (sum, child) => + sum + countManagedCarriersWithTimeout(child, expectedTimeout, isManagedCarrier), 0 ) } @@ -131,8 +137,7 @@ function countManagedCarriersWithTimeout(node: unknown, expectedTimeout: number) const record = node as Record let found = 0 const carrier = [record.command, record.bash, record.powershell].find( - (value): value is string => - typeof value === 'string' && value.includes(MANAGED_HOOKS_DIR_NEEDLE) + (value): value is string => typeof value === 'string' && isManagedCarrier(value) ) if (carrier !== undefined) { const timeout = typeof record.timeout === 'number' ? record.timeout : record.timeoutSec @@ -143,7 +148,7 @@ function countManagedCarriersWithTimeout(node: unknown, expectedTimeout: number) found += 1 } for (const value of Object.values(record)) { - found += countManagedCarriersWithTimeout(value, expectedTimeout) + found += countManagedCarriersWithTimeout(value, expectedTimeout, isManagedCarrier) } return found } @@ -182,7 +187,13 @@ describe('managed agent hook timeouts', () => { const status = new DroidHookService().install() expect(status.state).toBe('installed') const config = JSON.parse(readFileSync(join(homeDir, '.factory', 'settings.json'), 'utf8')) - const carriers = countManagedCarriersWithTimeout(config, MANAGED_HOOK_TIMEOUT_SECONDS) + const carriers = countManagedCarriersWithTimeout( + config, + MANAGED_HOOK_TIMEOUT_SECONDS, + (command) => + command.replaceAll('\\', '/').includes(MANAGED_HOOKS_DIR_NEEDLE) || + (process.platform === 'win32' && command.includes('-EncodedCommand')) + ) expect(carriers).toBeGreaterThan(0) } finally { homedirMock.mockImplementation(() => process.env.HOME ?? tmpdir()) diff --git a/src/main/agent-hooks/remote-hook-service-installers.test.ts b/src/main/agent-hooks/remote-hook-service-installers.test.ts index 8489048d0f5..3acca6a64ef 100644 --- a/src/main/agent-hooks/remote-hook-service-installers.test.ts +++ b/src/main/agent-hooks/remote-hook-service-installers.test.ts @@ -726,6 +726,35 @@ describe('remote hook service installers', () => { expect(byAgent.get('copilot')).toBe('installed') }) + it('stops before the next installer when its relay request is cancelled', async () => { + const controller = new AbortController() + const claudeInstall = vi + .spyOn(claudeHookService, 'installRemote') + .mockImplementation(async () => { + controller.abort() + return { + agent: 'claude', + state: 'installed', + configPath: '/home/dev/.claude/settings.json', + managedHooksPresent: true, + detail: null + } + }) + const openClaudeInstall = vi.spyOn(openClaudeHookService, 'installRemote') + try { + const { sftp } = createFakeSftp() + + await expect( + installRemoteManagedAgentHooks(sftp, '/home/dev', { signal: controller.signal }) + ).rejects.toMatchObject({ name: 'AbortError' }) + expect(claudeInstall).toHaveBeenCalledTimes(1) + expect(openClaudeInstall).not.toHaveBeenCalled() + } finally { + claudeInstall.mockRestore() + openClaudeInstall.mockRestore() + } + }) + it('installs remote Droid hooks into Factory settings.json (issue #7253)', async () => { const { sftp, fs } = createFakeSftp() diff --git a/src/main/agent-hooks/remote-managed-hook-installers.ts b/src/main/agent-hooks/remote-managed-hook-installers.ts index 39507f091f2..7f9e97f2f9f 100644 --- a/src/main/agent-hooks/remote-managed-hook-installers.ts +++ b/src/main/agent-hooks/remote-managed-hook-installers.ts @@ -23,6 +23,9 @@ export type RemoteManagedHookInstallOptions = { codexHomeDir?: string /** Explicit GROK_HOME for remote runtimes that redirect Grok's config. */ grokHomeDir?: string + /** Stops before starting the next installer when the owning relay request + * is cancelled. Individual filesystem mutations remain atomic. */ + signal?: AbortSignal } type RemoteManagedHookInstaller = readonly [ @@ -78,6 +81,9 @@ export async function installRemoteManagedAgentHooks( ): Promise { const results: AgentHookInstallStatus[] = [] for (const [agent, install] of REMOTE_MANAGED_HOOK_INSTALLERS) { + // Why: relay requests can disappear during reconnect; do not start more + // user-config mutations after their client has gone away. + options?.signal?.throwIfAborted() try { const result = await install(sftp, remoteHome, options) results.push(result) diff --git a/src/main/ipc/ssh.test.ts b/src/main/ipc/ssh.test.ts index cbca2db9d2a..49b6bcba83a 100644 --- a/src/main/ipc/ssh.test.ts +++ b/src/main/ipc/ssh.test.ts @@ -1225,6 +1225,49 @@ describe('SSH IPC handlers', () => { expect(mockConnectionManager.disconnect).toHaveBeenCalledWith('ssh-1') }) + it('invalidates a pending connect when disconnect wins and allows a fresh connect', async () => { + const target: SshTarget = { + id: 'ssh-1', + label: 'Server', + host: 'example.com', + port: 22, + username: 'deploy' + } + const staleConn = {} + const freshConn = {} + let resolveStaleConnect!: (connection: unknown) => void + mockSshStore.getTarget.mockReturnValue(target) + mockConnectionManager.connect.mockReturnValueOnce( + new Promise((resolve) => { + resolveStaleConnect = resolve + }) + ) + mockConnectionManager.getState.mockReturnValue({ + targetId: 'ssh-1', + status: 'connected', + error: null, + reconnectAttempt: 0 + }) + + const staleConnect = handlers.get('ssh:connect')!(null, { + targetId: 'ssh-1' + }) as Promise + await vi.waitFor(() => expect(mockConnectionManager.connect).toHaveBeenCalledTimes(1)) + + await handlers.get('ssh:disconnect')!(null, { targetId: 'ssh-1' }) + mockConnectionManager.connect.mockResolvedValueOnce(freshConn) + const freshConnect = handlers.get('ssh:connect')!(null, { + targetId: 'ssh-1' + }) as Promise + await vi.waitFor(() => expect(mockConnectionManager.connect).toHaveBeenCalledTimes(2)) + + resolveStaleConnect(staleConn) + + await expect(staleConnect).rejects.toThrow('SSH connection attempt was cancelled') + await expect(freshConnect).resolves.toMatchObject({ targetId: 'ssh-1', status: 'connected' }) + expect(mockDeployAndLaunchRelay).toHaveBeenCalledTimes(1) + }) + it('ssh:terminateSessions preserves tracking when relay shutdown fails', async () => { const target: SshTarget = { id: 'ssh-1', diff --git a/src/main/ipc/ssh.ts b/src/main/ipc/ssh.ts index d1c3e51c3fd..f8f55ecaae8 100644 --- a/src/main/ipc/ssh.ts +++ b/src/main/ipc/ssh.ts @@ -99,6 +99,7 @@ export function listRegisteredRemovedSshTargetLabels(): Record { } export async function disconnectRegisteredSshTarget(targetId: string): Promise { + invalidateConnectAttempt(targetId) if (!connectionManager) { return } @@ -110,7 +111,8 @@ export async function removeRegisteredSshTarget(targetId: string): Promise if (!sshStore) { return } - // Why: removal is destructive — dispose() (not detach()) so the relay shuts down and remote PTY leases are terminated, not preserved for a reattach. + invalidateConnectAttempt(targetId) + // Why: removal is destructive; dispose so remote PTYs cannot reattach to a deleted target. await disposeActiveSshSession(targetId) try { await connectionManager?.disconnect(targetId) @@ -172,8 +174,33 @@ function relayGracePeriodForTarget(target: SshTarget | null | undefined): number return target?.relayGracePeriodSeconds } -// Why: concurrent ssh:connect from multiple tabs would each create a session (leaking the first); hold the in-flight promise so the second awaits it. -const connectInFlight = new Map>() +// Why: tabs must share one connect, while a disconnect must invalidate that +// attempt so its late continuation cannot clobber a replacement. +type ConnectAttempt = { + generation: number + promise: Promise +} + +const connectInFlight = new Map() +const connectGenerationByTarget = new Map() + +function currentConnectGeneration(targetId: string): number { + return connectGenerationByTarget.get(targetId) ?? 0 +} + +function invalidateConnectAttempt(targetId: string): void { + connectGenerationByTarget.set(targetId, currentConnectGeneration(targetId) + 1) + connectInFlight.delete(targetId) + credentialRequestedForTarget.delete(targetId) +} + +function isCurrentConnectAttempt(targetId: string, generation: number): boolean { + return currentConnectGeneration(targetId) === generation +} + +function connectCancelledError(): Error { + return new Error('SSH connection attempt was cancelled') +} // Why: publish reset's teardown/force-stop/disconnect lifecycle so new connects and duplicate resets can't race it. const resetRelayInFlight = new Map>() @@ -732,6 +759,7 @@ export function registerSshHandlers( // ── Connection lifecycle ─────────────────────────────────────────── async function connectTarget(targetId: string): Promise { + const observedGeneration = currentConnectGeneration(targetId) const reset = resetRelayInFlight.get(targetId) if (reset) { await reset @@ -740,15 +768,23 @@ export function registerSshHandlers( // Why: serialize concurrent ssh:connect for the same target; interleaved connects otherwise leak the first session. const existing = connectInFlight.get(targetId) if (existing) { - return existing + return existing.promise + } + if (currentConnectGeneration(targetId) !== observedGeneration) { + throw connectCancelledError() } - const promise = doConnect(targetId) - connectInFlight.set(targetId, promise) + const generation = observedGeneration + 1 + connectGenerationByTarget.set(targetId, generation) + const promise = doConnect(targetId, generation) + const attempt = { generation, promise } + connectInFlight.set(targetId, attempt) try { return await promise } finally { - connectInFlight.delete(targetId) + if (connectInFlight.get(targetId) === attempt) { + connectInFlight.delete(targetId) + } } } @@ -759,7 +795,7 @@ export function registerSshHandlers( return connectTarget(args.targetId) }) - async function doConnect(targetId: string): Promise { + async function doConnect(targetId: string, generation: number): Promise { const target = sshStore!.getTarget(targetId) if (!target) { throw new Error(`SSH target "${targetId}" not found`) @@ -788,6 +824,9 @@ export function registerSshHandlers( if (existingSession) { // Why: await port teardown before disposing, else the new session's restorePortForwards can hit EADDRINUSE on not-yet-released ports. await portForwardManager!.removeAllForwards(targetId) + if (!isCurrentConnectAttempt(targetId, generation)) { + throw connectCancelledError() + } existingSession.detach() activeSessions.delete(targetId) clearRelayLostBackoff(targetId) @@ -805,14 +844,22 @@ export function registerSshHandlers( ) configureRelaySessionCallbacks(session) activeSessions.set(targetId, session) + const ownsSession = (): boolean => + isCurrentConnectAttempt(targetId, generation) && activeSessions.get(targetId) === session try { conn = await connectionManager!.connect(target) + if (!ownsSession()) { + throw connectCancelledError() + } } catch (err) { // Why: connect()'s internal state may not have reached the renderer; broadcast explicitly so the UI leaves 'connecting'. const errObj = err instanceof Error ? err : new Error(String(err)) const status: SshConnectionStatus = isAuthError(errObj) ? 'auth-failed' : 'error' - // Why: clear this failed connect's credential flag so a later non-prompting connect can't persist lastRequiredPassphrase=true. + if (!ownsSession()) { + throw connectCancelledError() + } + // Why: clear this failed connect's flag so a later non-prompting connect isn't deferred. credentialRequestedForTarget.delete(targetId) activeSessions.delete(targetId) clearRelayLostBackoff(targetId) @@ -835,6 +882,9 @@ export function registerSshHandlers( }) await session.establish(conn, relayGracePeriodForTarget(target)) + if (!ownsSession()) { + throw connectCancelledError() + } // Why: we manually pushed `deploying-relay`, so send `connected` straight to the renderer — routing through onStateChange would trigger reconnect logic. clearRelayStateOverride(targetId) @@ -846,6 +896,9 @@ export function registerSshHandlers( supportsFolderDownload: conn.usesSystemSshTransport?.() !== true }) } catch (err) { + if (!ownsSession()) { + throw connectCancelledError() + } activeSessions.delete(targetId) clearRelayLostBackoff(targetId) await connectionManager!.disconnect(targetId) @@ -865,6 +918,7 @@ export function registerSshHandlers( }) ipcMain.handle('ssh:terminateSessions', async (_event, args: { targetId: string }) => { + invalidateConnectAttempt(args.targetId) const session = activeSessions.get(args.targetId) const provider = getSshPtyProvider(args.targetId) const leasedIds = persistedStore! @@ -931,8 +985,8 @@ export function registerSshHandlers( const inFlightConnect = connectInFlight.get(targetId) if (inFlightConnect) { try { - // Why: await the in-flight connect first; tearing down activeSessions mid-deploy would dispose the session doConnect is about to use. - await inFlightConnect + // Why: resetting activeSessions mid-deploy would dispose the session doConnect will use. + await inFlightConnect.promise } catch { // The reset can still recover a stale remote relay after a failed connect. } @@ -1029,7 +1083,7 @@ export function registerSshHandlers( const inFlight = connectInFlight.get(args.targetId) if (inFlight) { try { - const state = await inFlight + const state = await inFlight.promise return { success: true, state } } catch (err) { return { @@ -1177,6 +1231,7 @@ export async function resetSshHandlerStateForTests(): Promise { } relayStateOverrides.clear() connectInFlight.clear() + connectGenerationByTarget.clear() resetRelayInFlight.clear() testingTargets.clear() credentialRequestedForTarget.clear() diff --git a/src/main/ssh/ssh-connection-manager.test.ts b/src/main/ssh/ssh-connection-manager.test.ts new file mode 100644 index 00000000000..2667dc28e1f --- /dev/null +++ b/src/main/ssh/ssh-connection-manager.test.ts @@ -0,0 +1,74 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { SshTarget } from '../../shared/ssh-types' + +const mockState = vi.hoisted(() => ({ + connectResults: [] as Promise[], + instances: [] as { + connect: ReturnType + disconnect: ReturnType + status: 'connecting' | 'connected' | 'disconnected' + }[] +})) + +vi.mock('./ssh-connection', () => ({ + SshConnection: class MockSshConnection { + status: 'connecting' | 'connected' | 'disconnected' = 'connecting' + connect = vi.fn(async () => { + await (mockState.connectResults.shift() ?? Promise.resolve()) + this.status = 'connected' + }) + disconnect = vi.fn(async () => { + this.status = 'disconnected' + }) + + constructor() { + mockState.instances.push(this) + } + + getState(): { status: 'connecting' | 'connected' | 'disconnected' } { + return { status: this.status } + } + + setCallbacks(): void {} + } +})) + +import { SshConnectionManager } from './ssh-connection-manager' + +const target = { + id: 'target-1', + label: 'Target 1', + host: 'example.test', + port: 22, + username: 'demo', + source: 'manual' +} as SshTarget + +describe('SshConnectionManager', () => { + beforeEach(() => { + mockState.connectResults.length = 0 + mockState.instances.length = 0 + }) + + it('lets disconnect start a new connect before the cancelled attempt settles', async () => { + let rejectFirst!: (error: Error) => void + mockState.connectResults.push( + new Promise((_resolve, reject) => { + rejectFirst = reject + }), + Promise.resolve() + ) + const manager = new SshConnectionManager({ + onStateChange: vi.fn() + }) + + const firstConnect = manager.connect(target) + await manager.disconnect(target.id) + const secondConnection = await manager.connect(target) + rejectFirst(new Error('cancelled')) + + await expect(firstConnect).rejects.toThrow('cancelled') + expect(mockState.instances).toHaveLength(2) + expect(manager.getConnection(target.id)).toBe(secondConnection) + }) +}) diff --git a/src/main/ssh/ssh-connection-manager.ts b/src/main/ssh/ssh-connection-manager.ts index d568d439409..8bc17d06b70 100644 --- a/src/main/ssh/ssh-connection-manager.ts +++ b/src/main/ssh/ssh-connection-manager.ts @@ -9,10 +9,9 @@ import { SshConnection, type SshConnectionCallbacks } from './ssh-connection' export class SshConnectionManager { private connections = new Map() private callbacks: SshConnectionCallbacks - // Why: two concurrent connect() calls for the same target would both pass - // the "existing" check, create two SshConnections, and orphan the first. - // This set prevents a second call from racing with an in-progress one. - private connectingTargets = new Set() + // Why: attempt identity lets disconnect unblock a replacement without the + // cancelled attempt later clearing the replacement's state. + private connectingTargets = new Map() constructor(callbacks: SshConnectionCallbacks) { this.callbacks = callbacks @@ -35,7 +34,8 @@ export class SshConnectionManager { throw new Error(`Connection to ${target.label} is already in progress`) } - this.connectingTargets.add(target.id) + const attempt = Symbol(target.id) + this.connectingTargets.set(target.id, attempt) try { if (existing) { @@ -48,23 +48,32 @@ export class SshConnectionManager { try { await conn.connect() } catch (err) { - this.connections.delete(target.id) + if (this.connections.get(target.id) === conn) { + this.connections.delete(target.id) + } throw err } return conn } finally { - this.connectingTargets.delete(target.id) + if (this.connectingTargets.get(target.id) === attempt) { + this.connectingTargets.delete(target.id) + } } } async disconnect(targetId: string): Promise { + // Why: disconnect invalidates the old attempt immediately so a reconnect + // need not wait for the cancelled socket's late completion. + this.connectingTargets.delete(targetId) const conn = this.connections.get(targetId) if (!conn) { return } await conn.disconnect() - this.connections.delete(targetId) + if (this.connections.get(targetId) === conn) { + this.connections.delete(targetId) + } } async reconnect(targetId: string): Promise { diff --git a/src/main/ssh/ssh-connection.test.ts b/src/main/ssh/ssh-connection.test.ts index 37bffc0d691..750932289d8 100644 --- a/src/main/ssh/ssh-connection.test.ts +++ b/src/main/ssh/ssh-connection.test.ts @@ -60,6 +60,9 @@ vi.mock('ssh2', () => { } connect(config?: unknown) { this.lastConnectConfig = config + const hostVerifier = (config as { hostVerifier?: (key: Buffer) => boolean } | undefined) + ?.hostVerifier + hostVerifier?.(Buffer.from('mock-ssh-host-key')) setTimeout(() => { const next = connectSequence.shift() if (next instanceof Error) { @@ -320,6 +323,35 @@ describe('SshConnection', () => { expect(clientInstances[0].setNoDelay).toHaveBeenCalledWith(true) }) + it('captures the negotiated SSH server key fingerprint', async () => { + const conn = new SshConnection(createTarget(), createCallbacks()) + await conn.connect() + + expect(conn.getHostKeyFingerprint()).toMatch(/^SHA256:[A-Za-z\d+/]{43}$/) + }) + + it('ignores a late host fingerprint from an obsolete connect generation', async () => { + const conn = new SshConnection(createTarget(), createCallbacks()) + await conn.connect() + const firstVerifier = ( + clientInstances[0].lastConnectConfig as { hostVerifier?: (key: Buffer) => boolean } + ).hostVerifier + + const privateConn = conn as unknown as { attemptConnect: () => Promise } + await privateConn.attemptConnect() + const secondVerifier = ( + clientInstances[1].lastConnectConfig as { hostVerifier?: (key: Buffer) => boolean } + ).hostVerifier + expect(firstVerifier).toBeTypeOf('function') + expect(secondVerifier).toBeTypeOf('function') + + secondVerifier?.(Buffer.from('newer-ssh-host-key')) + const currentFingerprint = conn.getHostKeyFingerprint() + firstVerifier?.(Buffer.from('obsolete-ssh-host-key')) + + expect(conn.getHostKeyFingerprint()).toBe(currentFingerprint) + }) + it('allows concurrent exec commands for ssh2 transport', async () => { const conn = new SshConnection(createTarget(), createCallbacks()) await conn.connect() @@ -1248,6 +1280,7 @@ describe('SshConnection', () => { expect(conn.getState().status).toBe('connected') expect(conn.usesSystemSshTransport()).toBe(true) + expect(conn.getHostKeyFingerprint()).toBeUndefined() expect(onCredentialRequest).not.toHaveBeenCalled() }) diff --git a/src/main/ssh/ssh-connection.ts b/src/main/ssh/ssh-connection.ts index eb2f79cf13a..baba633d79d 100644 --- a/src/main/ssh/ssh-connection.ts +++ b/src/main/ssh/ssh-connection.ts @@ -1,5 +1,6 @@ /* eslint-disable max-lines -- Why: SSH connection lifecycle, credential retries, reconnect policy, and transport fallback are intentionally co-located so state transitions stay auditable in one file. */ import * as net from 'node:net' +import { createHash } from 'node:crypto' import { Client as SshClient } from 'ssh2' import type { ChildProcess } from 'node:child_process' import type { ClientChannel, ConnectConfig, SFTPWrapper } from 'ssh2' @@ -82,6 +83,7 @@ export class SshConnection { private disposed = false private cachedPassphrase: string | null = null private cachedPassword: string | null = null + private hostKeyFingerprint: string | undefined private connectGeneration = 0 constructor(target: SshTarget, callbacks: SshConnectionCallbacks) { @@ -121,6 +123,11 @@ export class SshConnection { getSystemSshResolvedConfig(): SshResolvedConfig | null { return cloneResolvedConfig(this.systemSshResolvedConfig) } + getHostKeyFingerprint(): string | undefined { + // Why: system SSH does not expose its negotiated key; a fingerprint from a + // failed ssh2 attempt may identify a different load-balanced execution host. + return this.useSystemSshTransport ? undefined : this.hostKeyFingerprint + } setCallbacks(callbacks: SshConnectionCallbacks): void { this.callbacks = callbacks @@ -1075,6 +1082,16 @@ export class SshConnection { const client = new SshClient() let settled = false + // Why: the relay uses the negotiated server key to isolate shared-home + // install locks without comparing PIDs from an unrelated SSH host. + config.hostVerifier = (key: Buffer): boolean => { + if (!this.disposed && connectGeneration === this.connectGeneration) { + const digest = createHash('sha256').update(key).digest('base64').replace(/=+$/, '') + this.hostKeyFingerprint = `SHA256:${digest}` + } + return true + } + const cleanupStartupListeners = (): void => { client.off('ready', onReady) client.off('error', onStartupError) diff --git a/src/main/ssh/ssh-relay-session.test.ts b/src/main/ssh/ssh-relay-session.test.ts index 55abd17b9c6..826911bf0a6 100644 --- a/src/main/ssh/ssh-relay-session.test.ts +++ b/src/main/ssh/ssh-relay-session.test.ts @@ -1,14 +1,14 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' import { SshRelaySession } from './ssh-relay-session' import type { SshConnection } from './ssh-connection' -import { AGENT_HOOK_INSTALL_PLUGINS_METHOD } from '../../shared/agent-hook-relay' +import { + AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD, + AGENT_HOOK_INSTALL_PLUGINS_METHOD +} from '../../shared/agent-hook-relay' import { SSH_RELAY_CONFIGURE_GRACE_TIME_METHOD } from '../../shared/ssh-types' import { createMockDeps, mockDeploySuccess } from './ssh-relay-session-test-fixtures' -const { muxRequestMock, installRemoteManagedAgentHooksMock } = vi.hoisted(() => ({ - muxRequestMock: vi.fn(), - installRemoteManagedAgentHooksMock: vi.fn() -})) +const { muxRequestMock } = vi.hoisted(() => ({ muxRequestMock: vi.fn() })) vi.mock('./ssh-relay-deploy', () => ({ deployAndLaunchRelay: vi.fn() @@ -32,10 +32,6 @@ vi.mock('./ssh-channel-multiplexer', () => { } }) -vi.mock('../agent-hooks/remote-managed-hook-installers', () => ({ - installRemoteManagedAgentHooks: installRemoteManagedAgentHooksMock -})) - vi.mock('../providers/ssh-pty-provider', () => ({ isSshPtyNotFoundError: (err: unknown) => (err instanceof Error ? err.message : String(err)).includes('not found'), @@ -115,8 +111,6 @@ describe('SshRelaySession', () => { delete process.env.ORCA_FEATURE_REMOTE_AGENT_HOOKS muxRequestMock.mockReset() muxRequestMock.mockResolvedValue([]) - installRemoteManagedAgentHooksMock.mockReset() - installRemoteManagedAgentHooksMock.mockResolvedValue([]) mockDeploySuccess() vi.mocked(getPtyIdsForConnection).mockReturnValue([]) _resetHiddenRendererPtyDeliveryGateForTest() @@ -233,99 +227,86 @@ describe('SshRelaySession', () => { expect(registerSshGitProvider).toHaveBeenCalledWith('target-1', expect.anything()) }) - it.each([ - ['/bin/bash', "'/bin/bash' -lc 'printenv GROK_HOME | head -c 4097'"], - ['/usr/bin/fish', "'/usr/bin/fish' -lc 'printenv GROK_HOME | head -c 4097'"], - ['/bin/tcsh', "'/bin/tcsh' -lc 'printenv GROK_HOME | head -c 4097'"], - ['/bin/sh', "'/bin/sh' -c 'printenv GROK_HOME | head -c 4097'"] - ])('uses a shell-independent GROK_HOME probe with login shell %s', async (shell, command) => { + it('installs all managed hooks in one relay RPC before plugins and PTY registration', async () => { process.env.ORCA_FEATURE_REMOTE_AGENT_HOOKS = '1' - muxRequestMock.mockImplementation(async (method: string) => - method === 'session.resolveHome' ? { resolvedPath: '/home/orca' } : { ok: true } - ) - vi.mocked(execCommand).mockResolvedValueOnce(`${shell}\n`).mockResolvedValueOnce('/srv/grok\n') - const sftp = { end: vi.fn() } - const { mockStore, mockPortForward, getMainWindow } = createMockDeps() - const mockConn = { sftp: vi.fn().mockResolvedValue(sftp) } as unknown as SshConnection - const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) - - await session.establish(mockConn) - - expect(execCommand).toHaveBeenNthCalledWith( - 1, - mockConn, - "printenv SHELL || printf '/bin/sh\\n'", - { timeoutMs: 8_000 } - ) - expect(execCommand).toHaveBeenNthCalledWith(2, mockConn, command, { - wrapCommand: false, - timeoutMs: 8_000 - }) - expect(installRemoteManagedAgentHooksMock).toHaveBeenCalledWith(sftp, '/home/orca', { - grokHomeDir: '/srv/grok' - }) - }) - - it('installs remote managed hooks and relay-owned plugin assets before registering the SSH PTY provider', async () => { - process.env.ORCA_FEATURE_REMOTE_AGENT_HOOKS = '1' - muxRequestMock.mockImplementation(async (method: string) => { - if (method === 'session.resolveHome') { - return { resolvedPath: '/home/orca' } - } - return { ok: true } - }) - const sftp = { end: vi.fn() } - vi.mocked(execCommand) - .mockResolvedValueOnce('/bin/bash\n') - .mockResolvedValueOnce('/srv/grok profile\n') + muxRequestMock.mockResolvedValue({ installers: 14, errors: 0 }) const { mockStore, mockPortForward, getMainWindow } = createMockDeps() + const sftp = vi.fn() const mockConn = { - sftp: vi.fn().mockResolvedValue(sftp) + sftp, + getHostKeyFingerprint: vi.fn(() => 'SHA256:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA') } as unknown as SshConnection const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) await session.establish(mockConn) + const managedHookCalls = muxRequestMock.mock.calls.filter( + ([method]) => method === AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD + ) + const managedHookCallIndex = muxRequestMock.mock.calls.findIndex( + ([method]) => method === AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD + ) const installPluginsCallIndex = muxRequestMock.mock.calls.findIndex( ([method]) => method === AGENT_HOOK_INSTALL_PLUGINS_METHOD ) + expect(managedHookCalls).toEqual([ + [ + AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD, + { hostKeyFingerprint: 'SHA256:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA' } + ] + ]) expect(installPluginsCallIndex).toBeGreaterThanOrEqual(0) const installPluginsParams = muxRequestMock.mock.calls[installPluginsCallIndex]?.[1] expect(installPluginsParams).toMatchObject({ piExtensionSource: expect.stringContaining('/hook/pi'), ompExtensionSource: expect.stringContaining('/hook/omp') }) - expect(mockConn.sftp).toHaveBeenCalledTimes(1) - expect(installRemoteManagedAgentHooksMock).toHaveBeenCalledWith(sftp, '/home/orca', { - grokHomeDir: '/srv/grok profile' - }) - expect(sftp.end).toHaveBeenCalledTimes(1) - expect(installRemoteManagedAgentHooksMock.mock.invocationCallOrder[0]).toBeLessThan( - muxRequestMock.mock.invocationCallOrder[installPluginsCallIndex] - ) + expect(sftp).not.toHaveBeenCalled() + expect(managedHookCallIndex).toBeLessThan(installPluginsCallIndex) expect(muxRequestMock.mock.invocationCallOrder[installPluginsCallIndex]).toBeLessThan( vi.mocked(registerSshPtyProvider).mock.invocationCallOrder[0] ) }) - it('falls back to login-home Grok config when the remote env probe is invalid', async () => { + it('continues provider registration when the relay managed-hook request fails', async () => { process.env.ORCA_FEATURE_REMOTE_AGENT_HOOKS = '1' - muxRequestMock.mockImplementation(async (method: string) => - method === 'session.resolveHome' ? { resolvedPath: '/home/orca' } : { ok: true } - ) - vi.mocked(execCommand) - .mockResolvedValueOnce('/bin/bash\n') - .mockResolvedValueOnce('relative/grok-home\n') - const sftp = { end: vi.fn() } + muxRequestMock.mockImplementation(async (method: string) => { + if (method === AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD) { + throw new Error('runtime unavailable') + } + return { ok: true } + }) const { mockStore, mockPortForward, getMainWindow } = createMockDeps() - const mockConn = { sftp: vi.fn().mockResolvedValue(sftp) } as unknown as SshConnection const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) - await session.establish(mockConn) + await session.establish({} as SshConnection) - expect(installRemoteManagedAgentHooksMock).toHaveBeenCalledWith(sftp, '/home/orca', { - grokHomeDir: '/home/orca/.grok' + expect(registerSshPtyProvider).toHaveBeenCalledWith('target-1', expect.anything()) + expect( + muxRequestMock.mock.calls.some(([method]) => method === AGENT_HOOK_INSTALL_PLUGINS_METHOD) + ).toBe(true) + }) + + it('suppresses expected managed-hook teardown errors during disconnect', async () => { + process.env.ORCA_FEATURE_REMOTE_AGENT_HOOKS = '1' + muxRequestMock.mockImplementation(async (method: string) => { + if (method === AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD) { + throw Object.assign(new Error('request disposed'), { code: 'DISPOSED' }) + } + return { ok: true } }) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const { mockStore, mockPortForward, getMainWindow } = createMockDeps() + const session = new SshRelaySession('target-1', getMainWindow, mockStore, mockPortForward) + + try { + await session.establish({} as SshConnection) + + expect(registerSshPtyProvider).toHaveBeenCalledWith('target-1', expect.anything()) + expect(warn.mock.calls.flat().join(' ')).not.toContain('relay managed hook install failed') + } finally { + warn.mockRestore() + } }) it('does not run POSIX managed hook installers on Windows remotes', async () => { @@ -351,7 +332,11 @@ describe('SshRelaySession', () => { await session.establish(mockConn) - expect(installRemoteManagedAgentHooksMock).not.toHaveBeenCalled() + expect( + muxRequestMock.mock.calls.some( + ([method]) => method === AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD + ) + ).toBe(false) expect( muxRequestMock.mock.calls.some(([method]) => method === AGENT_HOOK_INSTALL_PLUGINS_METHOD) ).toBe(true) diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 0428a4d3086..146f2b2dd5b 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -16,9 +16,9 @@ import { toAppSshPtyId, toRelaySshPtyId } from '../providers/ssh-pty-id' import { SshFilesystemProvider } from '../providers/ssh-filesystem-provider' import { SshGitProvider } from '../providers/ssh-git-provider' import { agentHookServer } from '../agent-hooks/server' -import { installRemoteManagedAgentHooks } from '../agent-hooks/remote-managed-hook-installers' import { isAgentStatusHooksEnabled } from '../agent-hooks/managed-agent-hook-controls' import { + AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD, AGENT_HOOK_INSTALL_PLUGINS_METHOD, AGENT_HOOK_NOTIFICATION_METHOD, AGENT_HOOK_REQUEST_REPLAY_METHOD, @@ -68,7 +68,6 @@ import { runRemoteOrcaCli } from './ssh-remote-orca-cli' import { toSshExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' import { isTerminalLeafId, makePaneKey } from '../../shared/stable-pane-id' import { isValidTerminalTabId } from '../../shared/terminal-tab-id' -import { shellEscape } from './ssh-connection-utils' export type RelaySessionState = 'idle' | 'deploying' | 'ready' | 'reconnecting' | 'disposed' @@ -84,67 +83,6 @@ type RemoteCliBridgeEnv = { type ExpectedPtyIdentity = { paneKey?: string; tabId?: string } -const REMOTE_GROK_HOME_MAX_LENGTH = 4096 -const REMOTE_GROK_HOME_PROBE_TIMEOUT_MS = 8_000 - -function defaultRemoteGrokHome(remoteHome: string): string { - const home = remoteHome.replace(/\/+$/, '') || remoteHome - return `${home}/.grok` -} - -function normalizeRemoteGrokHome(candidate: string): string | null { - if ( - candidate.length === 0 || - candidate.length > REMOTE_GROK_HOME_MAX_LENGTH || - candidate !== candidate.trim() || - !candidate.startsWith('/') || - candidate.includes('\\') || - hasControlCharacter(candidate) - ) { - return null - } - return candidate.replace(/\/+$/, '') || '/' -} - -function hasControlCharacter(value: string): boolean { - return Array.from(value).some((character) => { - const code = character.charCodeAt(0) - return code <= 0x1f || code === 0x7f - }) -} - -function loginShellCommand(shell: string, command: string): string { - const shellName = shell.split('/').at(-1) - const mode = shellName === 'sh' || shellName === 'dash' ? '-c' : '-lc' - return `${shellEscape(shell)} ${mode} ${shellEscape(command)}` -} - -async function resolveRemoteGrokHome( - connection: SshConnection, - remoteHome: string -): Promise { - const fallback = defaultRemoteGrokHome(remoteHome) - try { - // Why: remote PTYs start login shells, so probe that profile-derived env, not the relay service or local Electron env. - const shellOutput = await execCommand(connection, "printenv SHELL || printf '/bin/sh\\n'", { - timeoutMs: REMOTE_GROK_HOME_PROBE_TIMEOUT_MS - }) - const shell = shellOutput.trim().split(/\r?\n/, 1)[0] - if (!shell?.startsWith('/') || hasControlCharacter(shell)) { - return fallback - } - // Why: runs in the user's login shell (maybe fish/tcsh), so use external commands to avoid shell-specific syntax. - const probe = `printenv GROK_HOME | head -c ${REMOTE_GROK_HOME_MAX_LENGTH + 1}` - const output = await execCommand(connection, loginShellCommand(shell, probe), { - wrapCommand: false, - timeoutMs: REMOTE_GROK_HOME_PROBE_TIMEOUT_MS - }) - return normalizeRemoteGrokHome(output.split(/\r?\n/, 1)[0] ?? '') ?? fallback - } catch { - return fallback - } -} - function expectedIdentityForLease(lease: { tabId?: string leafId?: string @@ -646,7 +584,7 @@ export class SshRelaySession { }) } - // Why: hook-script agents need remote config files (relay env alone isn't enough); install before the PTY provider so first prompts report status. + // Why: hooks must exist before PTY spawn; relay-local work keeps all managed installs to one SSH round trip. private async installManagedHooksOnRemote(mux: SshChannelMultiplexer): Promise { if (!isRemoteAgentHooksEnabled() || !this.areAgentStatusHooksEnabled()) { return @@ -659,41 +597,34 @@ export class SshRelaySession { return } - let remoteHome: string try { - const result = (await mux.request('session.resolveHome', { path: '~' })) as { - resolvedPath?: unknown + const hostKeyFingerprint = this.requireReadyConnection().getHostKeyFingerprint?.() + const params = hostKeyFingerprint ? { hostKeyFingerprint } : {} + const result = (await mux.request(AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD, params)) as { + errors?: unknown } - if (typeof result.resolvedPath !== 'string' || result.resolvedPath.length === 0) { + if (typeof result.errors === 'number' && result.errors > 0) { console.warn( - `[ssh-relay-session] skipped remote managed hook install for ${this.targetId}: could not resolve remote home` + `[ssh-relay-session] ${result.errors} remote managed hook installers failed for ${this.targetId}` ) + } + } catch (error) { + // Why: teardown routinely cancels this best-effort request; only warn for + // installer failures that survive the connection lifecycle. + const code = (error as { code?: unknown })?.code + if ( + code === -32601 || + code === 'CONNECTION_LOST' || + code === 'DISPOSED' || + mux.isDisposed() + ) { return } - remoteHome = result.resolvedPath - } catch (error) { console.warn( - `[ssh-relay-session] skipped remote managed hook install for ${this.targetId}: ${ + `[ssh-relay-session] relay managed hook install failed for ${this.targetId}: ${ error instanceof Error ? error.message : String(error) }` ) - return - } - - let sftp: Awaited> | null = null - try { - const connection = this.requireReadyConnection() - const remoteGrokHome = await resolveRemoteGrokHome(connection, remoteHome) - sftp = await connection.sftp() - await installRemoteManagedAgentHooks(sftp, remoteHome, { grokHomeDir: remoteGrokHome }) - } catch (error) { - console.warn( - `[ssh-relay-session] remote managed hook install failed for ${this.targetId}: ${ - error instanceof Error ? error.message : String(error) - }` - ) - } finally { - ;(sftp as { end?: () => void } | null)?.end?.() } } diff --git a/src/main/ssh/ssh-relay-versioned-install.test.ts b/src/main/ssh/ssh-relay-versioned-install.test.ts index 9bd3f73c8df..23d57757b5b 100644 --- a/src/main/ssh/ssh-relay-versioned-install.test.ts +++ b/src/main/ssh/ssh-relay-versioned-install.test.ts @@ -128,12 +128,13 @@ describe('isRelayAlreadyInstalled', () => { ).rejects.toBe(sessionLimitError) }) - it('checks for both relay process artifacts and .install-complete', async () => { + it('checks every relay runtime artifact and .install-complete', async () => { mockExec.mockResolvedValueOnce('OK') await isRelayAlreadyInstalled(conn, '/r') const cmd = mockExec.mock.calls.at(-1)?.[1] ?? '' expect(cmd).toContain('relay.js') expect(cmd).toContain('relay-watcher.js') + expect(cmd).toContain('managed-hook-runtime.js') expect(cmd).toContain('.install-complete') }) }) diff --git a/src/main/ssh/ssh-relay-versioned-install.ts b/src/main/ssh/ssh-relay-versioned-install.ts index def8840ca57..9945c997ef2 100644 --- a/src/main/ssh/ssh-relay-versioned-install.ts +++ b/src/main/ssh/ssh-relay-versioned-install.ts @@ -104,9 +104,8 @@ export function computeRemoteRelayDir( } /** - * Probe whether a fully-installed relay exists at remoteRelayDir — meaning - * relay.js, its relay-watcher.js child, and the .install-complete sentinel are - * all present. Missing artifacts force a complete re-deploy. + * Probe for relay.js, its watcher, the managed-hook runtime, and the + * completion sentinel. Any missing artifact forces a complete re-deploy. */ export async function isRelayAlreadyInstalled( conn: SshConnection, diff --git a/src/main/ssh/ssh-remote-commands.test.ts b/src/main/ssh/ssh-remote-commands.test.ts index 96cc8925515..594ed2864d5 100644 --- a/src/main/ssh/ssh-remote-commands.test.ts +++ b/src/main/ssh/ssh-remote-commands.test.ts @@ -108,7 +108,9 @@ describe('ssh remote command builders', () => { it('keeps POSIX deploy commands POSIX-native', () => { expect(readRemoteHomeCommand(posix)).toBe('echo $HOME') expect(makeRemoteDirectoryCommand(posix, '/home/me/.orca-remote')).toContain('mkdir -p') - expect(probeRelayInstalledCommand(posix, '/home/me/relay')).toContain('test -d') + const probe = probeRelayInstalledCommand(posix, '/home/me/relay') + expect(probe).toContain('test -d') + expect(probe).toContain('managed-hook-runtime.js') }) it('uses encoded PowerShell for Windows deploy commands', () => { @@ -116,7 +118,9 @@ describe('ssh remote command builders', () => { expect(makeRemoteDirectoryCommand(windows, 'C:/Users/me/.orca-remote')).toContain( '-EncodedCommand' ) - expect(probeRelayInstalledCommand(windows, 'C:/Users/me/relay')).toContain('-EncodedCommand') + const probe = probeRelayInstalledCommand(windows, 'C:/Users/me/relay') + expect(probe).toContain('-EncodedCommand') + expect(decodePowerShellCommand(probe)).toContain('managed-hook-runtime.js') }) it('uses a legacy-visible Windows lock directory with an exclusive owner file', () => { diff --git a/src/main/ssh/ssh-remote-commands.ts b/src/main/ssh/ssh-remote-commands.ts index b70761f8d54..155fb5ba66f 100644 --- a/src/main/ssh/ssh-remote-commands.ts +++ b/src/main/ssh/ssh-remote-commands.ts @@ -78,12 +78,14 @@ export function probeRelayInstalledCommand( ): string { const relayJs = joinRemotePath(host, remoteRelayDir, 'relay.js') const relayWatcherJs = joinRemotePath(host, remoteRelayDir, 'relay-watcher.js') + const managedHookRuntimeJs = joinRemotePath(host, remoteRelayDir, 'managed-hook-runtime.js') const installComplete = joinRemotePath(host, remoteRelayDir, '.install-complete') if (!isWindowsRemoteHost(host)) { return ( `test -d ${shellEscape(remoteRelayDir)} ` + `&& test -f ${shellEscape(relayJs)} ` + `&& test -f ${shellEscape(relayWatcherJs)} ` + + `&& test -f ${shellEscape(managedHookRuntimeJs)} ` + `&& test -f ${shellEscape(installComplete)} ` + `&& echo OK || echo MISSING` ) @@ -93,8 +95,9 @@ export function probeRelayInstalledCommand( `$dir = ${powerShellLiteral(remoteRelayDir)}`, `$relay = ${powerShellLiteral(relayJs)}`, `$watcher = ${powerShellLiteral(relayWatcherJs)}`, + `$managedHooks = ${powerShellLiteral(managedHookRuntimeJs)}`, `$complete = ${powerShellLiteral(installComplete)}`, - "if ((Test-Path -LiteralPath $dir -PathType Container) -and (Test-Path -LiteralPath $relay -PathType Leaf) -and (Test-Path -LiteralPath $watcher -PathType Leaf) -and (Test-Path -LiteralPath $complete -PathType Leaf)) { 'OK' } else { 'MISSING' }" + "if ((Test-Path -LiteralPath $dir -PathType Container) -and (Test-Path -LiteralPath $relay -PathType Leaf) -and (Test-Path -LiteralPath $watcher -PathType Leaf) -and (Test-Path -LiteralPath $managedHooks -PathType Leaf) -and (Test-Path -LiteralPath $complete -PathType Leaf)) { 'OK' } else { 'MISSING' }" ].join('; ') ) } diff --git a/src/main/ssh/ssh-system-transport.integration.test.ts b/src/main/ssh/ssh-system-transport.integration.test.ts index b949dc411af..33e5fb3c80d 100644 --- a/src/main/ssh/ssh-system-transport.integration.test.ts +++ b/src/main/ssh/ssh-system-transport.integration.test.ts @@ -142,6 +142,7 @@ function createRelayTree(root: string, remoteHome: string): void { writeFileSync(join(remoteDir, 'node_modules', 'node-pty', 'index.js'), '') writeFileSync(join(remoteDir, 'node_modules', '@parcel', 'watcher', 'index.js'), '') writeFileSync(join(remoteDir, '.install-complete'), '') + writeFileSync(join(remoteDir, 'managed-hook-runtime.js'), '') writeFakeRelay(remoteDir) } diff --git a/src/relay/managed-hook-installer.test.ts b/src/relay/managed-hook-installer.test.ts new file mode 100644 index 00000000000..a060114ffcc --- /dev/null +++ b/src/relay/managed-hook-installer.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it, vi } from 'vitest' +import { AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD } from '../shared/agent-hook-relay' +import type { MethodHandler, RequestContext } from './dispatcher' +import { registerManagedHookInstaller, type ManagedHookRuntime } from './managed-hook-installer' + +function captureHandler(loadRuntime: () => ManagedHookRuntime): MethodHandler { + let handler: MethodHandler | undefined + registerManagedHookInstaller( + { + onRequest: (method, nextHandler) => { + expect(method).toBe(AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD) + handler = nextHandler + } + }, + loadRuntime + ) + return handler! +} + +function context(signal?: AbortSignal): RequestContext { + return { clientId: 1, isStale: () => signal?.aborted ?? false, signal } +} + +describe('registerManagedHookInstaller', () => { + it('forwards request cancellation to the remote runtime', async () => { + const controller = new AbortController() + const installManagedHooks = vi.fn().mockResolvedValue({ installers: 14, errors: 0 }) + const handler = captureHandler(() => ({ installManagedHooks })) + + await expect(handler({}, context(controller.signal))).resolves.toEqual({ + installers: 14, + errors: 0 + }) + expect(installManagedHooks).toHaveBeenCalledWith({ signal: controller.signal }) + }) + + it('does not load or start the runtime for an already-cancelled request', async () => { + const controller = new AbortController() + controller.abort() + const loadRuntime = vi.fn() + const handler = captureHandler(loadRuntime) + + await expect(handler({}, context(controller.signal))).rejects.toMatchObject({ + name: 'AbortError' + }) + expect(loadRuntime).not.toHaveBeenCalled() + }) + + it('forwards only a valid negotiated server-key fingerprint', async () => { + const installManagedHooks = vi.fn().mockResolvedValue({ installers: 14, errors: 0 }) + const handler = captureHandler(() => ({ installManagedHooks })) + const fingerprint = 'SHA256:AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA' + + await handler({ hostKeyFingerprint: fingerprint }, context()) + await handler({ hostKeyFingerprint: 'ssh://untrusted-host' }, context()) + + expect(installManagedHooks).toHaveBeenNthCalledWith(1, { + signal: undefined, + hostKeyFingerprint: fingerprint + }) + expect(installManagedHooks).toHaveBeenNthCalledWith(2, { signal: undefined }) + }) +}) diff --git a/src/relay/managed-hook-installer.ts b/src/relay/managed-hook-installer.ts new file mode 100644 index 00000000000..a27a6b8bf49 --- /dev/null +++ b/src/relay/managed-hook-installer.ts @@ -0,0 +1,56 @@ +import { join } from 'node:path' +import { + AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD, + type AgentHookInstallManagedHooksParams +} from '../shared/agent-hook-relay' +import type { RelayDispatcher, RequestContext } from './dispatcher' + +export type ManagedHookInstallSummary = { + installers: number + errors: number +} + +export type ManagedHookRuntime = { + installManagedHooks: (options?: { + signal?: AbortSignal + hostKeyFingerprint?: string + }) => Promise +} + +const SHA256_HOST_KEY_PATTERN = /^SHA256:[A-Za-z\d+/]{43}$/ + +function readHostKeyFingerprint(params: unknown): string | undefined { + const fingerprint = (params as Partial | null) + ?.hostKeyFingerprint + return typeof fingerprint === 'string' && SHA256_HOST_KEY_PATTERN.test(fingerprint) + ? fingerprint + : undefined +} + +let managedHookRuntime: ManagedHookRuntime | null = null + +function loadManagedHookRuntime(): ManagedHookRuntime { + if (!managedHookRuntime) { + // Why: keep the sizeable installer implementation out of relay startup and + // its narrow TS project while still executing it in-process on the remote. + managedHookRuntime = require(join(__dirname, 'managed-hook-runtime.js')) as ManagedHookRuntime + } + return managedHookRuntime +} + +export function registerManagedHookInstaller( + dispatcher: Pick, + loadRuntime: () => ManagedHookRuntime = loadManagedHookRuntime +): void { + dispatcher.onRequest( + AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD, + async (params, context: RequestContext): Promise => { + context.signal?.throwIfAborted() + const hostKeyFingerprint = readHostKeyFingerprint(params) + return await loadRuntime().installManagedHooks({ + signal: context.signal, + ...(hostKeyFingerprint ? { hostKeyFingerprint } : {}) + }) + } + ) +} diff --git a/src/relay/relay.ts b/src/relay/relay.ts index 2e3c77bb4b3..e279bec49c7 100644 --- a/src/relay/relay.ts +++ b/src/relay/relay.ts @@ -52,6 +52,7 @@ import { pickRemoteCliEnv } from './remote-cli-env' import { relayLogLine } from './relay-diagnostic-log' import { remoteCliRequestTimeoutMs } from './remote-cli-timeout' import { shouldReadRemoteCliStdin } from './remote-cli-stdin' +import { registerManagedHookInstaller } from './managed-hook-installer' const DEFAULT_GRACE_MS = DEFAULT_SSH_RELAY_GRACE_PERIOD_SECONDS * 1000 const SOCK_NAME = 'relay.sock' @@ -540,6 +541,9 @@ async function main(): Promise { return { replayed } }) + // Why: relay-local installers collapse hundreds of SFTP request/response RTTs to one RPC. + registerManagedHookInstaller(dispatcher) + // Why: plugin sources ship over the wire so an Orca update doesn't force a relay redeploy; cache them per spawn. See docs/design/agent-status-over-ssh.md §4. // Why: bound per-source size so a buggy/hostile Orca can't OOM the relay by pushing a giant string. dispatcher.onRequest(AGENT_HOOK_INSTALL_PLUGINS_METHOD, async (params) => { diff --git a/src/shared/agent-hook-relay.ts b/src/shared/agent-hook-relay.ts index 6469a002270..9cbd6c18faa 100644 --- a/src/shared/agent-hook-relay.ts +++ b/src/shared/agent-hook-relay.ts @@ -108,6 +108,15 @@ export const AGENT_HOOK_REQUEST_REPLAY_METHOD = 'agent_hook.requestReplay' as co * overlay dirs on the remote. */ export const AGENT_HOOK_INSTALL_PLUGINS_METHOD = 'agent_hook.installPlugins' as const +/** JSON-RPC request method that asks the remote relay to install every + * managed hook using its local filesystem instead of WAN-bound SFTP. */ +export const AGENT_HOOK_INSTALL_MANAGED_HOOKS_METHOD = 'agent_hook.installManagedHooks' as const + +export type AgentHookInstallManagedHooksParams = { + /** SHA-256 fingerprint of the server key negotiated by Orca's SSH transport. */ + hostKeyFingerprint?: string +} + /** Feature-flag env var. Read once at process start by Orca and the relay. * Remote agent hooks ship as the default SSH behavior; set "0" to opt out. */ export const ORCA_FEATURE_REMOTE_AGENT_HOOKS_ENV = 'ORCA_FEATURE_REMOTE_AGENT_HOOKS' as const From 1d2ba1e39de05ebba74d249d251f0b9df85156f6 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 20 Jul 2026 12:50:47 -0700 Subject: [PATCH 5/5] fix: never surface coding-agent scratch worktrees in the sidebar (#9535) * fix: never surface coding-agent scratch worktrees in the sidebar Sub-agent runs (e.g. Claude Code worktree isolation) create throwaway git worktrees at tool-internal paths like /.claude/worktrees/. These showed up as workspace rows for repos whose non-Orca worktree visibility is 'show', and would otherwise ping the discovery card and new-worktrees inbox on every fan-out. Classify them as 'agent-scratch' via a curated path-segment matcher and suppress them unconditionally: sidebar, discovery card, inbox, dialog counts, add-handoff reveal trigger, and the metadata fallback. Selected checkouts and explicitly imported paths still win. Fixes #9388 * fix: preserve scratch worktree visibility boundaries * fix: hide scratch worktrees from linked checkouts * docs: explain scratch root normalization --- src/main/ipc/worktrees.test.ts | 42 +++++ src/main/ipc/worktrees.ts | 31 ++-- src/main/runtime/orca-runtime.test.ts | 25 +++ src/main/runtime/orca-runtime.ts | 57 +++++-- .../sidebar/WorktreeVisibilityDialog.tsx | 16 +- ...imported-worktrees-card-candidates.test.ts | 25 ++- .../project-added-default-checkout.test.ts | 30 +++- .../sidebar/project-added-default-checkout.ts | 5 +- src/shared/agent-scratch-worktrees.test.ts | 103 ++++++++++++ src/shared/agent-scratch-worktrees.ts | 39 +++++ src/shared/external-worktree-inbox.test.ts | 12 ++ src/shared/external-worktree-inbox.ts | 24 ++- src/shared/types.ts | 2 +- src/shared/worktree-ownership.test.ts | 158 ++++++++++++++++++ src/shared/worktree-ownership.ts | 32 ++++ 15 files changed, 562 insertions(+), 39 deletions(-) create mode 100644 src/shared/agent-scratch-worktrees.test.ts create mode 100644 src/shared/agent-scratch-worktrees.ts diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 3dcf1d46593..bcbc940f72e 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -2306,6 +2306,48 @@ describe('registerWorktreeHandlers', () => { }) }) + it('hides agent scratch created inside a linked checkout from desktop listings', async () => { + const linkedCheckoutPath = '/workspace/feature-x' + const scratchPath = `${linkedCheckoutPath}/.claude/worktrees/agent-a04ccaaa` + listWorktreesMock.mockResolvedValue([ + { + path: '/workspace/repo', + head: 'main-head', + branch: 'refs/heads/main', + isBare: false, + isMainWorktree: true + }, + { + path: linkedCheckoutPath, + head: 'feature-head', + branch: 'refs/heads/feature-x', + isBare: false, + isMainWorktree: false + }, + { + path: scratchPath, + head: 'scratch-head', + branch: 'refs/heads/worktree-agent-a04ccaaa', + isBare: false, + isMainWorktree: false + } + ]) + + const detected = (await handlers['worktrees:listDetected'](null, { + repoId: 'repo-1' + })) as { worktrees: (Worktree & { ownership: string; visible: boolean })[] } + const visible = (await handlers['worktrees:list'](null, { repoId: 'repo-1' })) as Worktree[] + + expect(detected.worktrees.find((worktree) => worktree.path === scratchPath)).toMatchObject({ + ownership: 'agent-scratch', + visible: false + }) + expect(visible.map((worktree) => worktree.path)).toEqual([ + '/workspace/repo', + linkedCheckoutPath + ]) + }) + it('does not reuse host detected worktree scans for a selected WSL runtime', async () => { listWorktreesMock .mockResolvedValueOnce([ diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index f3256b5b171..ee878ae0926 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -33,10 +33,12 @@ import type { import { assertWorktreeUnlockedForRemoval } from '../../shared/worktree-removal' import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' import { + applyMetadataFallbackVisibility, buildKnownOrcaWorkspaceLayouts, isLegacyRepoForExternalWorktreeVisibility, toDetectedWorktree } from '../../shared/worktree-ownership' +import { createAgentScratchWorktreePathMatcher } from '../../shared/agent-scratch-worktrees' import { assertWorktreeCleanForRemoval, forceDeleteLocalBranch, @@ -729,8 +731,14 @@ function buildDetectedGitWorktrees( const knownOrcaLayouts = buildKnownOrcaWorkspaceLayouts(settings, repo) const isLegacyRepoForVisibility = isLegacyRepoForExternalWorktreeVisibility(repo) // Why: a prunable registration has no working directory (issue #8389); only this listing omits it — cleanup flows list separately. - const liveWorktrees = gitWorktrees.filter((gitWorktree) => !gitWorktree.prunable) - return dedupeWorktreesByPath(liveWorktrees).map((gitWorktree) => { + const liveWorktrees = dedupeWorktreesByPath( + gitWorktrees.filter((gitWorktree) => !gitWorktree.prunable) + ) + const agentScratchWorktreePathMatcher = createAgentScratchWorktreePathMatcher([ + repo.path, + ...liveWorktrees.map((worktree) => worktree.path) + ]) + return liveWorktrees.map((gitWorktree) => { const worktreeId = `${repo.id}::${gitWorktree.path}` let meta = store.getWorktreeMeta(worktreeId) const worktree = mergeWorktree(repo.id, gitWorktree, meta, repo.displayName) @@ -740,7 +748,8 @@ function buildDetectedGitWorktrees( meta, settings, knownOrcaLayouts, - isLegacyRepoForVisibility + isLegacyRepoForVisibility, + agentScratchWorktreePathMatcher }) if (!detected.visible) { return detected @@ -753,7 +762,8 @@ function buildDetectedGitWorktrees( meta, settings, knownOrcaLayouts, - isLegacyRepoForVisibility + isLegacyRepoForVisibility, + agentScratchWorktreePathMatcher }) }) } @@ -945,6 +955,10 @@ function buildDisconnectedDetectedWorktrees( worktrees: Worktree[] ): DetectedWorktree[] { const settings = store.getSettings() + const agentScratchWorktreePathMatcher = createAgentScratchWorktreePathMatcher([ + repo.path, + ...worktrees.map((worktree) => worktree.path) + ]) return worktrees.map((worktree) => { const meta = store.getWorktreeMeta(worktree.id) const detected = toDetectedWorktree({ @@ -953,13 +967,10 @@ function buildDisconnectedDetectedWorktrees( meta, settings, knownOrcaLayouts: [], - isLegacyRepoForVisibility: true + isLegacyRepoForVisibility: true, + agentScratchWorktreePathMatcher }) - return { - ...detected, - visible: true, - ownership: detected.ownership === 'orca-managed' ? 'orca-managed' : 'unknown-legacy' - } + return applyMetadataFallbackVisibility(detected) }) } diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 4939d056a46..a63d25b9a04 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -26417,6 +26417,31 @@ describe('OrcaRuntimeService', () => { expect(removeWorktreeLineage).not.toHaveBeenCalled() }) + it('hides agent scratch created inside a linked checkout from runtime listings', async () => { + const linkedCheckoutPath = '/tmp/worktree-a' + const scratchPath = `${linkedCheckoutPath}/.claude/worktrees/agent-a04ccaaa` + vi.mocked(listWorktrees).mockClear() + vi.mocked(listWorktrees).mockResolvedValue([ + makeWorktreeInfo(TEST_REPO_PATH), + makeWorktreeInfo(linkedCheckoutPath), + makeWorktreeInfo(scratchPath) + ]) + const runtime = createRuntime() + + const detected = await runtime.listDetectedManagedWorktrees(`id:${TEST_REPO_ID}`) + const listed = await runtime.listManagedWorktrees(`id:${TEST_REPO_ID}`) + + expect(detected.worktrees.find((worktree) => worktree.path === scratchPath)).toMatchObject({ + ownership: 'agent-scratch', + visible: false + }) + expect(listed.worktrees.map((worktree) => worktree.path)).toEqual([ + TEST_REPO_PATH, + linkedCheckoutPath + ]) + expect(listWorktrees).toHaveBeenCalledTimes(1) + }) + it('bounds repeated detected worktree scans across the reported 15-repo shape', async () => { vi.mocked(listWorktrees).mockReset() const repos = Array.from({ length: 15 }, (_, index) => ({ diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index fe9eb66574d..fbb426994a2 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -264,10 +264,15 @@ import type { FolderWorkspacePathStatusRequest } from '../../shared/folder-workspace-path-status' import { + applyMetadataFallbackVisibility, buildKnownOrcaWorkspaceLayouts, isLegacyRepoForExternalWorktreeVisibility, toDetectedWorktree } from '../../shared/worktree-ownership' +import { + createAgentScratchWorktreePathMatcher, + type AgentScratchWorktreePathMatcher +} from '../../shared/agent-scratch-worktrees' import { BROWSER_HEADLESS_RUNTIME_CAPABILITY, BROWSER_CERTIFICATE_TRUST_RUNTIME_CAPABILITY, @@ -15273,11 +15278,29 @@ export class OrcaRuntimeService { } const resolved = await this.listResolvedWorktrees() const repoId = repoSelector ? (await this.resolveRepoSelector(repoSelector)).id : null + const checkoutPathsByRepoId = new Map() + for (const worktree of resolved) { + const checkoutPaths = checkoutPathsByRepoId.get(worktree.repoId) ?? [] + checkoutPaths.push(worktree.path) + checkoutPathsByRepoId.set(worktree.repoId, checkoutPaths) + } + const agentScratchMatchersByRepoId = new Map( + (this.store?.getRepos() ?? []).map((repo) => [ + repo.id, + createAgentScratchWorktreePathMatcher([ + repo.path, + ...(checkoutPathsByRepoId.get(repo.id) ?? []) + ]) + ]) + ) const worktrees = resolved.filter((worktree) => { if (repoId && worktree.repoId !== repoId) { return false } - return this.isRuntimeWorktreeVisible(worktree) + return this.isRuntimeWorktreeVisible( + worktree, + agentScratchMatchersByRepoId.get(worktree.repoId) + ) }) return { worktrees: worktrees.slice(0, limit), @@ -15306,19 +15329,23 @@ export class OrcaRuntimeService { if (scan.ok) { this.pruneLineageForMissingRepoWorktrees(repo, scan.worktrees) } + const agentScratchWorktreePathMatcher = createAgentScratchWorktreePathMatcher([ + repo.path, + ...scan.worktrees.map((worktree) => worktree.path) + ]) const detected = scan.worktrees.map((gitWorktree) => { const worktreeId = `${repo.id}::${gitWorktree.path}` const meta = this.store?.getWorktreeMeta(worktreeId) const worktree = mergeWorktree(repo.id, gitWorktree, meta, repo.displayName) - const detectedWorktree = this.toRuntimeDetectedWorktree(repo, worktree) + const detectedWorktree = this.toRuntimeDetectedWorktree( + repo, + worktree, + agentScratchWorktreePathMatcher + ) if (scan.ok) { return detectedWorktree } - return { - ...detectedWorktree, - visible: true, - ownership: detectedWorktree.ownership === 'orca-managed' ? 'orca-managed' : 'unknown-legacy' - } satisfies DetectedWorktree + return applyMetadataFallbackVisibility(detectedWorktree) }) return { repoId: repo.id, @@ -15328,15 +15355,22 @@ export class OrcaRuntimeService { } } - private isRuntimeWorktreeVisible(worktree: Worktree): boolean { + private isRuntimeWorktreeVisible( + worktree: Worktree, + agentScratchWorktreePathMatcher?: AgentScratchWorktreePathMatcher + ): boolean { const repo = this.store?.getRepo(worktree.repoId) if (!repo || !this.store) { return true } - return this.toRuntimeDetectedWorktree(repo, worktree).visible + return this.toRuntimeDetectedWorktree(repo, worktree, agentScratchWorktreePathMatcher).visible } - private toRuntimeDetectedWorktree(repo: Repo, worktree: Worktree): DetectedWorktree { + private toRuntimeDetectedWorktree( + repo: Repo, + worktree: Worktree, + agentScratchWorktreePathMatcher?: AgentScratchWorktreePathMatcher + ): DetectedWorktree { const settings = this.store?.getSettings() if (!settings) { return { @@ -15352,7 +15386,8 @@ export class OrcaRuntimeService { meta: this.store?.getWorktreeMeta(worktree.id), settings, knownOrcaLayouts: buildKnownOrcaWorkspaceLayouts(settings, repo), - isLegacyRepoForVisibility: isLegacyRepoForExternalWorktreeVisibility(repo) + isLegacyRepoForVisibility: isLegacyRepoForExternalWorktreeVisibility(repo), + agentScratchWorktreePathMatcher }) } diff --git a/src/renderer/src/components/sidebar/WorktreeVisibilityDialog.tsx b/src/renderer/src/components/sidebar/WorktreeVisibilityDialog.tsx index 71c6fa793c2..5cad24b5ec7 100644 --- a/src/renderer/src/components/sidebar/WorktreeVisibilityDialog.tsx +++ b/src/renderer/src/components/sidebar/WorktreeVisibilityDialog.tsx @@ -9,6 +9,10 @@ import { DialogTitle } from '@/components/ui/dialog' import { Button } from '@/components/ui/button' +import { + getHiddenExternalWorktrees, + getVisibleExternalWorktrees +} from '../../../../shared/external-worktree-inbox' import { isGitRepoKind } from '../../../../shared/repo-kind' import { effectiveExternalWorktreeVisibility, @@ -32,16 +36,8 @@ export default function WorktreeVisibilityDialog(): React.JSX.Element | null { ? effectiveExternalWorktreeVisibility(repo, isLegacyRepoForExternalWorktreeVisibility(repo)) === 'show' : false - const hiddenCount = - detected?.authoritative === true - ? detected.worktrees.filter((worktree) => !worktree.visible).length - : 0 - const otherCount = - detected?.authoritative === true - ? detected.worktrees.filter( - (worktree) => !worktree.selectedCheckout && worktree.ownership !== 'orca-managed' - ).length - : 0 + const hiddenCount = getHiddenExternalWorktrees(detected).length + const otherCount = getVisibleExternalWorktrees(detected).length const hiddenWorktreeLabel = `${hiddenCount} ${hiddenCount === 1 ? 'worktree' : 'worktrees'}` const shownWorktreeLabel = `${otherCount} ${otherCount === 1 ? 'worktree' : 'worktrees'}` diff --git a/src/renderer/src/components/sidebar/imported-worktrees-card-candidates.test.ts b/src/renderer/src/components/sidebar/imported-worktrees-card-candidates.test.ts index 985100fd108..5a6ed56abfc 100644 --- a/src/renderer/src/components/sidebar/imported-worktrees-card-candidates.test.ts +++ b/src/renderer/src/components/sidebar/imported-worktrees-card-candidates.test.ts @@ -75,7 +75,12 @@ describe('getHiddenImportedWorktrees', () => { hidden, detectedWorktree({ id: 'visible', visible: true }), detectedWorktree({ id: 'selected', selectedCheckout: true }), - detectedWorktree({ id: 'orca-managed', ownership: 'orca-managed' }) + detectedWorktree({ id: 'orca-managed', ownership: 'orca-managed' }), + detectedWorktree({ + id: 'agent-scratch', + path: '/repo/.claude/worktrees/agent-1', + ownership: 'agent-scratch' + }) ]) ) @@ -103,6 +108,24 @@ describe('buildImportedWorktreesCardCandidates', () => { }) }) + it('builds no candidate when the only hidden worktrees are agent scratch', () => { + const candidates = buildImportedWorktreesCardCandidates({ + repos: [repo], + visibleWorktrees: [visibleWorktree], + detectedWorktreesByRepo: { + [repo.id]: detectedResult([ + detectedWorktree({ + id: 'agent-scratch', + path: '/repo/.claude/worktrees/agent-1', + ownership: 'agent-scratch' + }) + ]) + } + }) + + expect(candidates.size).toBe(0) + }) + it('suppresses candidates after show, dismissal, folder repos, or repo filters exclude the repo', () => { const detectedWorktreesByRepo = { [repo.id]: detectedResult([detectedWorktree()]) } diff --git a/src/renderer/src/components/sidebar/project-added-default-checkout.test.ts b/src/renderer/src/components/sidebar/project-added-default-checkout.test.ts index e7c3c061c27..d4a2831f25a 100644 --- a/src/renderer/src/components/sidebar/project-added-default-checkout.test.ts +++ b/src/renderer/src/components/sidebar/project-added-default-checkout.test.ts @@ -1,6 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { getDefaultOnboardingState } from '../../../../shared/constants' -import type { DetectedWorktreeListResult, Worktree } from '../../../../shared/types' +import type { + DetectedWorktree, + DetectedWorktreeListResult, + Worktree +} from '../../../../shared/types' import { finishProjectAddWithDefaultCheckout, getProjectDefaultCheckout, @@ -67,7 +71,7 @@ function makeWorktree(overrides: Partial = {}): Worktree { function makeDetectedLinkedWorktrees( defaultCheckout: Worktree, - options: { linkedVisible?: boolean } = {} + options: { linkedVisible?: boolean; linkedOwnership?: DetectedWorktree['ownership'] } = {} ): DetectedWorktreeListResult { const linkedVisible = options.linkedVisible ?? false return { @@ -89,7 +93,7 @@ function makeDetectedLinkedWorktrees( branch: 'refs/heads/feature', isMainWorktree: false }), - ownership: 'external', + ownership: options.linkedOwnership ?? 'external', selectedCheckout: false, visible: linkedVisible } @@ -299,6 +303,26 @@ describe('finishProjectAddWithDefaultCheckout', () => { expect(mocks.activateAndRevealWorktree).toHaveBeenCalledWith('repo-1::/repo') }) + it('does not flip visibility when the only hidden siblings are agent scratch', async () => { + const defaultCheckout = makeWorktree() + mocks.state.worktreesByRepo = { + 'repo-1': [defaultCheckout] + } + mocks.state.detectedWorktreesByRepo = { + 'repo-1': makeDetectedLinkedWorktrees(defaultCheckout, { linkedOwnership: 'agent-scratch' }) + } + + await openProjectDefaultCheckout({ + repoId: 'repo-1', + source: 'local_folder_picker', + setHideDefaultBranchWorkspace: vi.fn() + }) + + expect(mocks.state.updateRepo).not.toHaveBeenCalled() + expect(mocks.state.fetchWorktrees).not.toHaveBeenCalled() + expect(mocks.activateAndRevealWorktree).toHaveBeenCalledWith('repo-1::/repo') + }) + it('does not repeat linked-worktree reveal after a hidden default checkout refresh', async () => { const defaultCheckout = makeWorktree() mocks.state.detectedWorktreesByRepo = { diff --git a/src/renderer/src/components/sidebar/project-added-default-checkout.ts b/src/renderer/src/components/sidebar/project-added-default-checkout.ts index d8ed9fb4ff9..b30ca0bb24d 100644 --- a/src/renderer/src/components/sidebar/project-added-default-checkout.ts +++ b/src/renderer/src/components/sidebar/project-added-default-checkout.ts @@ -36,7 +36,10 @@ function hasDetectedHiddenLinkedExternalWorktrees( !worktree.isMainWorktree && !worktree.selectedCheckout && !worktree.visible && - worktree.ownership !== 'orca-managed' + worktree.ownership !== 'orca-managed' && + // Why: a repo whose only externals are agent scratch must not get + // flipped to repo-wide 'show' by the add handoff (#9388). + worktree.ownership !== 'agent-scratch' ) } diff --git a/src/shared/agent-scratch-worktrees.test.ts b/src/shared/agent-scratch-worktrees.test.ts new file mode 100644 index 00000000000..b2714bd6464 --- /dev/null +++ b/src/shared/agent-scratch-worktrees.test.ts @@ -0,0 +1,103 @@ +import { describe, expect, it } from 'vitest' +import { + createAgentScratchWorktreePathMatcher, + isAgentScratchWorktreePath +} from './agent-scratch-worktrees' + +describe('isAgentScratchWorktreePath', () => { + const repoPath = '/Users/dev/app' + + it('matches Claude Code sub-agent worktrees', () => { + expect( + isAgentScratchWorktreePath( + repoPath, + '/Users/dev/app/.claude/worktrees/agent-a04ccaaa55ddadb91' + ) + ).toBe(true) + }) + + it('matches gsd parallel-agent workspaces', () => { + expect( + isAgentScratchWorktreePath(repoPath, '/Users/dev/app/.gsd-workspaces/phase-1-subagent-2') + ).toBe(true) + }) + + it('matches scratch worktrees created from a linked checkout', () => { + const matchesAgentScratch = createAgentScratchWorktreePathMatcher([ + repoPath, + '/Users/dev/orca/workspaces/app/feature-x' + ]) + + expect( + matchesAgentScratch( + '/Users/dev/orca/workspaces/app/feature-x/.claude/worktrees/agent-a04ccaaa' + ) + ).toBe(true) + expect(matchesAgentScratch('/Users/dev/other/feature-x/.claude/worktrees/agent-a04ccaaa')).toBe( + false + ) + }) + + it('matches Windows path separators and casing', () => { + expect( + isAgentScratchWorktreePath( + 'C:\\Users\\dev\\app', + 'c:\\USERS\\dev\\app\\.Claude\\Worktrees\\agent-a04ccaaa' + ) + ).toBe(true) + }) + + it('matches WSL UNC paths', () => { + expect( + isAgentScratchWorktreePath( + '//wsl$/Ubuntu/home/dev/app', + '//wsl.localhost/Ubuntu/home/dev/app/.claude/worktrees/agent-a04ccaaa' + ) + ).toBe(true) + }) + + it('preserves case-sensitive POSIX and WSL tool segments', () => { + expect( + isAgentScratchWorktreePath(repoPath, '/Users/dev/app/.Claude/Worktrees/agent-a04ccaaa') + ).toBe(false) + expect( + isAgentScratchWorktreePath( + '//wsl.localhost/Ubuntu/home/dev/app', + '//wsl.localhost/ubuntu/home/dev/app/.Claude/Worktrees/agent-a04ccaaa' + ) + ).toBe(false) + }) + + it('requires the tool directory at the repo root', () => { + expect( + isAgentScratchWorktreePath(repoPath, '/Users/dev/app/.claude/other/worktrees/agent-1') + ).toBe(false) + expect( + isAgentScratchWorktreePath(repoPath, '/Users/dev/app/packages/demo/.claude/worktrees/agent-1') + ).toBe(false) + expect(isAgentScratchWorktreePath(repoPath, '/Users/dev/app/.gsd-workspaces')).toBe(false) + }) + + it('does not match undotted claude directories', () => { + expect(isAgentScratchWorktreePath(repoPath, '/Users/dev/app/claude/worktrees/agent-1')).toBe( + false + ) + }) + + it('does not inherit a scratch classification from the repo parent path', () => { + expect( + isAgentScratchWorktreePath( + '/Users/dev/.claude/worktrees/app', + '/Users/dev/.claude/worktrees/app/manual/feature-x' + ) + ).toBe(false) + }) + + it('does not match user worktree conventions', () => { + expect(isAgentScratchWorktreePath(repoPath, '/Users/dev/app/.worktrees/feature-x')).toBe(false) + expect( + isAgentScratchWorktreePath('/Users/dev/app', '/Users/dev/.superset/worktrees/app/fix-notes') + ).toBe(false) + expect(isAgentScratchWorktreePath('/Users/dev/app', '/orca/workspaces/app/feature')).toBe(false) + }) +}) diff --git a/src/shared/agent-scratch-worktrees.ts b/src/shared/agent-scratch-worktrees.ts new file mode 100644 index 00000000000..2ff216fbd6e --- /dev/null +++ b/src/shared/agent-scratch-worktrees.ts @@ -0,0 +1,39 @@ +import { normalizeRuntimePathForComparison } from './cross-platform-path' + +/** Why: agent CLIs reserve these repo-root paths for scratch; broader matches + * can hide legitimate user worktrees (#9388). */ +const AGENT_SCRATCH_PATH_PREFIXES: readonly (readonly string[])[] = [ + ['.claude', 'worktrees'], + ['.gsd-workspaces'] +] + +export type AgentScratchWorktreePathMatcher = (worktreePath: string) => boolean + +export function createAgentScratchWorktreePathMatcher( + checkoutPaths: readonly string[] +): AgentScratchWorktreePathMatcher { + const checkoutPathKeys = new Set(checkoutPaths.map(normalizeRuntimePathForComparison)) + return (worktreePath) => { + const segments = normalizeRuntimePathForComparison(worktreePath).split('/') + for (const prefix of AGENT_SCRATCH_PATH_PREFIXES) { + for (let index = 0; index + prefix.length < segments.length; index += 1) { + if (!prefix.every((segment, offset) => segments[index + offset] === segment)) { + continue + } + const checkoutPath = segments.slice(0, index).join('/') + // Why: splitting strips the separator from filesystem roots, but normalized checkout keys retain it. + const checkoutPathKey = /^[a-z]:$/i.test(checkoutPath) + ? `${checkoutPath}/` + : checkoutPath || '/' + if (checkoutPathKeys.has(checkoutPathKey)) { + return true + } + } + } + return false + } +} + +export function isAgentScratchWorktreePath(repoPath: string, worktreePath: string): boolean { + return createAgentScratchWorktreePathMatcher([repoPath])(worktreePath) +} diff --git a/src/shared/external-worktree-inbox.test.ts b/src/shared/external-worktree-inbox.test.ts index 1e16c6d5134..34bfadd7f66 100644 --- a/src/shared/external-worktree-inbox.test.ts +++ b/src/shared/external-worktree-inbox.test.ts @@ -10,6 +10,7 @@ import type { import { getHiddenExternalWorktrees, getNewExternalWorktreeInboxWorktrees, + getVisibleExternalWorktrees, mergeExternalWorktreeInboxPaths, shouldOfferNewExternalWorktreeInbox } from './external-worktree-inbox' @@ -162,6 +163,17 @@ describe('external worktree inbox', () => { ).toEqual([hidden]) }) + it('excludes explicitly visible agent scratch from visibility-control counts', () => { + const visible = detectedWorktree({ id: 'visible', visible: true }) + const scratch = detectedWorktree({ + id: 'agent-scratch', + ownership: 'agent-scratch', + visible: true + }) + + expect(getVisibleExternalWorktrees(detectedResult([visible, scratch]))).toEqual([visible]) + }) + it('offers metadata-free nested Orca workspace worktrees through the inbox', () => { const settings = makeSettings() const manual = toDetectedWorktree({ diff --git a/src/shared/external-worktree-inbox.ts b/src/shared/external-worktree-inbox.ts index 13ddd60dd6b..6b6fdaa74aa 100644 --- a/src/shared/external-worktree-inbox.ts +++ b/src/shared/external-worktree-inbox.ts @@ -39,8 +39,28 @@ export function getHiddenExternalWorktrees( return [] } return detected.worktrees.filter( - (worktree) => - !worktree.visible && !worktree.selectedCheckout && worktree.ownership !== 'orca-managed' + (worktree) => !worktree.visible && isUserFacingExternalWorktree(worktree) + ) +} + +export function getVisibleExternalWorktrees( + detected: DetectedWorktreeListResult | undefined +): DetectedWorktree[] { + if (detected?.authoritative !== true) { + return [] + } + return detected.worktrees.filter( + (worktree) => worktree.visible && isUserFacingExternalWorktree(worktree) + ) +} + +function isUserFacingExternalWorktree(worktree: DetectedWorktree): boolean { + // Why: an explicit scratch import may be visible, but agent plumbing must + // stay outside repo-wide discovery and visibility controls (#9388). + return ( + !worktree.selectedCheckout && + worktree.ownership !== 'orca-managed' && + worktree.ownership !== 'agent-scratch' ) } diff --git a/src/shared/types.ts b/src/shared/types.ts index fbb1288de3d..ffdc410e129 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -641,7 +641,7 @@ export type WorktreeMeta = { automationProvenance?: AutomationWorkspaceProvenance } -export type WorktreeOwnership = 'orca-managed' | 'external' | 'unknown-legacy' +export type WorktreeOwnership = 'orca-managed' | 'external' | 'unknown-legacy' | 'agent-scratch' export type DetectedWorktreeListSource = 'git' | 'metadata-fallback' | 'session-fallback' diff --git a/src/shared/worktree-ownership.test.ts b/src/shared/worktree-ownership.test.ts index 0b41859d28b..1697b654d5a 100644 --- a/src/shared/worktree-ownership.test.ts +++ b/src/shared/worktree-ownership.test.ts @@ -1,6 +1,8 @@ import { describe, expect, it } from 'vitest' import type { GlobalSettings, Repo, Worktree, WorktreeMeta } from './types' +import { createAgentScratchWorktreePathMatcher } from './agent-scratch-worktrees' import { + applyMetadataFallbackVisibility, buildKnownOrcaWorkspaceLayouts, classifyWorktreeOwnership, effectiveExternalWorktreeVisibility, @@ -423,3 +425,159 @@ describe('external worktree visibility policy', () => { ).toBe(true) }) }) + +describe('agent scratch worktrees', () => { + const scratchPath = '/repos/app/.claude/worktrees/agent-a04ccaaa55ddadb91' + + it('classifies sub-agent scratch paths as agent-scratch without metadata', () => { + const repo = makeRepo() + const settings = makeSettings() + expect( + classifyWorktreeOwnership({ + repo, + settings, + worktree: makeWorktree({ path: scratchPath, isMainWorktree: false }), + knownOrcaLayouts: buildKnownOrcaWorkspaceLayouts(settings, repo) + }) + ).toBe('agent-scratch') + }) + + it('classifies scratch worktrees created inside another linked checkout', () => { + const repo = makeRepo() + const settings = makeSettings() + const linkedCheckoutPath = '/orca/workspaces/app/feature-x' + expect( + classifyWorktreeOwnership({ + repo, + settings, + worktree: makeWorktree({ + path: `${linkedCheckoutPath}/.claude/worktrees/agent-a04ccaaa`, + isMainWorktree: false + }), + knownOrcaLayouts: buildKnownOrcaWorkspaceLayouts(settings, repo), + agentScratchWorktreePathMatcher: createAgentScratchWorktreePathMatcher([ + repo.path, + linkedCheckoutPath + ]) + }) + ).toBe('agent-scratch') + }) + + it('keeps strong Orca metadata authoritative over the scratch path match', () => { + const repo = makeRepo() + const settings = makeSettings() + expect( + classifyWorktreeOwnership({ + repo, + settings, + worktree: makeWorktree({ path: scratchPath, isMainWorktree: false }), + meta: makeMeta({ orcaCreatedAt: 1 }), + knownOrcaLayouts: buildKnownOrcaWorkspaceLayouts(settings, repo) + }) + ).toBe('orca-managed') + }) + + it('hides agent scratch even when the repo shows non-Orca worktrees', () => { + for (const repo of [ + makeRepo({ externalWorktreeVisibility: 'show' }), + makeRepo({ addedAt: EXTERNAL_WORKTREE_VISIBILITY_ROLLOUT_AT - 1 }) + ]) { + const settings = makeSettings() + const detected = toDetectedWorktree({ + repo, + settings, + worktree: makeWorktree({ path: scratchPath, isMainWorktree: false }), + knownOrcaLayouts: buildKnownOrcaWorkspaceLayouts(settings, repo) + }) + expect(detected.ownership).toBe('agent-scratch') + expect(detected.visible).toBe(false) + } + }) + + it('still shows agent scratch for the selected checkout or an explicit import', () => { + const repo = makeRepo({ + externalWorktreeVisibility: 'hide', + importedExternalWorktreePaths: [scratchPath] + }) + expect( + shouldShowWorktree({ + repo, + worktree: makeWorktree({ path: scratchPath, isMainWorktree: false }), + ownership: 'agent-scratch', + isLegacyRepoForVisibility: false, + isSelectedCheckout: false, + importedExternalWorktreePaths: repo.importedExternalWorktreePaths + }) + ).toBe(true) + expect( + shouldShowWorktree({ + repo: makeRepo(), + worktree: makeWorktree({ path: scratchPath, isMainWorktree: false }), + ownership: 'agent-scratch', + isLegacyRepoForVisibility: false, + isSelectedCheckout: true + }) + ).toBe(true) + }) + + it('keeps agent scratch hidden in the metadata fallback while revealing the rest', () => { + const repo = makeRepo() + const settings = makeSettings() + const layouts = buildKnownOrcaWorkspaceLayouts(settings, repo) + const scratch = toDetectedWorktree({ + repo, + settings, + worktree: makeWorktree({ path: scratchPath, isMainWorktree: false }), + knownOrcaLayouts: layouts + }) + const external = toDetectedWorktree({ + repo, + settings, + worktree: makeWorktree({ path: '/scratch/manual', isMainWorktree: false }), + knownOrcaLayouts: layouts + }) + + expect(applyMetadataFallbackVisibility(scratch)).toMatchObject({ + visible: false, + ownership: 'agent-scratch' + }) + expect(applyMetadataFallbackVisibility(external)).toMatchObject({ + visible: true, + ownership: 'unknown-legacy' + }) + }) + + it('preserves an explicit scratch import in the metadata fallback', () => { + const repo = makeRepo({ + externalWorktreeVisibility: 'hide', + importedExternalWorktreePaths: [scratchPath] + }) + const settings = makeSettings() + const scratch = toDetectedWorktree({ + repo, + settings, + worktree: makeWorktree({ path: scratchPath, isMainWorktree: false }), + knownOrcaLayouts: buildKnownOrcaWorkspaceLayouts(settings, repo) + }) + + expect(scratch).toMatchObject({ ownership: 'agent-scratch', visible: true }) + expect(applyMetadataFallbackVisibility(scratch)).toBe(scratch) + }) + + it('does not classify worktrees from a repo stored below a scratch-looking parent', () => { + const repo = makeRepo({ path: '/repos/.claude/worktrees/app' }) + const settings = makeSettings() + + expect( + classifyWorktreeOwnership({ + repo, + settings, + worktree: makeWorktree({ + path: '/repos/.claude/worktrees/app/manual/feature-x', + isMainWorktree: false + }), + knownOrcaLayouts: buildKnownOrcaWorkspaceLayouts(settings, repo) + }) + ).not.toBe('agent-scratch') + }) +}) diff --git a/src/shared/worktree-ownership.ts b/src/shared/worktree-ownership.ts index 94ba1d908ff..1b04a05282f 100644 --- a/src/shared/worktree-ownership.ts +++ b/src/shared/worktree-ownership.ts @@ -7,6 +7,10 @@ import { resolveRuntimePath } from './cross-platform-path' import { parseWslUncPath } from './wsl-paths' +import { + isAgentScratchWorktreePath, + type AgentScratchWorktreePathMatcher +} from './agent-scratch-worktrees' import { isExplicitlyImportedExternalWorktreePath } from './external-worktree-inbox' import type { DetectedWorktree, @@ -154,11 +158,21 @@ export function classifyWorktreeOwnership(args: { meta?: WorktreeMeta settings: Pick knownOrcaLayouts: OrcaWorkspaceLayout[] + agentScratchWorktreePathMatcher?: AgentScratchWorktreePathMatcher }): WorktreeOwnership { if (hasStrongOrcaMetadata(args.meta)) { return 'orca-managed' } + // Why: sub-agent scratch worktrees (e.g. .claude/worktrees) are tool + // plumbing, not workspaces; classify before layout heuristics (#9388). + if ( + args.agentScratchWorktreePathMatcher?.(args.worktree.path) ?? + isAgentScratchWorktreePath(args.repo.path, args.worktree.path) + ) { + return 'agent-scratch' + } + if (isUnderFlatOrUntrustedOrcaRoot(args.worktree.path, args.knownOrcaLayouts)) { return 'unknown-legacy' } @@ -179,6 +193,7 @@ export function toDetectedWorktree(args: { settings: Pick knownOrcaLayouts: OrcaWorkspaceLayout[] isLegacyRepoForVisibility?: boolean + agentScratchWorktreePathMatcher?: AgentScratchWorktreePathMatcher }): DetectedWorktree { const ownership = classifyWorktreeOwnership(args) const selectedCheckout = areRuntimePathsEqual(args.worktree.path, args.repo.path) @@ -222,12 +237,29 @@ export function shouldShowWorktree(args: { ) { return true } + // Why: agent scratch stays hidden even when the repo shows non-Orca + // worktrees; only an explicit import or selected checkout reveals it. + if (args.ownership === 'agent-scratch') { + return false + } if (args.ownership === 'unknown-legacy' && args.isLegacyRepoForVisibility) { return true } return effectiveExternalWorktreeVisibility(args.repo, args.isLegacyRepoForVisibility) === 'show' } +export function applyMetadataFallbackVisibility(detected: DetectedWorktree): DetectedWorktree { + if (detected.ownership === 'agent-scratch') { + // Why: retain scratch policy, including explicit imports, while ordinary fallback fails open. + return detected + } + return { + ...detected, + visible: true, + ownership: detected.ownership === 'orca-managed' ? 'orca-managed' : 'unknown-legacy' + } +} + export function areRuntimePathsEqual(leftPath: string, rightPath: string): boolean { return ( normalizeRuntimePathForComparison(leftPath) === normalizeRuntimePathForComparison(rightPath)
- Windows? Grab the latest RC — has Windows fixes. -