From 9e2c63ec7c6e265498ed5d311c215682e2eb0fb7 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Wed, 15 Jul 2026 01:40:41 -0700 Subject: [PATCH 01/66] fix(mobile): detect and repair overriding Windows Firewall Block rules for pairing (#8846) * fix(mobile): detect and repair overriding Windows Firewall Block rules for pairing Firewall inspection now reports an overriding inbound Block rule as blocked instead of false success, the UAC repair removes only conflicting rules for the current Orca executable, TCP pairing port, and Private profile before recreating the scoped allow rule, and the notice re-inspects Windows policy after repair instead of optimistically reporting success. Stale focus-triggered inspections can no longer overwrite a newer result during UAC elevation. Fixes #8371 * fix(mobile): inspect the ActiveStore so GPO firewall rules are visible Without -PolicyStore ActiveStore the NetSecurity queries read only the local persistent store, so a GPO-applied Block rule was invisible and the post-repair re-inspection could report a false success on managed hosts. --- .../runtime/windows-mobile-firewall.test.ts | 39 ++++++++ src/main/runtime/windows-mobile-firewall.ts | 52 +++++++--- src/preload/api-types.ts | 1 + .../mobile/WindowsFirewallNotice.test.tsx | 90 ++++++++++++++++- .../mobile/WindowsFirewallNotice.tsx | 98 ++++++++++++++----- src/renderer/src/i18n/locales/en.json | 8 +- src/renderer/src/i18n/locales/es.json | 8 +- src/renderer/src/i18n/locales/ja.json | 8 +- src/renderer/src/i18n/locales/ko.json | 8 +- src/renderer/src/i18n/locales/zh.json | 8 +- src/shared/windows-mobile-firewall.ts | 1 + 11 files changed, 271 insertions(+), 50 deletions(-) diff --git a/src/main/runtime/windows-mobile-firewall.test.ts b/src/main/runtime/windows-mobile-firewall.test.ts index 56464c15bf1..19109a444b8 100644 --- a/src/main/runtime/windows-mobile-firewall.test.ts +++ b/src/main/runtime/windows-mobile-firewall.test.ts @@ -25,6 +25,7 @@ describe('windows mobile firewall', () => { const runPowerShell = vi.fn().mockResolvedValue( JSON.stringify({ matchingRuleScopes: [{ remoteAddresses: ['192.168.0.0/24'] }], + blockingRuleDetected: false, localAddress: '192.168.0.108', localPrefixLength: 24, privateFirewallEnabled: true, @@ -38,22 +39,55 @@ describe('windows mobile firewall', () => { supported: true, port: 6768, ruleAllowed: true, + blockingRuleDetected: false, privateFirewallEnabled: true, networkCategory: 'private', inspectionAvailable: true }) const script = runPowerShell.mock.calls[0]![0] as string + // Why: without ActiveStore, GPO-applied Block rules are invisible and the + // post-repair re-inspection could report a false success on managed hosts. + expect(script).toContain( + "Get-NetFirewallApplicationFilter -PolicyStore ActiveStore -Program 'C:\\Users\\O''Brien\\Orca\\Orca.exe'" + ) + expect(script).toContain('Get-NetFirewallProfile -PolicyStore ActiveStore -Name Private') expect(script).toContain("LocalPort | Where-Object { [string]$_ -eq 'Any'") expect(script).toContain("[string]$_ -eq '6768'") expect(script).toContain("C:\\Users\\O''Brien\\Orca\\Orca.exe") expect(script).toContain("$profile -match 'Private'") expect(script).toContain("Get-NetIPAddress -IPAddress '192.168.0.108'") expect(script).toContain('Get-NetFirewallAddressFilter') + expect(script).toContain("[string]$rule.Action -eq 'Block'") expect(script).toContain('remoteAddresses = @($addressFilter.RemoteAddress') expect(script).toContain('$localPrefixLength = [int]$ip.PrefixLength') }) + it('treats an overlapping inbound Block rule as overriding a matching Allow rule', async () => { + const runPowerShell = vi.fn().mockResolvedValue( + JSON.stringify({ + matchingRuleScopes: [{ remoteAddresses: ['Any'] }], + blockingRuleDetected: true, + localAddress: '192.168.0.108', + localPrefixLength: 24, + privateFirewallEnabled: true, + networkCategory: 'Private' + }) + ) + + await expect( + inspectWindowsMobileFirewall(6768, '192.168.0.108', environment(runPowerShell)) + ).resolves.toEqual({ + supported: true, + port: 6768, + ruleAllowed: false, + blockingRuleDetected: true, + privateFirewallEnabled: true, + networkCategory: 'private', + inspectionAvailable: true + }) + }) + it('does not accept a qualifying rule whose remote-address scope excludes the phone subnet', async () => { const runPowerShell = vi.fn().mockResolvedValue( JSON.stringify({ @@ -100,6 +134,7 @@ describe('windows mobile firewall', () => { supported: true, port: 6768, ruleAllowed: false, + blockingRuleDetected: false, privateFirewallEnabled: true, networkCategory: 'unknown', inspectionAvailable: false @@ -133,6 +168,10 @@ describe('windows mobile firewall', () => { expect(encoded).toBeTruthy() const repairScript = Buffer.from(encoded!, 'base64').toString('utf16le') expect(repairScript).toContain("-Name 'Orca.MobilePairing'") + expect(repairScript).toContain( + "Where-Object { $_.Enabled -eq 'True' -and $_.Direction -eq 'Inbound' -and $_.Action -eq 'Block' }" + ) + expect(repairScript).toContain('$rule | Remove-NetFirewallRule') expect(repairScript).toContain('-Profile Private') expect(repairScript).toContain('-Protocol TCP') expect(repairScript).toContain('-LocalPort 6769') diff --git a/src/main/runtime/windows-mobile-firewall.ts b/src/main/runtime/windows-mobile-firewall.ts index 36797b83eb2..88b885ab5f0 100644 --- a/src/main/runtime/windows-mobile-firewall.ts +++ b/src/main/runtime/windows-mobile-firewall.ts @@ -24,6 +24,7 @@ export type WindowsMobileFirewallEnvironment = { type FirewallInspection = { matchingRuleScopes?: unknown + blockingRuleDetected?: unknown localAddress?: unknown localPrefixLength?: unknown privateFirewallEnabled: boolean @@ -51,14 +52,20 @@ export async function inspectWindowsMobileFirewall( POWERSHELL_TIMEOUT_MS ) const result = JSON.parse(stdout.trim()) as FirewallInspection + // Why: the phone address is unknown before pairing, so any matching Block + // rule must fail this advisory check closed instead of risking false success. + const blockingRuleDetected = result.blockingRuleDetected === true return { supported: true, port, - ruleAllowed: hasSufficientWindowsFirewallRemoteScope( - result.matchingRuleScopes, - result.localAddress, - result.localPrefixLength - ), + ruleAllowed: + !blockingRuleDetected && + hasSufficientWindowsFirewallRemoteScope( + result.matchingRuleScopes, + result.localAddress, + result.localPrefixLength + ), + blockingRuleDetected, privateFirewallEnabled: result.privateFirewallEnabled !== false, networkCategory: parseNetworkCategory(result.networkCategory), inspectionAvailable: true @@ -131,6 +138,7 @@ function unavailableStatus(port: number): WindowsMobileFirewallStatus { supported: true, port, ruleAllowed: false, + blockingRuleDetected: false, privateFirewallEnabled: true, networkCategory: 'unknown', inspectionAvailable: false @@ -166,26 +174,34 @@ try { } catch {}` : '' // Why: NetSecurity filter properties are stable across localized Windows - // display output and keep every rule's address scope independent. + // display output and keep every rule's address scope independent. ActiveStore + // includes GPO-applied rules the default persistent store hides, so managed + // Block rules cannot produce a false success after repair. return `$ErrorActionPreference = 'Stop' $matchingRuleScopes = @() -$rules = @(Get-NetFirewallApplicationFilter -Program ${quotePowerShell(executablePath)} -ErrorAction SilentlyContinue | Get-NetFirewallRule | Where-Object { $_.Enabled -eq 'True' -and $_.Direction -eq 'Inbound' -and $_.Action -eq 'Allow' }) +$blockingRuleDetected = $false +$rules = @(Get-NetFirewallApplicationFilter -PolicyStore ActiveStore -Program ${quotePowerShell(executablePath)} -ErrorAction SilentlyContinue | Get-NetFirewallRule | Where-Object { $_.Enabled -eq 'True' -and $_.Direction -eq 'Inbound' }) foreach ($rule in $rules) { $portFilter = $rule | Get-NetFirewallPortFilter $protocol = [string]$portFilter.Protocol $profile = [string]$rule.Profile $portMatches = @($portFilter.LocalPort | Where-Object { [string]$_ -eq 'Any' -or [string]$_ -eq '${port}' }).Count -gt 0 if (($protocol -eq 'Any' -or $protocol -eq 'TCP' -or $protocol -eq '6') -and ($profile -eq 'Any' -or $profile -match 'Private') -and $portMatches) { - $addressFilter = $rule | Get-NetFirewallAddressFilter - $matchingRuleScopes += [pscustomobject]@{ - remoteAddresses = @($addressFilter.RemoteAddress | ForEach-Object { [string]$_ }) + if ([string]$rule.Action -eq 'Block') { + $blockingRuleDetected = $true + } elseif ([string]$rule.Action -eq 'Allow') { + $addressFilter = $rule | Get-NetFirewallAddressFilter + $matchingRuleScopes += [pscustomobject]@{ + remoteAddresses = @($addressFilter.RemoteAddress | ForEach-Object { [string]$_ }) + } } } } -$privateFirewallEnabled = [bool](Get-NetFirewallProfile -Name Private).Enabled +$privateFirewallEnabled = [bool](Get-NetFirewallProfile -PolicyStore ActiveStore -Name Private).Enabled $networkCategory = 'Unknown'${addressLookup} [pscustomobject]@{ matchingRuleScopes = @($matchingRuleScopes) + blockingRuleDetected = $blockingRuleDetected localAddress = $localAddress localPrefixLength = $localPrefixLength privateFirewallEnabled = $privateFirewallEnabled @@ -194,7 +210,21 @@ $networkCategory = 'Unknown'${addressLookup} } function buildRepairScript(port: number, executablePath: string): string { + // Why: Windows gives explicit Block rules precedence over narrower Allow + // rules, so the user's repair action must remove exact-app conflicts first. + // Removal deliberately ignores the Block rule's remote-address scope, + // mirroring the fail-closed inspection (the phone address is unknown). return `$ErrorActionPreference = 'Stop' +$blockingRules = @(Get-NetFirewallApplicationFilter -Program ${quotePowerShell(executablePath)} -ErrorAction SilentlyContinue | Get-NetFirewallRule | Where-Object { $_.Enabled -eq 'True' -and $_.Direction -eq 'Inbound' -and $_.Action -eq 'Block' }) +foreach ($rule in $blockingRules) { + $portFilter = $rule | Get-NetFirewallPortFilter + $protocol = [string]$portFilter.Protocol + $profile = [string]$rule.Profile + $portMatches = @($portFilter.LocalPort | Where-Object { [string]$_ -eq 'Any' -or [string]$_ -eq '${port}' }).Count -gt 0 + if (($protocol -eq 'Any' -or $protocol -eq 'TCP' -or $protocol -eq '6') -and ($profile -eq 'Any' -or $profile -match 'Private') -and $portMatches) { + $rule | Remove-NetFirewallRule + } +} Get-NetFirewallRule -Name ${quotePowerShell(FIREWALL_RULE_NAME)} -ErrorAction SilentlyContinue | Remove-NetFirewallRule New-NetFirewallRule -Name ${quotePowerShell(FIREWALL_RULE_NAME)} -DisplayName ${quotePowerShell(FIREWALL_RULE_DISPLAY_NAME)} -Description 'Allows Orca Mobile to connect to this Orca desktop on private networks.' -Direction Inbound -Action Allow -Enabled True -Profile Private -Protocol TCP -LocalPort ${port} -Program ${quotePowerShell(executablePath)} -EdgeTraversalPolicy Block | Out-Null` } diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 62ad82b8748..47d9f9d3f99 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -3180,6 +3180,7 @@ export type PreloadApi = { supported: true port: number ruleAllowed: boolean + blockingRuleDetected: boolean privateFirewallEnabled: boolean networkCategory: 'private' | 'public' | 'domain' | 'unknown' inspectionAvailable: boolean diff --git a/src/renderer/src/components/mobile/WindowsFirewallNotice.test.tsx b/src/renderer/src/components/mobile/WindowsFirewallNotice.test.tsx index f28c516df9d..e19b527fc10 100644 --- a/src/renderer/src/components/mobile/WindowsFirewallNotice.test.tsx +++ b/src/renderer/src/components/mobile/WindowsFirewallNotice.test.tsx @@ -2,7 +2,7 @@ import '@testing-library/jest-dom/vitest' import { afterEach, describe, expect, it, vi } from 'vitest' -import { cleanup, render, screen, waitFor } from '@testing-library/react' +import { act, cleanup, render, screen, waitFor } from '@testing-library/react' import userEvent from '@testing-library/user-event' import { WindowsFirewallNotice } from './WindowsFirewallNotice' @@ -28,6 +28,7 @@ describe('WindowsFirewallNotice', () => { supported: true, port: 6768, ruleAllowed: false, + blockingRuleDetected: false, privateFirewallEnabled: true, networkCategory: 'private', inspectionAvailable: true @@ -45,15 +46,28 @@ describe('WindowsFirewallNotice', () => { it('repairs only after explicit user action and hides after success', async () => { const repairWindowsFirewall = vi.fn().mockResolvedValue({ ok: true }) - setMobileApi({ - getWindowsFirewallStatus: vi.fn().mockResolvedValue({ + const getWindowsFirewallStatus = vi + .fn() + .mockResolvedValueOnce({ supported: true, port: 6768, ruleAllowed: false, + blockingRuleDetected: false, privateFirewallEnabled: true, networkCategory: 'private', inspectionAvailable: true - }), + }) + .mockResolvedValueOnce({ + supported: true, + port: 6768, + ruleAllowed: true, + blockingRuleDetected: false, + privateFirewallEnabled: true, + networkCategory: 'private', + inspectionAvailable: true + }) + setMobileApi({ + getWindowsFirewallStatus, repairWindowsFirewall }) const user = userEvent.setup() @@ -61,11 +75,77 @@ describe('WindowsFirewallNotice', () => { await user.click(await screen.findByRole('button', { name: /allow phone connections/i })) expect(repairWindowsFirewall).toHaveBeenCalledTimes(1) + await waitFor(() => expect(getWindowsFirewallStatus).toHaveBeenCalledTimes(2)) await waitFor(() => expect(screen.queryByText(/allow phone connections through/i)).not.toBeInTheDocument() ) }) + it('reports an overriding Block rule and stays actionable if repair cannot clear it', async () => { + const blockedStatus = { + supported: true, + port: 6768, + ruleAllowed: false, + blockingRuleDetected: true, + privateFirewallEnabled: true, + networkCategory: 'private', + inspectionAvailable: true + } + const getWindowsFirewallStatus = vi.fn().mockResolvedValue(blockedStatus) + const repairWindowsFirewall = vi.fn().mockResolvedValue({ ok: true }) + setMobileApi({ getWindowsFirewallStatus, repairWindowsFirewall }) + const user = userEvent.setup() + render() + + expect(await screen.findByText(/Windows may be blocking Orca Mobile/i)).toBeInTheDocument() + expect(screen.getByText(/Block rule can override/i)).toBeInTheDocument() + await user.click(screen.getByRole('button', { name: /repair firewall access/i })) + + await waitFor(() => expect(getWindowsFirewallStatus).toHaveBeenCalledTimes(2)) + expect(screen.getByText(/Windows may be blocking Orca Mobile/i)).toBeInTheDocument() + }) + + it('ignores a stale inspection that resolves after a newer result', async () => { + const blockedStatus = { + supported: true, + port: 6768, + ruleAllowed: false, + blockingRuleDetected: true, + privateFirewallEnabled: true, + networkCategory: 'private', + inspectionAvailable: true + } + const clearedStatus = { ...blockedStatus, ruleAllowed: true, blockingRuleDetected: false } + let resolveStale: (status: typeof blockedStatus) => void = () => {} + const getWindowsFirewallStatus = vi + .fn() + .mockResolvedValueOnce(blockedStatus) + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveStale = resolve + }) + ) + .mockResolvedValueOnce(clearedStatus) + setMobileApi({ getWindowsFirewallStatus }) + render() + expect(await screen.findByText(/Windows may be blocking Orca Mobile/i)).toBeInTheDocument() + + // Why: UAC elevation bounces window focus, so an older in-flight + // inspection can resolve after a newer one and must not win. + window.dispatchEvent(new Event('focus')) + window.dispatchEvent(new Event('focus')) + await waitFor(() => expect(getWindowsFirewallStatus).toHaveBeenCalledTimes(3)) + await waitFor(() => + expect(screen.queryByText(/Windows may be blocking Orca Mobile/i)).not.toBeInTheDocument() + ) + + await act(async () => { + resolveStale(blockedStatus) + }) + expect(screen.queryByText(/Windows may be blocking Orca Mobile/i)).not.toBeInTheDocument() + }) + it('does not offer a firewall rule while the selected network is public', async () => { const openWindowsNetworkSettings = vi.fn().mockResolvedValue(true) setMobileApi({ @@ -73,6 +153,7 @@ describe('WindowsFirewallNotice', () => { supported: true, port: 6768, ruleAllowed: true, + blockingRuleDetected: false, privateFirewallEnabled: false, networkCategory: 'public', inspectionAvailable: true @@ -95,6 +176,7 @@ describe('WindowsFirewallNotice', () => { supported: true, port: 6768, ruleAllowed: false, + blockingRuleDetected: false, privateFirewallEnabled: true, networkCategory: 'domain', inspectionAvailable: true diff --git a/src/renderer/src/components/mobile/WindowsFirewallNotice.tsx b/src/renderer/src/components/mobile/WindowsFirewallNotice.tsx index 9fd06428246..fb9d1827673 100644 --- a/src/renderer/src/components/mobile/WindowsFirewallNotice.tsx +++ b/src/renderer/src/components/mobile/WindowsFirewallNotice.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useState } from 'react' +import { useCallback, useEffect, useRef, useState } from 'react' import { CircleAlert, Loader2, ShieldCheck } from 'lucide-react' import { toast } from 'sonner' import type { WindowsMobileFirewallStatus } from '../../../../shared/windows-mobile-firewall' @@ -21,23 +21,29 @@ export function WindowsFirewallNotice({ const [status, setStatus] = useState(null) const [repairing, setRepairing] = useState(false) const mountedRef = useMountedRef() + const inspectIdRef = useRef(0) - const inspect = useCallback(async () => { + const inspect = useCallback(async (): Promise => { + // Why: UAC elevation steals and returns window focus, so a focus-triggered + // inspection can race the post-repair one; only the latest result may win. + const inspectId = ++inspectIdRef.current if (!pairingReady) { setStatus(null) - return + return null } try { const next = await window.api.mobile.getWindowsFirewallStatus( address ? { address } : undefined ) - if (mountedRef.current) { + if (mountedRef.current && inspectIdRef.current === inspectId) { setStatus(next) } + return next } catch { - if (mountedRef.current) { + if (mountedRef.current && inspectIdRef.current === inspectId) { setStatus(null) } + return null } }, [address, mountedRef, pairingReady]) @@ -52,11 +58,16 @@ export function WindowsFirewallNotice({ } const firewallStatus = status const networkIsPublic = firewallStatus.networkCategory === 'public' + const blockingRuleDetected = firewallStatus.blockingRuleDetected // Why: a Private-profile allow rule cannot help on managed domain networks. if (!pairingReady || firewallStatus.networkCategory === 'domain') { return null } - if (!networkIsPublic && (firewallStatus.ruleAllowed || !firewallStatus.privateFirewallEnabled)) { + if ( + !networkIsPublic && + (!firewallStatus.privateFirewallEnabled || + (firewallStatus.ruleAllowed && !blockingRuleDetected)) + ) { return null } @@ -68,11 +79,32 @@ export function WindowsFirewallNotice({ return } if (result.ok) { - setStatus({ ...firewallStatus, ruleAllowed: true }) - toast.success( + // Why: elevation success only confirms the script ran; managed policy + // can still leave an overriding Block rule in effect. + const next = await inspect() + if (!mountedRef.current) { + return + } + if ( + next?.supported && + (!next.privateFirewallEnabled || + (next.ruleAllowed && !next.blockingRuleDetected && next.inspectionAvailable)) + ) { + toast.success( + translate( + 'auto.components.mobile.WindowsFirewallNotice.repair-success', + 'Windows Firewall now allows Orca Mobile on private networks' + ) + ) + return + } + if (!next) { + setStatus(firewallStatus) + } + toast.error( translate( - 'auto.components.mobile.WindowsFirewallNotice.repair-success', - 'Windows Firewall now allows Orca Mobile on private networks' + 'auto.components.mobile.WindowsFirewallNotice.repair-unverified', + 'Windows Firewall access could not be verified' ) ) return @@ -81,7 +113,7 @@ export function WindowsFirewallNotice({ toast.error( translate( 'auto.components.mobile.WindowsFirewallNotice.repair-failed', - 'Could not add the Windows Firewall rule' + 'Could not update the Windows Firewall rules' ) ) } @@ -90,7 +122,7 @@ export function WindowsFirewallNotice({ toast.error( translate( 'auto.components.mobile.WindowsFirewallNotice.repair-failed', - 'Could not add the Windows Firewall rule' + 'Could not update the Windows Firewall rules' ) ) } @@ -113,10 +145,15 @@ export function WindowsFirewallNotice({ 'auto.components.mobile.WindowsFirewallNotice.public-title', 'Windows marks this network as public' ) - : translate( - 'auto.components.mobile.WindowsFirewallNotice.missing-title', - 'Allow phone connections through Windows Firewall' - )} + : blockingRuleDetected + ? translate( + 'auto.components.mobile.WindowsFirewallNotice.blocked-title', + 'Windows may be blocking Orca Mobile' + ) + : translate( + 'auto.components.mobile.WindowsFirewallNotice.missing-title', + 'Allow phone connections through Windows Firewall' + )}

{networkIsPublic @@ -124,11 +161,17 @@ export function WindowsFirewallNotice({ 'auto.components.mobile.WindowsFirewallNotice.public-description', 'Change this trusted Wi-Fi network to Private before allowing Orca Mobile connections.' ) - : translate( - 'auto.components.mobile.WindowsFirewallNotice.missing-description', - 'Windows may block the pairing server. Add a rule for this Orca app and TCP port {{port}} on Private networks.', - { port: firewallStatus.port } - )} + : blockingRuleDetected + ? translate( + 'auto.components.mobile.WindowsFirewallNotice.blocked-description', + 'An existing inbound Block rule can override the pairing exception. Repair removes conflicting TCP rules for this Orca app, then allows port {{port}} on Private networks.', + { port: firewallStatus.port } + ) + : translate( + 'auto.components.mobile.WindowsFirewallNotice.missing-description', + 'Windows may block the pairing server. Add a rule for this Orca app and TCP port {{port}} on Private networks.', + { port: firewallStatus.port } + )}

{networkIsPublic ? ( @@ -155,10 +198,15 @@ export function WindowsFirewallNotice({ 'auto.components.mobile.WindowsFirewallNotice.waiting', 'Waiting for Windows…' ) - : translate( - 'auto.components.mobile.WindowsFirewallNotice.allow', - 'Allow phone connections' - )} + : blockingRuleDetected + ? translate( + 'auto.components.mobile.WindowsFirewallNotice.repair', + 'Repair firewall access' + ) + : translate( + 'auto.components.mobile.WindowsFirewallNotice.allow', + 'Allow phone connections' + )} )} diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index a9bbe9f92c1..bead7860f98 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -10703,14 +10703,18 @@ }, "WindowsFirewallNotice": { "repair-success": "Windows Firewall now allows Orca Mobile on private networks", - "repair-failed": "Could not add the Windows Firewall rule", + "repair-failed": "Could not update the Windows Firewall rules", "public-title": "Windows marks this network as public", "missing-title": "Allow phone connections through Windows Firewall", "public-description": "Change this trusted Wi-Fi network to Private before allowing Orca Mobile connections.", "missing-description": "Windows may block the pairing server. Add a rule for this Orca app and TCP port {{port}} on Private networks.", "open-settings": "Open network settings", "waiting": "Waiting for Windows…", - "allow": "Allow phone connections" + "allow": "Allow phone connections", + "repair-unverified": "Windows Firewall access could not be verified", + "blocked-title": "Windows may be blocking Orca Mobile", + "blocked-description": "An existing inbound Block rule can override the pairing exception. Repair removes conflicting TCP rules for this Orca app, then allows port {{port}} on Private networks.", + "repair": "Repair firewall access" } }, "gitlab": { diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 9f99aa350f0..17f5d8ed52f 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -10703,14 +10703,18 @@ }, "WindowsFirewallNotice": { "repair-success": "Windows Firewall now allows Orca Mobile on private networks", - "repair-failed": "Could not add the Windows Firewall rule", + "repair-failed": "Could not update the Windows Firewall rules", "public-title": "Windows marks this network as public", "missing-title": "Allow phone connections through Windows Firewall", "public-description": "Change this trusted Wi-Fi network to Private before allowing Orca Mobile connections.", "missing-description": "Windows may block the pairing server. Add a rule for this Orca app and TCP port {{port}} on Private networks.", "open-settings": "Open network settings", "waiting": "Waiting for Windows…", - "allow": "Allow phone connections" + "allow": "Allow phone connections", + "repair-unverified": "Windows Firewall access could not be verified", + "blocked-title": "Windows may be blocking Orca Mobile", + "blocked-description": "An existing inbound Block rule can override the pairing exception. Repair removes conflicting TCP rules for this Orca app, then allows port {{port}} on Private networks.", + "repair": "Repair firewall access" } }, "gitlab": { diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 99bfa5604d4..24ef9ff7829 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -10703,14 +10703,18 @@ }, "WindowsFirewallNotice": { "repair-success": "Windows Firewall now allows Orca Mobile on private networks", - "repair-failed": "Could not add the Windows Firewall rule", + "repair-failed": "Could not update the Windows Firewall rules", "public-title": "Windows marks this network as public", "missing-title": "Allow phone connections through Windows Firewall", "public-description": "Change this trusted Wi-Fi network to Private before allowing Orca Mobile connections.", "missing-description": "Windows may block the pairing server. Add a rule for this Orca app and TCP port {{port}} on Private networks.", "open-settings": "Open network settings", "waiting": "Waiting for Windows…", - "allow": "Allow phone connections" + "allow": "Allow phone connections", + "repair-unverified": "Windows Firewall access could not be verified", + "blocked-title": "Windows may be blocking Orca Mobile", + "blocked-description": "An existing inbound Block rule can override the pairing exception. Repair removes conflicting TCP rules for this Orca app, then allows port {{port}} on Private networks.", + "repair": "Repair firewall access" } }, "gitlab": { diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 0ad69da4c06..28ec3f79a94 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -10703,14 +10703,18 @@ }, "WindowsFirewallNotice": { "repair-success": "Windows Firewall now allows Orca Mobile on private networks", - "repair-failed": "Could not add the Windows Firewall rule", + "repair-failed": "Could not update the Windows Firewall rules", "public-title": "Windows marks this network as public", "missing-title": "Allow phone connections through Windows Firewall", "public-description": "Change this trusted Wi-Fi network to Private before allowing Orca Mobile connections.", "missing-description": "Windows may block the pairing server. Add a rule for this Orca app and TCP port {{port}} on Private networks.", "open-settings": "Open network settings", "waiting": "Waiting for Windows…", - "allow": "Allow phone connections" + "allow": "Allow phone connections", + "repair-unverified": "Windows Firewall access could not be verified", + "blocked-title": "Windows may be blocking Orca Mobile", + "blocked-description": "An existing inbound Block rule can override the pairing exception. Repair removes conflicting TCP rules for this Orca app, then allows port {{port}} on Private networks.", + "repair": "Repair firewall access" } }, "gitlab": { diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 2a72e96cabf..5044f67be66 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -10703,14 +10703,18 @@ }, "WindowsFirewallNotice": { "repair-success": "Windows Firewall now allows Orca Mobile on private networks", - "repair-failed": "Could not add the Windows Firewall rule", + "repair-failed": "Could not update the Windows Firewall rules", "public-title": "Windows marks this network as public", "missing-title": "Allow phone connections through Windows Firewall", "public-description": "Change this trusted Wi-Fi network to Private before allowing Orca Mobile connections.", "missing-description": "Windows may block the pairing server. Add a rule for this Orca app and TCP port {{port}} on Private networks.", "open-settings": "Open network settings", "waiting": "Waiting for Windows…", - "allow": "Allow phone connections" + "allow": "Allow phone connections", + "repair-unverified": "Windows Firewall access could not be verified", + "blocked-title": "Windows may be blocking Orca Mobile", + "blocked-description": "An existing inbound Block rule can override the pairing exception. Repair removes conflicting TCP rules for this Orca app, then allows port {{port}} on Private networks.", + "repair": "Repair firewall access" } }, "gitlab": { diff --git a/src/shared/windows-mobile-firewall.ts b/src/shared/windows-mobile-firewall.ts index f6570031723..62f504fff88 100644 --- a/src/shared/windows-mobile-firewall.ts +++ b/src/shared/windows-mobile-firewall.ts @@ -6,6 +6,7 @@ export type WindowsMobileFirewallStatus = supported: true port: number ruleAllowed: boolean + blockingRuleDetected: boolean privateFirewallEnabled: boolean networkCategory: WindowsNetworkCategory inspectionAvailable: boolean From 0302ae86b8487da81ec0667b174a7cb7bb0dcdac Mon Sep 17 00:00:00 2001 From: s546126 Date: Wed, 15 Jul 2026 01:58:46 -0700 Subject: [PATCH 02/66] feat(ssh): support Kerberos/GSSAPI hosts via the system OpenSSH transport (#7507) * feat(ssh): support Kerberos/GSSAPI hosts via the system OpenSSH transport ssh2 has no gssapi-with-mic support, and adding it would mean forking its protocol layer plus packaging the kerberos native module for three platforms. Instead, route GSSAPI hosts through the existing system-OpenSSH transport, which delegates Kerberos (tickets, SSPI on Windows) to the platform ssh binary. Two tiers, because RHEL-family distros enable GSSAPIAuthentication globally in /etc/ssh/ssh_config and ssh -G therefore reports it for every host: - Targets whose ~/.ssh/config Host block explicitly sets GSSAPIAuthentication yes (imported as target.gssapiAuthentication) try system ssh first, falling through to ssh2 so key auth and credential prompts still work when no ticket is available. - When ssh2 exhausts key/agent auth and the ssh -G-resolved config enables GSSAPI, retry over system ssh before prompting for credentials, so Kerberos-only hosts on distro-default configs connect without a password prompt. Hosts where keys work never leave the ssh2 path. Manual targets flagged for GSSAPI pass -o GSSAPIAuthentication=yes explicitly since they bypass ssh_config. Both tiers work headless (no credential callbacks required). Co-Authored-By: Claude Fable 5 * fix(ssh): harden GSSAPI transport selection (review fixes for PR #7507) Review fixes on top of the Kerberos/GSSAPI feature branch (s546126/kerberos-ssh): - HIGH: reset useSystemSshTransport on the ssh2 fall-through. doSystemSshProbe sets the flag before spawnSystemSshCommand, which throws synchronously when no system ssh binary is on PATH (outside the probe try/catch). The proactive fall-through previously reset only 2 of 3 transport fields, so exec/sftp kept routing through the failed transport - breaking GSSAPI on Windows-with-Git-ssh and headless Linux. - MEDIUM: throw a cancellation error (not the stale ssh2 authError) when a disconnect supersedes the reactive probe mid-flight, and guard connect()'s catch on disposed, so a deliberate disconnect is not overwritten with auth-failed. - MEDIUM: skip the encrypted-key passphrase prompt when the GSSAPI fallback applies, so a Kerberos ticket is tried before prompting; the general prompt still fires if the probe fails. Adds 3 mutation-verified regression tests and hardens two existing tests to assert the probe actually ran. Not connected to any PR remote. Co-authored-by: Orca * fix(ssh): isolate GSSAPI system transport Co-authored-by: Orca --------- Co-authored-by: s546126 <268420947+s546126@users.noreply.github.com> Co-authored-by: Claude Fable 5 Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> Co-authored-by: Jinwoo-H Co-authored-by: Orca --- docs/reference/ssh-gssapi-kerberos.md | 39 +++ src/main/ssh/ssh-config-parser.test.ts | 49 ++++ src/main/ssh/ssh-config-parser.ts | 8 + src/main/ssh/ssh-connection-store.test.ts | 28 +++ src/main/ssh/ssh-connection-store.ts | 1 + src/main/ssh/ssh-connection-utils.test.ts | 44 ++++ src/main/ssh/ssh-connection-utils.ts | 18 ++ src/main/ssh/ssh-connection.test.ts | 230 ++++++++++++++++++ src/main/ssh/ssh-connection.ts | 74 +++++- src/main/ssh/ssh-control-socket.test.ts | 7 + src/main/ssh/ssh-control-socket.ts | 7 +- src/main/ssh/ssh-g-config-resolution.ts | 3 + src/main/ssh/ssh-system-fallback.test.ts | 47 ++++ .../ssh-system-transport.integration.test.ts | 21 +- src/main/ssh/system-ssh-args.ts | 28 ++- src/shared/ssh-types.ts | 4 + 16 files changed, 597 insertions(+), 11 deletions(-) create mode 100644 docs/reference/ssh-gssapi-kerberos.md diff --git a/docs/reference/ssh-gssapi-kerberos.md b/docs/reference/ssh-gssapi-kerberos.md new file mode 100644 index 00000000000..e7594151667 --- /dev/null +++ b/docs/reference/ssh-gssapi-kerberos.md @@ -0,0 +1,39 @@ +# Kerberos / GSSAPI SSH Authentication + +Orca's ssh2-based SSH stack cannot speak `gssapi-with-mic` — the ssh2 library +has no GSSAPI userauth support, and adding it would mean forking ssh2's +protocol layer plus shipping the `kerberos` native module (MIT krb5 / Heimdal / +Windows SSPI) as a prebuilt Electron dependency on three platforms. Instead, +hosts that need Kerberos ride the existing **system OpenSSH transport** — the +same parallel transport already used for `ProxyCommand`/`ProxyJump` hosts — +which delegates GSSAPI, ticket lookup (`kinit` cache, Windows domain logon), +and credential delegation to the platform's own `ssh` binary on macOS, Linux, +and Windows (Win32-OpenSSH uses SSPI). + +## Transport selection + +Two tiers, deliberately asymmetric because RHEL-family distros ship +`GSSAPIAuthentication yes` in the global `/etc/ssh/ssh_config`, which makes +`ssh -G` report GSSAPI enabled for *every* host: + +1. **Proactive** — a target with `gssapiAuthentication: true` (parsed from an + explicit `GSSAPIAuthentication yes` in the host's `~/.ssh/config` block, or + set on the target directly) tries the system-ssh probe first. If that fails + (e.g. no ticket), the connect falls through to the normal ssh2 key/agent + path where passphrase/password prompts remain available — OpenSSH semantics + allow other auth methods alongside GSSAPI. +2. **Auth-failure fallback** — when ssh2 exhausts key/agent auth and the + `ssh -G`-resolved config enables GSSAPI (`isGssapiSystemSshFallbackCandidate` + in `ssh-connection-utils.ts`), the connection retries over system ssh + *before* prompting the user for credentials. Kerberos-only hosts on + distro-default configs connect this way; hosts where keys work never leave + the ssh2 path. + +Manual (non-ssh-config) targets flagged for GSSAPI get an explicit +`-o GSSAPIAuthentication=yes` in `system-ssh-args.ts`; config-backed targets +inherit the option from their own `Host` block since the system binary re-reads +ssh_config. + +Both tiers work headless (`orca serve`): the system-ssh probe needs no +credential callbacks, and GSSAPI itself is non-interactive once a ticket +exists. diff --git a/src/main/ssh/ssh-config-parser.test.ts b/src/main/ssh/ssh-config-parser.test.ts index f550c833a0f..a9eda1f0248 100644 --- a/src/main/ssh/ssh-config-parser.test.ts +++ b/src/main/ssh/ssh-config-parser.test.ts @@ -163,6 +163,40 @@ Host myserver expect(hosts[0].identitiesOnly).toBe(true) }) + it('parses GSSAPIAuthentication', () => { + const config = ` +Host krb-host + HostName krb.example.com + GSSAPIAuthentication yes + +Host plain-host + HostName plain.example.com + GSSAPIAuthentication no + +Host silent-host + HostName silent.example.com +` + const hosts = parseSshConfig(config) + expect(hosts[0].gssapiAuthentication).toBe(true) + expect(hosts[1].gssapiAuthentication).toBe(false) + expect(hosts[2].gssapiAuthentication).toBeUndefined() + }) + + it('keeps the first GSSAPIAuthentication value like OpenSSH', () => { + const hosts = parseSshConfig(` +Host enabled + GSSAPIAuthentication yes + GSSAPIAuthentication no + +Host disabled + GSSAPIAuthentication no + GSSAPIAuthentication yes +`) + + expect(hosts[0].gssapiAuthentication).toBe(true) + expect(hosts[1].gssapiAuthentication).toBe(false) + }) + it('parses ProxyCommand, ProxyUseFdpass, and ProxyJump', () => { const config = ` Host internal @@ -359,6 +393,12 @@ describe('sshConfigHostsToTargets', () => { expect(targets[0].jumpHost).toBe('bastion.example.com') }) + it('carries through gssapiAuthentication', () => { + const hosts = [{ host: 'krb-host', hostname: 'krb.example.com', gssapiAuthentication: true }] + const targets = sshConfigHostsToTargets(hosts, new Set()) + expect(targets[0].gssapiAuthentication).toBe(true) + }) + it('imports duplicate aliases only once and keeps the first concrete host', () => { const hosts = [ { host: 'dup', hostname: 'first.example.com', user: 'first' }, @@ -508,6 +548,15 @@ describe('parseSshGOutput', () => { expect(result.identitiesOnly).toBe(true) }) + it('parses gssapiauthentication', () => { + const output = 'hostname example.com\ngssapiauthentication yes\nport 22' + const result = parseSshGOutput(output) + expect(result.gssapiAuthentication).toBe(true) + + const offResult = parseSshGOutput('hostname example.com\ngssapiauthentication no\nport 22') + expect(offResult.gssapiAuthentication).toBe(false) + }) + it('parses controlmaster options and filters controlpath none', () => { const output = [ 'hostname example.com', diff --git a/src/main/ssh/ssh-config-parser.ts b/src/main/ssh/ssh-config-parser.ts index 9bd02f49eb6..92cec9dac0e 100644 --- a/src/main/ssh/ssh-config-parser.ts +++ b/src/main/ssh/ssh-config-parser.ts @@ -14,6 +14,7 @@ export type SshConfigHost = { identityFile?: string identityAgent?: string identitiesOnly?: boolean + gssapiAuthentication?: boolean proxyCommand?: string proxyUseFdpass?: boolean proxyJump?: string @@ -104,6 +105,12 @@ export function parseSshConfig(content: string): SshConfigHost[] { host.identitiesOnly = value.toLowerCase() === 'yes' } break + case 'gssapiauthentication': + for (const host of current) { + // Why: OpenSSH uses the first obtained value for each parameter. + host.gssapiAuthentication ??= value.toLowerCase() === 'yes' + } + break case 'proxycommand': for (const host of current) { // Why: OpenSSH treats ProxyCommand as a shell snippet and preserves @@ -249,6 +256,7 @@ export function sshConfigHostsToTargets( identityFile: entry.identityFile, identityAgent: entry.identityAgent, identitiesOnly: entry.identitiesOnly, + gssapiAuthentication: entry.gssapiAuthentication, proxyCommand: entry.proxyCommand, jumpHost: entry.proxyJump }) diff --git a/src/main/ssh/ssh-connection-store.test.ts b/src/main/ssh/ssh-connection-store.test.ts index 4bfa7259dab..90d92a1499e 100644 --- a/src/main/ssh/ssh-connection-store.test.ts +++ b/src/main/ssh/ssh-connection-store.test.ts @@ -260,6 +260,34 @@ describe('SshConnectionStore', () => { ) }) + it('refreshes gssapiAuthentication on sync', () => { + mockStore.addSshTarget({ + id: 'ssh-1', + label: 'krb-box', + configHost: 'krb-box', + host: 'krb.example.com', + port: 22, + username: 'dev', + source: 'ssh-config' + }) + loadUserSshConfigMock.mockReturnValue([{ host: 'krb-box' }]) + sshConfigHostsToTargetsMock.mockReturnValue([ + candidate({ + configHost: 'krb-box', + host: 'krb.example.com', + username: 'dev', + gssapiAuthentication: true + }) + ]) + + sshStore.importFromSshConfig() + + expect(mockStore.updateSshTarget).toHaveBeenCalledWith( + 'ssh-1', + expect.objectContaining({ gssapiAuthentication: true }) + ) + }) + it('never overwrites a manual target that owns the alias', () => { mockStore.addSshTarget({ id: 'ssh-m', diff --git a/src/main/ssh/ssh-connection-store.ts b/src/main/ssh/ssh-connection-store.ts index 295e7d57ac3..408b4522afa 100644 --- a/src/main/ssh/ssh-connection-store.ts +++ b/src/main/ssh/ssh-connection-store.ts @@ -184,6 +184,7 @@ export class SshConnectionStore { identityFile: candidate.identityFile, identityAgent: candidate.identityAgent, identitiesOnly: candidate.identitiesOnly, + gssapiAuthentication: candidate.gssapiAuthentication, proxyCommand: candidate.proxyCommand, jumpHost: candidate.jumpHost } diff --git a/src/main/ssh/ssh-connection-utils.test.ts b/src/main/ssh/ssh-connection-utils.test.ts index 0c578cff7d2..50f2eb66ff2 100644 --- a/src/main/ssh/ssh-connection-utils.test.ts +++ b/src/main/ssh/ssh-connection-utils.test.ts @@ -31,6 +31,7 @@ vi.mock('fs', () => ({ import { isTransientError, isSystemSshFallbackError, + isGssapiSystemSshFallbackCandidate, isAuthError, isAgentFallbackError, sleep, @@ -166,6 +167,49 @@ describe('isSystemSshFallbackError', () => { }) }) +// ── isGssapiSystemSshFallbackCandidate ─────────────────────────────── + +describe('isGssapiSystemSshFallbackCandidate', () => { + const authErr = new Error('All configured authentication methods failed') + + it('returns true for auth failures when resolved config enables GSSAPI', () => { + expect(isGssapiSystemSshFallbackCandidate(authErr, {}, { gssapiAuthentication: true })).toBe( + true + ) + }) + + it('returns true for passphrase failures so Kerberos SSO runs before prompting', () => { + const passphraseErr = new Error('Encrypted private OpenSSH key detected, but no passphrase') + expect( + isGssapiSystemSshFallbackCandidate(passphraseErr, {}, { gssapiAuthentication: true }) + ).toBe(true) + }) + + it('returns false when the target already tried system ssh proactively', () => { + expect( + isGssapiSystemSshFallbackCandidate( + authErr, + { gssapiAuthentication: true }, + { gssapiAuthentication: true } + ) + ).toBe(false) + }) + + it('returns false without GSSAPI in the resolved config', () => { + expect(isGssapiSystemSshFallbackCandidate(authErr, {}, { gssapiAuthentication: false })).toBe( + false + ) + expect(isGssapiSystemSshFallbackCandidate(authErr, {}, null)).toBe(false) + }) + + it('returns false for network errors so retry semantics stay unchanged', () => { + const netErr = new Error('connect ETIMEDOUT 1.2.3.4:22') + expect(isGssapiSystemSshFallbackCandidate(netErr, {}, { gssapiAuthentication: true })).toBe( + false + ) + }) +}) + // ── isAuthError ────────────────────────────────────────────────────── describe('isAuthError', () => { diff --git a/src/main/ssh/ssh-connection-utils.ts b/src/main/ssh/ssh-connection-utils.ts index cc8269c1b48..9e496ae0fa0 100644 --- a/src/main/ssh/ssh-connection-utils.ts +++ b/src/main/ssh/ssh-connection-utils.ts @@ -84,6 +84,24 @@ export function isSystemSshFallbackError(err: Error): boolean { return err.message.includes('EHOSTUNREACH') || err.message.includes('ENETUNREACH') } +// Why: ssh2 has no gssapi-with-mic support. When the effective OpenSSH config +// enables GSSAPIAuthentication (often a distro-wide /etc/ssh default), a +// Kerberos ticket can still authenticate through the system ssh binary after +// key/agent auth fails — but only auth-shaped failures qualify, so network +// errors keep their existing retry semantics. +export function isGssapiSystemSshFallbackCandidate( + err: Error, + target: Pick, + resolved: Pick | null +): boolean { + // Why: targets with an explicit per-host flag already tried system ssh + // proactively during this attempt; probing again cannot succeed. + if (target.gssapiAuthentication === true) { + return false + } + return (isAuthError(err) || isPassphraseError(err)) && resolved?.gssapiAuthentication === true +} + export function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)) } diff --git a/src/main/ssh/ssh-connection.test.ts b/src/main/ssh/ssh-connection.test.ts index 005d43c9ca8..d78c2ac15fe 100644 --- a/src/main/ssh/ssh-connection.test.ts +++ b/src/main/ssh/ssh-connection.test.ts @@ -1060,6 +1060,236 @@ describe('SshConnection', () => { expect(conn.usesSystemSshTransport()).toBe(false) }) + it('tries system SSH first for targets that explicitly request GSSAPI authentication', async () => { + const conn = new SshConnection(createTarget({ gssapiAuthentication: true }), createCallbacks()) + + await conn.connect() + await conn.exec('echo after-connect') + + expect(conn.getState().status).toBe('connected') + expect(conn.usesSystemSshTransport()).toBe(true) + expect(clientInstances).toHaveLength(0) + expect(spawnSystemSshCommandMock).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ gssapiAuthentication: true }), + 'echo ORCA-SYSTEM-SSH-OK', + { + gssapiOnly: true, + wrapCommand: false + } + ) + expect(spawnSystemSshCommandMock).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ gssapiAuthentication: true }), + 'echo after-connect', + { gssapiOnly: true } + ) + }) + + it('falls back to ssh2 when the GSSAPI-first system SSH attempt fails', async () => { + spawnSystemSshCommandMock.mockImplementation(() => + createFailingSystemCommandChannel(255, 'Permission denied (gssapi-with-mic,publickey)') + ) + const conn = new SshConnection(createTarget({ gssapiAuthentication: true }), createCallbacks()) + + await conn.connect() + + expect(conn.getState().status).toBe('connected') + expect(conn.usesSystemSshTransport()).toBe(false) + expect(clientInstances).toHaveLength(1) + // Why: proves the GSSAPI-first probe actually ran before the ssh2 fallback, + // so the test fails if the proactive block is removed. + expect(spawnSystemSshCommandMock).toHaveBeenCalledWith( + expect.objectContaining({ gssapiAuthentication: true }), + 'echo ORCA-SYSTEM-SSH-OK', + { + gssapiOnly: true, + wrapCommand: false + } + ) + }) + + it('falls back to system SSH after an ssh2 auth failure when resolved config enables GSSAPI', async () => { + connectBehavior = 'error' + connectErrorMessage = 'All configured authentication methods failed' + vi.mocked(resolveWithSshG).mockResolvedValue( + createResolvedConfig({ proxyUseFdpass: false, gssapiAuthentication: true }) + ) + const onCredentialRequest = vi.fn(async () => 'password-123') + const conn = new SshConnection( + createTarget({ configHost: 'krb-host' }), + createCallbacks({ onCredentialRequest }) + ) + + await conn.connect() + + expect(conn.getState().status).toBe('connected') + expect(conn.usesSystemSshTransport()).toBe(true) + expect(onCredentialRequest).not.toHaveBeenCalled() + }) + + it('connects through the GSSAPI fallback without credential callbacks (headless)', async () => { + connectBehavior = 'error' + connectErrorMessage = 'All configured authentication methods failed' + vi.mocked(resolveWithSshG).mockResolvedValue( + createResolvedConfig({ proxyUseFdpass: false, gssapiAuthentication: true }) + ) + const conn = new SshConnection(createTarget({ configHost: 'krb-host' }), createCallbacks()) + + await conn.connect() + + expect(conn.getState().status).toBe('connected') + expect(conn.usesSystemSshTransport()).toBe(true) + }) + + it('keeps prompting for credentials when the GSSAPI fallback probe fails', async () => { + // Why: identityAgent 'none' makes resolveAgentSocket return undefined on + // every platform (SSH_AUTH_SOCK='' alone leaves the Windows agent pipe), so + // ssh2's first connect carries any default key directly and the agent + // fallback retry never consumes the second connectSequence entry — + // deterministic on dev machines with both ~/.ssh/id_* and a live agent. + vi.stubEnv('SSH_AUTH_SOCK', '') + connectSequence = [new Error('All configured authentication methods failed'), 'ready'] + spawnSystemSshCommandMock.mockImplementation(() => + createFailingSystemCommandChannel(255, 'Permission denied (gssapi-with-mic,password)') + ) + vi.mocked(resolveWithSshG).mockResolvedValue( + createResolvedConfig({ + proxyUseFdpass: false, + gssapiAuthentication: true, + identityAgent: 'none' + }) + ) + const onCredentialRequest = vi.fn(async () => 'password-123') + const conn = new SshConnection( + createTarget({ configHost: 'krb-host' }), + createCallbacks({ onCredentialRequest }) + ) + + await conn.connect() + + expect(conn.getState().status).toBe('connected') + expect(conn.usesSystemSshTransport()).toBe(false) + // Why: proves the reactive GSSAPI probe actually ran before prompting, so + // the test fails if the reactive fallback block is removed. + expect(spawnSystemSshCommandMock).toHaveBeenCalledWith( + expect.objectContaining({ configHost: 'krb-host' }), + 'echo ORCA-SYSTEM-SSH-OK', + expect.objectContaining({ wrapCommand: false }) + ) + expect(onCredentialRequest).toHaveBeenCalledWith('target-1', 'password', expect.any(String)) + }) + + it('tries the GSSAPI probe before prompting for an encrypted key passphrase', async () => { + // Why: a valid Kerberos ticket should connect silently before the user is + // ever asked for the key passphrase. Agent auth fails, the explicit-key + // retry fails with a passphrase error, and resolved GSSAPI is on — so the + // reactive probe must run before onCredentialRequest. + vi.stubEnv('SSH_AUTH_SOCK', '/tmp/agent.sock') + const tempDir = mkdtempSync(join(tmpdir(), 'orca-ssh-key-')) + const keyPath = join(tempDir, 'id_ed25519') + writeFileSync(keyPath, 'test-key') + connectSequence = [ + new Error('All configured authentication methods failed'), + new Error('Encrypted private OpenSSH key detected, but no passphrase given') + ] + vi.mocked(resolveWithSshG).mockResolvedValue( + createResolvedConfig({ proxyUseFdpass: false, gssapiAuthentication: true }) + ) + const order: string[] = [] + spawnSystemSshCommandMock.mockImplementation(() => { + order.push('probe') + return createSystemCommandChannel() + }) + const onCredentialRequest = vi.fn(async () => { + order.push('prompt') + return 'secret' + }) + + try { + const conn = new SshConnection( + createTarget({ configHost: 'krb-host', identityFile: keyPath }), + createCallbacks({ onCredentialRequest }) + ) + + await conn.connect() + + expect(conn.getState().status).toBe('connected') + expect(conn.usesSystemSshTransport()).toBe(true) + // Why: the probe must precede any passphrase prompt (which here never runs). + expect(order[0]).toBe('probe') + expect(onCredentialRequest).not.toHaveBeenCalled() + } finally { + rmSync(tempDir, { recursive: true, force: true }) + } + }) + + it('does not try system SSH for auth failures when resolved config leaves GSSAPI off', async () => { + connectBehavior = 'error' + connectErrorMessage = 'All configured authentication methods failed' + vi.mocked(resolveWithSshG).mockResolvedValue(createResolvedConfig({ proxyUseFdpass: false })) + const conn = new SshConnection(createTarget({ configHost: 'plain-host' }), createCallbacks()) + + await expect(conn.connect()).rejects.toThrow('All configured authentication methods failed') + expect(conn.getState().status).toBe('auth-failed') + expect(spawnSystemSshCommandMock).not.toHaveBeenCalled() + }) + + it('clears system SSH transport when the GSSAPI-first probe throws synchronously', async () => { + // Why: no system ssh binary makes spawnSystemSshCommand throw before the + // probe's try/catch, so the ssh2 fall-through must still reset the flag — + // otherwise exec/sftp keep routing through the unusable system transport. + spawnSystemSshCommandMock.mockImplementation(() => { + throw new Error('No system ssh binary found. Install OpenSSH.') + }) + connectSequence = ['ready'] + const conn = new SshConnection(createTarget({ gssapiAuthentication: true }), createCallbacks()) + + await conn.connect() + + expect(conn.getState().status).toBe('connected') + expect(conn.usesSystemSshTransport()).toBe(false) + expect(clientInstances).toHaveLength(1) + }) + + it('keeps disconnected state when a disconnect cancels the reactive GSSAPI probe', async () => { + connectBehavior = 'error' + connectErrorMessage = 'All configured authentication methods failed' + vi.mocked(resolveWithSshG).mockResolvedValue( + createResolvedConfig({ proxyUseFdpass: false, gssapiAuthentication: true }) + ) + // Why: a probe channel that stays open until close() leaves the reactive + // fallback pending, so we can disconnect mid-probe; disconnect() then calls + // close() (bumping the generation first), which settles the probe as a + // cancellation rather than a probe failure. + let pendingChannel: ReturnType | null = null + spawnSystemSshCommandMock.mockImplementation(() => { + const channel = new EventEmitter() as ReturnType + channel.stdin = { end: vi.fn(), write: vi.fn() } + channel.stderr = new EventEmitter() + channel.close = vi.fn(() => channel.emit('close', null)) + pendingChannel = channel + return channel + }) + const onStateChange = vi.fn() + const conn = new SshConnection( + createTarget({ configHost: 'krb-host' }), + createCallbacks({ onStateChange }) + ) + + const connectPromise = conn.connect() + // Wait until the reactive probe has spawned its (never-closing) channel. + await vi.waitFor(() => expect(pendingChannel).not.toBeNull()) + + await conn.disconnect() + await connectPromise.catch(() => {}) + + expect(conn.getState().status).toBe('disconnected') + const statuses = onStateChange.mock.calls.map((call) => call[1].status) + expect(statuses).not.toContain('auth-failed') + expect(statuses).not.toContain('error') + }) + it('passes the detected host platform to system SSH file operations', async () => { vi.mocked(resolveWithSshG).mockResolvedValueOnce(createResolvedConfig()) const conn = new SshConnection(createTarget({ configHost: 'fdpass-host' }), createCallbacks()) diff --git a/src/main/ssh/ssh-connection.ts b/src/main/ssh/ssh-connection.ts index af74bdc712a..2856d33b2f0 100644 --- a/src/main/ssh/ssh-connection.ts +++ b/src/main/ssh/ssh-connection.ts @@ -27,6 +27,7 @@ import { isAuthError, isAgentFallbackError, isSystemSshFallbackError, + isGssapiSystemSshFallbackCandidate, isPassphraseError, sleep, buildConnectConfig, @@ -73,6 +74,7 @@ export class SshConnection { private systemOperationAbortController = new AbortController() private systemSshResolvedConfig: SshResolvedConfig | null = null private systemSshControlMasterDisabledForSession = false + private systemSshGssapiOnlyForSession = false private useSystemSshTransport = false private state: SshConnectionState private callbacks: SshConnectionCallbacks @@ -491,6 +493,12 @@ export class SshConnection { } catch (err) { lastError = err instanceof Error ? err : new Error(String(err)) + // Why: a concurrent disconnect() already set 'disconnected'; a cancelled + // attempt's late error must not overwrite it with auth-failed/error. + if (this.disposed) { + throw lastError + } + if (isAuthError(lastError) || isPassphraseError(lastError)) { this.setState('auth-failed', lastError.message) throw lastError @@ -525,8 +533,27 @@ export class SshConnection { await this.doSystemSshProbeWithControlMasterRetry(connectGeneration, resolved) return } + // Why: ssh2 has no gssapi-with-mic support, so hosts that explicitly + // request GSSAPIAuthentication try Kerberos SSO via the system OpenSSH + // binary first. Restrict the probe to GSSAPI so missing tickets fall through + // to Orca's existing key and credential-prompt path. + if (this.target.gssapiAuthentication === true) { + try { + await this.doSystemSshProbeWithControlMasterRetry(connectGeneration, resolved, true) + return + } catch (probeErr) { + if (this.disposed || !this.isCurrentConnectAttempt(connectGeneration)) { + throw probeErr + } + } + } + // Why: a synchronous spawn throw (no system ssh binary) bypasses the probe's + // own catch, so the ssh2 fall-through must clear all system-transport state + // itself — otherwise exec/sftp keep routing through the failed transport. this.systemSshResolvedConfig = null this.systemSshControlMasterDisabledForSession = false + this.systemSshGssapiOnlyForSession = false + this.useSystemSshTransport = false const config = buildConnectConfig(this.target, resolved) @@ -566,6 +593,7 @@ export class SshConnection { } catch { this.systemSshResolvedConfig = null this.systemSshControlMasterDisabledForSession = false + this.systemSshGssapiOnlyForSession = false this.useSystemSshTransport = false throw err } @@ -604,7 +632,15 @@ export class SshConnection { throw keyErr } authError = keyErr - if (isPassphraseError(authError) && !this.cachedPassphrase) { + // Why: when the effective config enables GSSAPI, let the reactive + // system-ssh probe (below) try a Kerberos ticket before prompting + // for the key passphrase; the general passphrase prompt still runs + // if that probe fails, since passphrasePromptHandled stays false. + if ( + isPassphraseError(authError) && + !this.cachedPassphrase && + !isGssapiSystemSshFallbackCandidate(authError, this.target, resolved) + ) { passphrasePromptHandled = true const detail = this.target.identityFile || resolved?.identityFile?.[0] || '(unknown)' const val = await this.callbacks.onCredentialRequest?.( @@ -624,6 +660,28 @@ export class SshConnection { } } + // Why: a Kerberos ticket may authenticate where keys did not; try the + // system ssh binary before falling back to interactive prompts. + if (isGssapiSystemSshFallbackCandidate(authError, this.target, resolved)) { + this.proxyProcess?.kill() + this.proxyProcess = null + try { + await this.doSystemSshProbeWithControlMasterRetry(connectGeneration, resolved, true) + return + } catch { + this.systemSshResolvedConfig = null + this.systemSshControlMasterDisabledForSession = false + this.systemSshGssapiOnlyForSession = false + this.useSystemSshTransport = false + } + // Why: if a disconnect/reconnect superseded this attempt mid-probe, throw + // the cancellation error — not the stale ssh2 authError — so connect() + // does not post auth-failed after the target was deliberately disconnected. + if (this.disposed || !this.isCurrentConnectAttempt(connectGeneration)) { + throw this.createCancelledConnectAttemptError() + } + } + if (!this.callbacks.onCredentialRequest) { this.proxyProcess?.kill() this.proxyProcess = null @@ -762,12 +820,15 @@ export class SshConnection { private async doSystemSshProbeWithControlMasterRetry( connectGeneration: number, - resolved: SshResolvedConfig | null + resolved: SshResolvedConfig | null, + gssapiOnly = false ): Promise { this.systemSshResolvedConfig = cloneResolvedConfig(resolved) this.systemSshControlMasterDisabledForSession = false + this.systemSshGssapiOnlyForSession = gssapiOnly const controlPath = getOrcaControlSocketPath(this.target, { - resolvedConfig: this.systemSshResolvedConfig + resolvedConfig: this.systemSshResolvedConfig, + gssapiOnly: this.systemSshGssapiOnlyForSession }) try { await this.doSystemSshProbe(connectGeneration) @@ -937,6 +998,9 @@ export class SshConnection { if (this.systemSshControlMasterDisabledForSession) { options.disableControlMaster = true } + if (this.systemSshGssapiOnlyForSession) { + options.gssapiOnly = true + } return options } @@ -1117,6 +1181,7 @@ export class SshConnection { this.systemSsh = null this.systemSshResolvedConfig = null this.systemSshControlMasterDisabledForSession = false + this.systemSshGssapiOnlyForSession = false this.useSystemSshTransport = false } @@ -1129,6 +1194,7 @@ export class SshConnection { this.systemSsh = null this.systemSshResolvedConfig = null this.systemSshControlMasterDisabledForSession = false + this.systemSshGssapiOnlyForSession = false this.useSystemSshTransport = false this.setState('connecting') try { @@ -1170,6 +1236,7 @@ export class SshConnection { this.useSystemSshTransport = false this.systemSshResolvedConfig = null this.systemSshControlMasterDisabledForSession = false + this.systemSshGssapiOnlyForSession = false this.setState('error', err instanceof Error ? err.message : String(err)) throw err } @@ -1198,6 +1265,7 @@ export class SshConnection { this.systemSsh = null this.systemSshResolvedConfig = null this.systemSshControlMasterDisabledForSession = false + this.systemSshGssapiOnlyForSession = false this.useSystemSshTransport = false this.setState('disconnected') } diff --git a/src/main/ssh/ssh-control-socket.test.ts b/src/main/ssh/ssh-control-socket.test.ts index 98ae45f0d34..c08bfc739e0 100644 --- a/src/main/ssh/ssh-control-socket.test.ts +++ b/src/main/ssh/ssh-control-socket.test.ts @@ -84,6 +84,13 @@ describe.skipIf(process.platform === 'win32')('getControlSocketPath', () => { }) }) + it('uses a separate socket for GSSAPI-only authentication', () => { + const ordinary = getControlSocketPath(createTarget(), createResolved()) + const gssapiOnly = getControlSocketPath(createTarget(), createResolved(), true) + + expect(gssapiOnly).not.toBe(ordinary) + }) + it('changes the path when a config-backed target resolves to a different host', () => { const before = getControlSocketPath(createTarget({ host: '10.0.0.5' }), createResolved()) const after = getControlSocketPath( diff --git a/src/main/ssh/ssh-control-socket.ts b/src/main/ssh/ssh-control-socket.ts index 5cbd0fdec5a..73622ba504e 100644 --- a/src/main/ssh/ssh-control-socket.ts +++ b/src/main/ssh/ssh-control-socket.ts @@ -28,7 +28,8 @@ const CONTROL_SOCKET_PATH_MAX_LENGTH = UNIX_SOCKET_PATH_LIMIT - OPENSSH_CONTROL_ export function getControlSocketPath( target: SshTarget, - resolvedConfig?: SystemSshResolvedConfig | null + resolvedConfig?: SystemSshResolvedConfig | null, + gssapiOnly = false ): string | null { if (process.platform === 'win32') { return null @@ -58,7 +59,9 @@ export function getControlSocketPath( identityAgent: target.identityAgent || '', identitiesOnly: target.identitiesOnly || false }, - resolved: normalizeResolvedConfig(resolvedConfig) + resolved: normalizeResolvedConfig(resolvedConfig), + // Why: a Kerberos-only session must not reuse a master authenticated by a key. + gssapiOnly }) const hash = createHash('sha256').update(key).digest('hex').slice(0, 16) const socketPath = pathJoin(dir, hash) diff --git a/src/main/ssh/ssh-g-config-resolution.ts b/src/main/ssh/ssh-g-config-resolution.ts index c004701e932..29c6d5eaae3 100644 --- a/src/main/ssh/ssh-g-config-resolution.ts +++ b/src/main/ssh/ssh-g-config-resolution.ts @@ -9,6 +9,8 @@ export type SshResolvedConfig = { identityAgent?: string identitiesOnly: boolean forwardAgent: boolean + /** Effective GSSAPIAuthentication, including distro-wide /etc/ssh defaults. */ + gssapiAuthentication?: boolean proxyCommand?: string proxyUseFdpass: boolean proxyJump?: string @@ -106,6 +108,7 @@ function buildSshResolvedConfig( identityAgent, identitiesOnly: map.get('identitiesonly') === 'yes', forwardAgent: map.get('forwardagent') === 'yes', + gssapiAuthentication: map.get('gssapiauthentication') === 'yes', proxyCommand, proxyUseFdpass: map.get('proxyusefdpass') === 'yes', proxyJump, diff --git a/src/main/ssh/ssh-system-fallback.test.ts b/src/main/ssh/ssh-system-fallback.test.ts index 324a0ed6e48..36698e92b2f 100644 --- a/src/main/ssh/ssh-system-fallback.test.ts +++ b/src/main/ssh/ssh-system-fallback.test.ts @@ -291,6 +291,53 @@ describe('spawnSystemSsh', () => { expect(args).toContain('deploy@127.0.0.1') }) + it('requests GSSAPI authentication explicitly for manual targets', () => { + const args = buildSshArgs( + createTarget({ source: 'manual', configHost: 'krb.example.com', gssapiAuthentication: true }) + ) + + expect(args).toContain('GSSAPIAuthentication=yes') + }) + + it('restricts Kerberos probes to non-interactive GSSAPI authentication', () => { + spawnSystemSshCommand( + createTarget({ + configHost: 'krb-host; touch /tmp/not-run', + source: 'ssh-config', + gssapiAuthentication: true + }), + 'echo ready', + { gssapiOnly: true, wrapCommand: false } + ) + + const args = spawnMock.mock.calls[0][1] as string[] + expect(args).toEqual( + expect.arrayContaining([ + '-o', + 'BatchMode=yes', + '-o', + 'GSSAPIAuthentication=yes', + '-o', + 'PreferredAuthentications=gssapi-with-mic' + ]) + ) + expect(args).not.toContain('BatchMode=no') + const standaloneControlIdx = args.indexOf('-S') + expect(standaloneControlIdx).toBeGreaterThan(-1) + expect(args[standaloneControlIdx + 1]).toBe('none') + expect(args.at(-2)).toBe('deploy@krb-host; touch /tmp/not-run') + expect(args.at(-1)).toBe('echo ready') + }) + + it('leaves GSSAPI to the Host block for ssh-config targets', () => { + const args = buildSshArgs( + createTarget({ configHost: 'krb-host', source: 'ssh-config', gssapiAuthentication: true }) + ) + + expect(args).not.toContain('GSSAPIAuthentication=yes') + expect(args).toContain('deploy@krb-host') + }) + it('does not inject Orca ControlMaster flags when ssh config already owns muxing', () => { const args = buildSshArgs(createTarget({ configHost: 'workbox', source: 'ssh-config' }), { resolvedConfig: createResolvedConfig({ diff --git a/src/main/ssh/ssh-system-transport.integration.test.ts b/src/main/ssh/ssh-system-transport.integration.test.ts index ffe5bf9c862..b949dc411af 100644 --- a/src/main/ssh/ssh-system-transport.integration.test.ts +++ b/src/main/ssh/ssh-system-transport.integration.test.ts @@ -32,7 +32,7 @@ function writeFakeSsh(dir: string): string { `#!/bin/sh while [ "$#" -gt 0 ]; do case "$1" in - -o|-p|-i|-J) shift 2 ;; + -o|-p|-i|-J|-S) shift 2 ;; -T) shift ;; --) shift; break ;; -*) shift ;; @@ -221,6 +221,25 @@ describe('system SSH transport integration', () => { 20_000 ) + it.skipIf(process.platform === 'win32')( + 'connects GSSAPI-flagged targets through system ssh without the force override', + async () => { + delete process.env.ORCA_SSH_FORCE_SYSTEM_TRANSPORT + const conn = new SshConnection( + { ...makeTarget(), gssapiAuthentication: true }, + { onStateChange: vi.fn() } + ) + await conn.connect() + try { + expect(conn.usesSystemSshTransport()).toBe(true) + expect(conn.getState().status).toBe('connected') + } finally { + await conn.disconnect() + } + }, + 20_000 + ) + it.skipIf(process.platform === 'win32')( 'uploads a directory through the system ssh stdin/stdout path', async () => { diff --git a/src/main/ssh/system-ssh-args.ts b/src/main/ssh/system-ssh-args.ts index 8e812a0b68b..810b77b122f 100644 --- a/src/main/ssh/system-ssh-args.ts +++ b/src/main/ssh/system-ssh-args.ts @@ -5,21 +5,30 @@ export type SystemSshBuildArgsOptions = { resolvedConfig?: SystemSshResolvedConfig | null disableControlMaster?: boolean suppressOrcaControlMaster?: boolean + gssapiOnly?: boolean } export function buildSshArgs(target: SshTarget, options?: SystemSshBuildArgsOptions): string[] { const args: string[] = [] - args.push('-o', 'BatchMode=no') + args.push('-o', options?.gssapiOnly ? 'BatchMode=yes' : 'BatchMode=no') + if (options?.gssapiOnly) { + // Why: the probe must neither authenticate with a key nor open an OpenSSH + // credential prompt; failure belongs to Orca's existing ssh2 prompt path. + args.push('-o', 'GSSAPIAuthentication=yes') + args.push('-o', 'PreferredAuthentications=gssapi-with-mic') + } // Forward stdin/stdout for relay communication args.push('-T') // Why: ControlMaster multiplexes all SSH exec commands over a single connection, // eliminating the ~9s handshake overhead per command. Without this, each // spawnSystemSshCommand call opens a new TCP connection. - const forceDisableControlMaster = - options?.disableControlMaster === true || target.systemSshConnectionReuse === false const controlPath = getOrcaControlSocketPath(target, options) + const forceDisableControlMaster = + options?.disableControlMaster === true || + target.systemSshConnectionReuse === false || + (options?.gssapiOnly === true && controlPath === null) if (forceDisableControlMaster) { // Why: muxed OpenSSH forwards remain registered on the master after the // client exits. Also honors the per-target compatibility opt-out even if @@ -53,6 +62,12 @@ export function buildSshArgs(target: SshTarget, options?: SystemSshBuildArgsOpti args.push('-o', 'IdentitiesOnly=yes') } + if (!useConfigHost && target.gssapiAuthentication && !options?.gssapiOnly) { + // Why: manual targets bypass ssh_config, so Kerberos auth must be + // requested explicitly; config-backed hosts inherit it from their entry. + args.push('-o', 'GSSAPIAuthentication=yes') + } + if (!useConfigHost && target.jumpHost) { args.push('-J', target.jumpHost) } @@ -75,7 +90,7 @@ export function getOrcaControlSocketPath( if (shouldDisableOrcaControlMaster(target, options)) { return null } - return getControlSocketPath(target, options?.resolvedConfig) + return getControlSocketPath(target, options?.resolvedConfig, options?.gssapiOnly === true) } export function getSystemSshBuildArgsFromOperationOptions( @@ -91,6 +106,9 @@ export function getSystemSshBuildArgsFromOperationOptions( if (options?.suppressOrcaControlMaster === true) { buildArgsOptions.suppressOrcaControlMaster = true } + if (options?.gssapiOnly === true) { + buildArgsOptions.gssapiOnly = true + } return Object.keys(buildArgsOptions).length === 0 ? undefined : buildArgsOptions } @@ -107,7 +125,7 @@ function shouldDisableOrcaControlMaster( options?.suppressOrcaControlMaster === true || target.systemSshConnectionReuse === false || unresolvedConfigBackedTarget || - hasUserConfiguredControlMaster(options?.resolvedConfig) + (hasUserConfiguredControlMaster(options?.resolvedConfig) && options?.gssapiOnly !== true) ) } diff --git a/src/shared/ssh-types.ts b/src/shared/ssh-types.ts index 5794cb5226c..112722b2826 100644 --- a/src/shared/ssh-types.ts +++ b/src/shared/ssh-types.ts @@ -24,6 +24,10 @@ export type SshTarget = { identityAgent?: string /** Whether OpenSSH IdentitiesOnly should limit public-key auth attempts. */ identitiesOnly?: boolean + /** Whether the host's SSH config explicitly requests GSSAPIAuthentication + * (Kerberos). ssh2 has no gssapi-with-mic support, so these targets try the + * system OpenSSH transport first. */ + gssapiAuthentication?: boolean /** ProxyCommand from SSH config, if any. */ proxyCommand?: string /** Jump host (ProxyJump), if any. */ From c12ade54d680009a34cb8946b1639667510ed41c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 15 Jul 2026 02:03:20 -0700 Subject: [PATCH 03/66] fix(mobile): dedupe host cards by pinned public key on re-pair (STA-1840) Re-pairing a desktop that was already paired created a duplicate host card (STA-1840). Pairing now resolves the durable host identity by the desktop's pinned publicKeyB64 and reuses the existing id/name, collapses any already-stored duplicates for that key, clears stale relay overlays on a direct-only re-pair, fails closed on unreadable storage, and closes the host's client on pairing success so a reused id reconnects on the newly-paired endpoint. Mobile only. Full mobile suite (1,719 tests) + typecheck/lint/format pass. --- mobile/app/pair-confirm.tsx | 9 ++ mobile/app/pair-scan.tsx | 9 ++ mobile/src/transport/host-store.test.ts | 87 +++++++++++++++++++ mobile/src/transport/host-store.ts | 60 +++++++++---- .../mobile-relay-host-overlay-store.test.ts | 21 +++++ .../mobile-relay-host-overlay-store.ts | 24 ++++- .../pre-profile-pairing-coordinator.test.ts | 34 +++++++- .../pre-profile-pairing-coordinator.ts | 14 +-- 8 files changed, 234 insertions(+), 24 deletions(-) diff --git a/mobile/app/pair-confirm.tsx b/mobile/app/pair-confirm.tsx index e51f3a11622..d4505b82411 100644 --- a/mobile/app/pair-confirm.tsx +++ b/mobile/app/pair-confirm.tsx @@ -9,6 +9,7 @@ import { type PreProfilePairingAttempt } from '../src/transport/pre-profile-pairing-coordinator' 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' @@ -24,6 +25,7 @@ const PAIRING_OVERALL_TIMEOUT_MS = 25_000 export default function PairConfirmScreen() { const router = useRouter() + const closeHost = useCloseHost() const insets = useSafeAreaInsets() const params = useLocalSearchParams<{ code?: string }>() const [status, setStatus] = useState('awaiting-confirm') @@ -104,6 +106,13 @@ export default function PairConfirmScreen() { if (!mountedRef.current || !attemptIsCurrent) { return } + // Why: re-pairing the same desktop now reuses its existing host id + // (STA-1840 dedup), so a client cached under that id from an earlier + // pairing would keep the stale endpoint/relay. Close it so the + // destination screen opens a fresh client with the newly-paired + // 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() if (!mountedRef.current) { return diff --git a/mobile/app/pair-scan.tsx b/mobile/app/pair-scan.tsx index aab9d86bbf5..6b980ee0be6 100644 --- a/mobile/app/pair-scan.tsx +++ b/mobile/app/pair-scan.tsx @@ -18,6 +18,7 @@ import { type PreProfilePairingAttempt } from '../src/transport/pre-profile-pairing-coordinator' import type { ConnectionLogEntry, PairingOffer } from '../src/transport/types' +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' @@ -43,6 +44,7 @@ function Step({ number, text }: { number: number; text: string }) { export default function PairScanScreen() { const router = useRouter() + const closeHost = useCloseHost() const insets = useSafeAreaInsets() const [permission, requestPermission] = useCameraPermissions() const [status, setStatus] = useState<'scanning' | 'connecting' | 'error'>('scanning') @@ -148,6 +150,13 @@ export default function PairScanScreen() { if (!mountedRef.current || !attemptIsCurrent) { return } + // Why: re-pairing the same desktop now reuses its existing host id + // (STA-1840 dedup), so a client cached under that id from an earlier + // pairing would keep the stale endpoint/relay. Close it so the + // destination screen opens a fresh client with the newly-paired + // 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() if (!mountedRef.current) { return diff --git a/mobile/src/transport/host-store.test.ts b/mobile/src/transport/host-store.test.ts index a1aa3d677a1..17246f57eed 100644 --- a/mobile/src/transport/host-store.test.ts +++ b/mobile/src/transport/host-store.test.ts @@ -38,7 +38,9 @@ import { MobileRelayUpgradeHostRemovedError, removeHost, renameHost, + resolvePairingHostIdentity, resetHostStoreForTests, + saveHost, saveExistingHostRelayUpgrade, updateLastConnected } from './host-store' @@ -85,6 +87,8 @@ describe('host-store list mutations', () => { asyncStorageMock.setItem.mockImplementation(async (key: string, raw: string) => { if (key === HOSTS_STORAGE_KEY) { storedHostsRaw = raw + } else if (key === OVERLAY_STORAGE_KEY) { + storedOverlayRaw = raw } }) secureStoreMock.getItemAsync.mockImplementation(async (key: string) => @@ -92,6 +96,89 @@ describe('host-store list mutations', () => { ) }) + it('resolves an existing host by pinned key with one durable read', async () => { + await expect(resolvePairingHostIdentity(HOST_TWO.publicKeyB64, 'host-new')).resolves.toEqual({ + id: HOST_TWO.id, + name: HOST_TWO.name + }) + expect(asyncStorageMock.getItem).toHaveBeenCalledOnce() + }) + + it('names a new host from the same durable read used for identity lookup', async () => { + await expect(resolvePairingHostIdentity('unpaired-key', 'host-new')).resolves.toEqual({ + id: 'host-new', + name: 'Host 3' + }) + expect(asyncStorageMock.getItem).toHaveBeenCalledOnce() + }) + + it('fails closed when durable host identity storage is unreadable', async () => { + asyncStorageMock.getItem.mockRejectedValueOnce(new Error('storage unavailable')) + + await expect(resolvePairingHostIdentity('key-new', 'host-new')).rejects.toThrow(/unreadable/) + expect(asyncStorageMock.setItem).not.toHaveBeenCalled() + }) + + it('collapses already-persisted duplicates when the desktop is re-paired', async () => { + storedHostsRaw = JSON.stringify([ + HOST_ONE, + { ...HOST_TWO, id: 'host-duplicate', publicKeyB64: HOST_ONE.publicKeyB64 } + ]) + + await saveHost({ + ...HOST_ONE, + endpoint: 'ws://127.0.0.1:3', + deviceToken: 'replacement-token' + }) + + expect(JSON.parse(storedHostsRaw)).toEqual([{ ...HOST_ONE, endpoint: 'ws://127.0.0.1:3' }]) + expect(scheduleCleanupMock).toHaveBeenCalledWith('host-duplicate', expect.any(Function)) + }) + + it('clears stale relay state when an existing host is re-paired direct-only', async () => { + const overlay: MobileRelayHostOverlay = { + v: 2, + hostId: HOST_ONE.id, + endpoints: [ + { id: 'direct-primary', kind: 'lan', url: HOST_ONE.endpoint }, + { + id: 'relay-primary', + kind: 'relay', + url: 'wss://relay-c1.onorca.dev/v1/connect/AbCdEf0123_-xyZ9' + } + ], + relayHostId: 'AbCdEf0123_-xyZ9', + relay: { + v: 1, + directorUrl: 'https://relay.onorca.dev', + cellUrl: 'https://relay-c1.onorca.dev', + assignmentEpoch: 7, + relayHostId: 'AbCdEf0123_-xyZ9', + e2eeFraming: 2 + } + } + storedOverlayRaw = JSON.stringify([overlay]) + + await saveHost({ ...HOST_ONE, deviceToken: 'replacement-token' }) + + expect(JSON.parse(storedOverlayRaw!)).toEqual([]) + expect(secureStoreMock.deleteItemAsync).not.toHaveBeenCalled() + }) + + it('does not touch relay storage when saving a new direct-only host', async () => { + await saveHost({ + id: 'host-new', + name: 'New Host', + endpoint: 'ws://127.0.0.1:3', + publicKeyB64: 'key-new', + deviceToken: 'new-token', + lastConnected: 0 + }) + + expect(asyncStorageMock.getItem).not.toHaveBeenCalledWith(OVERLAY_STORAGE_KEY) + expect(secureStoreMock.deleteItemAsync).not.toHaveBeenCalled() + }) + it('commits the removal when credential cleanup scheduling rejects', async () => { scheduleCleanupMock.mockRejectedValue(new Error('intent storage unavailable')) diff --git a/mobile/src/transport/host-store.ts b/mobile/src/transport/host-store.ts index a17e6f57a1b..ad2d7ebc51a 100644 --- a/mobile/src/transport/host-store.ts +++ b/mobile/src/transport/host-store.ts @@ -15,6 +15,7 @@ import { import { loadMobileRelayHostOverlayState, removeMobileRelayHostOverlay, + removeMobileRelayHostOverlays, saveMobileRelayHostOverlay } from './mobile-relay-host-overlay-store' import { deleteMobileRelayCredentialBundle } from './mobile-relay-credential-bundle' @@ -182,12 +183,18 @@ async function doLoadHosts(): Promise { return out } -async function loadStoredHosts(): Promise { - try { - return parseStoredHosts(await AsyncStorage.getItem(STORAGE_KEY)) ?? [] - } catch { - return [] - } +export async function resolvePairingHostIdentity( + publicKeyB64: string, + newHostId: string +): Promise<{ id: string; name: string }> { + // Why: one durable read both preserves an existing identity and names a new host, + // avoiding duplicate cards and a second serial storage read before connecting. + await hostListMutation + const hosts = await readStoredHostsForMutation() + const match = hosts.find((host) => host.publicKeyB64 === publicKeyB64) + return match + ? { id: match.id, name: match.name } + : { id: newHostId, name: getNextHostNameFromHosts(hosts) } } async function readStoredHostsForMutation(): Promise { @@ -242,18 +249,28 @@ export async function saveExistingHostRelayUpgrade(host: HostProfile): Promise { const validated = HostProfileSchema.parse(host) const stored = toStored(validated) + const duplicateHostIds = new Set() + let updatedExistingHost = false await mutateStoredHosts((hosts) => { const index = hosts.findIndex((h) => h.id === stored.id) + for (const candidate of hosts) { + if (candidate.id !== stored.id && candidate.publicKeyB64 === stored.publicKeyB64) { + duplicateHostIds.add(candidate.id) + } + } if (index >= 0) { - const next = hosts.slice() - next[index] = stored - return next + updatedExistingHost = true + // Why: affected installs may already contain duplicate rows; an authoritative + // save is the safe point to collapse them to the preserved host id. + return hosts + .filter(({ id }) => !duplicateHostIds.has(id)) + .map((candidate) => (candidate.id === stored.id ? stored : candidate)) } if (requireExisting) { // Why: an in-flight relay upgrade must not resurrect a host the user removed. throw new MobileRelayUpgradeHostRemovedError('mobile relay upgrade host was removed') } - return [...hosts, stored] + return [...hosts.filter(({ id }) => !duplicateHostIds.has(id)), stored] }) // Why: write metadata BEFORE the keychain token so a crash between the two // leaves orphaned metadata (which loadHosts skips and removeHost can clean @@ -271,6 +288,23 @@ async function persistHost(host: HostProfile, requireExisting: boolean): Promise relay: validated.relay }) } + const overlayRemovalIds = [...duplicateHostIds] + if (!validated.endpoints && updatedExistingHost) { + overlayRemovalIds.push(stored.id) + } + if (overlayRemovalIds.length > 0) { + // Why: reusing an id for direct-only re-pairing must not retain routing + // metadata from the host's previous transport state. + await removeMobileRelayHostOverlays(overlayRemovalIds) + } + for (const duplicateHostId of duplicateHostIds) { + tokenCache.delete(duplicateHostId) + try { + await scheduleHostCredentialCleanup(duplicateHostId, deleteHostCredentials) + } catch { + // Metadata is already deduplicated; orphan-token recovery is best-effort. + } + } } export async function removeHost(hostId: string): Promise { @@ -311,12 +345,6 @@ export async function renameHost(hostId: string, newName: string): Promise }) } -export async function getNextHostName(): Promise { - await hostListMutation - const hosts = await loadStoredHosts() - return getNextHostNameFromHosts(hosts) -} - export async function updateLastConnected(hostId: string): Promise { try { await mutateStoredHosts((hosts) => { diff --git a/mobile/src/transport/mobile-relay-host-overlay-store.test.ts b/mobile/src/transport/mobile-relay-host-overlay-store.test.ts index 8286fe67f5b..812d61ebb1d 100644 --- a/mobile/src/transport/mobile-relay-host-overlay-store.test.ts +++ b/mobile/src/transport/mobile-relay-host-overlay-store.test.ts @@ -9,6 +9,7 @@ vi.mock('@react-native-async-storage/async-storage', () => ({ default: asyncStor import { loadMobileRelayHostOverlays, + removeMobileRelayHostOverlays, resetMobileRelayHostOverlayStoreForTests, saveMobileRelayHostOverlay } from './mobile-relay-host-overlay-store' @@ -77,4 +78,24 @@ describe('mobile relay host overlay store', () => { await expect(saveMobileRelayHostOverlay(OVERLAY)).rejects.toThrow(/unreadable/) expect(asyncStorage.setItem).not.toHaveBeenCalled() }) + + it('removes requested overlays in one storage write', async () => { + const second = { ...OVERLAY, hostId: 'host-2' } + stored = JSON.stringify([OVERLAY, second]) + + await expect(removeMobileRelayHostOverlays(['host-1', 'host-missing'])).resolves.toBeUndefined() + + expect(JSON.parse(stored!)).toEqual([second]) + expect(asyncStorage.getItem).toHaveBeenCalledOnce() + expect(asyncStorage.setItem).toHaveBeenCalledOnce() + }) + + it('skips the storage write when no requested overlay exists', async () => { + stored = JSON.stringify([OVERLAY]) + + await expect(removeMobileRelayHostOverlays(['host-missing'])).resolves.toBeUndefined() + + expect(asyncStorage.getItem).toHaveBeenCalledOnce() + expect(asyncStorage.setItem).not.toHaveBeenCalled() + }) }) diff --git a/mobile/src/transport/mobile-relay-host-overlay-store.ts b/mobile/src/transport/mobile-relay-host-overlay-store.ts index 4dbb50d9f46..7f1519eb5c9 100644 --- a/mobile/src/transport/mobile-relay-host-overlay-store.ts +++ b/mobile/src/transport/mobile-relay-host-overlay-store.ts @@ -40,7 +40,12 @@ async function mutateOverlays( ): Promise { const mutation = overlayMutation.then(async () => { const current = await readOverlaysForMutation() - await AsyncStorage.setItem(OVERLAY_STORAGE_KEY, JSON.stringify(update(current))) + const next = update(current) + // Why: direct-only saves commonly have no overlay to remove; avoid a full + // AsyncStorage write when cleanup leaves the durable list unchanged. + if (next !== current) { + await AsyncStorage.setItem(OVERLAY_STORAGE_KEY, JSON.stringify(next)) + } }) overlayMutation = mutation.catch(() => {}) return mutation @@ -85,7 +90,22 @@ export async function saveMobileRelayHostOverlay(overlay: MobileRelayHostOverlay } export function removeMobileRelayHostOverlay(hostId: string): Promise { - return mutateOverlays((overlays) => overlays.filter((overlay) => overlay.hostId !== hostId)) + return removeMobileRelayHostOverlays([hostId]) +} + +export function removeMobileRelayHostOverlays(hostIds: readonly string[]): Promise { + const targets = new Set(hostIds) + let removed = false + return mutateOverlays((overlays) => { + const next = overlays.filter((overlay) => { + if (!targets.has(overlay.hostId)) { + return true + } + removed = true + return false + }) + return removed ? next : overlays + }) } /** Test-only: drain the module mutation chain between cases. */ diff --git a/mobile/src/transport/pre-profile-pairing-coordinator.test.ts b/mobile/src/transport/pre-profile-pairing-coordinator.test.ts index 16c833bf4f2..782c02aed50 100644 --- a/mobile/src/transport/pre-profile-pairing-coordinator.test.ts +++ b/mobile/src/transport/pre-profile-pairing-coordinator.test.ts @@ -64,7 +64,10 @@ function dependencies(client: RpcClient, events: string[]) { resolveInviteDirector: vi.fn(async () => { throw new Error('director unavailable') }), - getNextHostName: vi.fn(async () => 'Blue Whale'), + resolveHostIdentity: vi.fn(async (_publicKeyB64: string, hostId: string) => ({ + id: hostId, + name: 'Blue Whale' + })), saveHost: vi.fn(async (_host: HostProfile) => { events.push('save-host') }), @@ -136,6 +139,35 @@ describe('pre-profile pairing coordinator', () => { expect(events).toEqual(['connect', 'save-host']) }) + it('reuses the existing host id and name when re-pairing the same desktop key (no duplicate)', async () => { + // STA-1840: re-pairing a desktop already stored under a different id must + // merge into that card, not mint a new host-${now} and duplicate the row. + const events: string[] = [] + const client = fakeClient([success({ version: '1.0.0' })]) + const deps = dependencies(client, events) + deps.resolveHostIdentity = vi.fn(async (publicKeyB64: string, newHostId: string) => { + expect(publicKeyB64).toBe(directOffer.publicKeyB64) + expect(newHostId).toBe(`host-${now}`) + return { id: 'host-existing', name: 'Studio Mac' } + }) + + const attempt = startPreProfilePairing({ + offer: directOffer, + timeoutMs: 5_000, + dependencies: deps + }) + + await expect(attempt.result).resolves.toEqual({ hostId: 'host-existing' }) + expect(deps.saveHost).toHaveBeenCalledWith({ + id: 'host-existing', + name: 'Studio Mac', + endpoint: directOffer.endpoint, + deviceToken: directOffer.deviceToken, + publicKeyB64: directOffer.publicKeyB64, + lastConnected: now + }) + }) + it('journals before connecting and publishes only after authoritative direct install', async () => { const events: string[] = [] let journal: MobileRelayPairingJournal | null = null diff --git a/mobile/src/transport/pre-profile-pairing-coordinator.ts b/mobile/src/transport/pre-profile-pairing-coordinator.ts index 9d2a3b48ddb..82bd989be03 100644 --- a/mobile/src/transport/pre-profile-pairing-coordinator.ts +++ b/mobile/src/transport/pre-profile-pairing-coordinator.ts @@ -6,7 +6,7 @@ import { type MobileRelayEndpoint } from '../../../src/shared/mobile-relay-credential-contract' import { connect, type ConnectOptions } from './rpc-client' -import { getNextHostName, saveHost } from './host-store' +import { resolvePairingHostIdentity, saveHost } from './host-store' import type { HostProfile, PairingOffer, RpcResponse } from './types' import { createMobileRelayPairingJournal, @@ -39,7 +39,7 @@ type Dependencies = { connectDirect: typeof connect connectRelay: typeof connectMobileRelayForPairing resolveInviteDirector: typeof resolvePairingInviteThroughDirector - getNextHostName: typeof getNextHostName + resolveHostIdentity: typeof resolvePairingHostIdentity saveHost: typeof saveHost saveJournal: typeof saveMobileRelayPairingJournal updateJournal: typeof updateMobileRelayPairingJournal @@ -53,7 +53,7 @@ const defaultDependencies: Dependencies = { connectDirect: connect, connectRelay: connectMobileRelayForPairing, resolveInviteDirector: resolvePairingInviteThroughDirector, - getNextHostName, + resolveHostIdentity: resolvePairingHostIdentity, saveHost, saveJournal: saveMobileRelayPairingJournal, updateJournal: updateMobileRelayPairingJournal, @@ -130,8 +130,12 @@ async function runPairing( isDisposed: () => boolean ): Promise<{ hostId: string }> { const now = dependencies.now() - const hostId = `host-${now}` - const hostName = await dependencies.getNextHostName() + // Why: every pairing artifact must share the preserved host id so re-pairing + // updates one card instead of publishing a second identity (STA-1840). + const { id: hostId, name: hostName } = await dependencies.resolveHostIdentity( + offer.publicKeyB64, + `host-${now}` + ) assertActive(isDisposed) let journal: MobileRelayPairingJournal | null = null if (offer.relay && dependencies.platform !== 'web') { From 190ed7d4ed0e43f1e2e543435d4ee09ab40fb7ca Mon Sep 17 00:00:00 2001 From: Haohan Lin Date: Wed, 15 Jul 2026 17:38:40 +0800 Subject: [PATCH 04/66] fix(ssh): make POSIX relay wrapper survive csh/tcsh login shells (#8709) Reconstruct remote POSIX commands with bounded printf arguments so non-POSIX SSH login shells can forward them without requiring remote base64. Preserve relay and system-SSH stdin, and centralize login-shell flag selection for csh/tcsh compatibility.\n\nValidated against real csh and tcsh OpenSSH targets with built-in and system SSH, including cold relay deployment, stdin upload, PTY I/O, file mutation, and reconnect. --- src/main/ssh/ssh-connection-utils.ts | 40 +++++++- src/main/ssh/ssh-connection.test.ts | 14 ++- src/main/ssh/ssh-login-shell-command.ts | 11 +++ .../ssh/ssh-posix-command-wrapper.test.ts | 94 +++++++++++++++++++ .../ssh/ssh-remote-node-resolution.test.ts | 35 +++++++ src/main/ssh/ssh-remote-node-resolution.ts | 11 +-- src/main/ssh/ssh-system-fallback.test.ts | 11 ++- 7 files changed, 198 insertions(+), 18 deletions(-) create mode 100644 src/main/ssh/ssh-login-shell-command.ts create mode 100644 src/main/ssh/ssh-posix-command-wrapper.test.ts diff --git a/src/main/ssh/ssh-connection-utils.ts b/src/main/ssh/ssh-connection-utils.ts index 9e496ae0fa0..de9e0b6c63f 100644 --- a/src/main/ssh/ssh-connection-utils.ts +++ b/src/main/ssh/ssh-connection-utils.ts @@ -110,10 +110,44 @@ export function shellEscape(s: string): string { return `'${s.replace(/'/g, "'\\''")}'` } +const REMOTE_COMMAND_CHUNK_MAX_BYTES = 1_024 +const REMOTE_COMMAND_PRINTF_ESCAPED_BYTES = new Set([0x21, 0x27, 0x5c]) + +function encodeRemoteCommandForPrintf(command: string): string[] { + const chunks: string[] = [] + let chunk = '' + let chunkBytes = 0 + for (const character of command) { + const codePoint = character.codePointAt(0)! + const isSafePrintableAscii = + codePoint >= 0x20 && codePoint <= 0x7e && !REMOTE_COMMAND_PRINTF_ESCAPED_BYTES.has(codePoint) + const encodedCharacter = + codePoint > 0x7f || isSafePrintableAscii + ? character + : `\\0${codePoint.toString(8).padStart(3, '0')}` + const encodedBytes = codePoint > 0x7f ? Buffer.byteLength(character) : encodedCharacter.length + if (chunkBytes + encodedBytes > REMOTE_COMMAND_CHUNK_MAX_BYTES) { + chunks.push(chunk) + chunk = '' + chunkBytes = 0 + } + chunk += encodedCharacter + chunkBytes += encodedBytes + } + chunks.push(chunk) + return chunks +} + +/** Wrap a POSIX snippet into one line that non-POSIX SSH login shells can forward. */ export function wrapRemoteCommandForPosixShell(command: string): string { - // Why: sshd asks the user's login shell to parse exec commands. Orca emits - // POSIX sh snippets; `exec` avoids leaving that shell around for relay bridges. - return `exec /bin/sh -c ${shellEscape(command)}` + // Why: csh/tcsh split multiline SSH exec strings before /bin/sh sees them. + // POSIX printf rebuilds bounded argument chunks without consuming relay stdin. + const encodedChunks = encodeRemoteCommandForPrintf(command) + const decodeAndRun = + 'decoded=$(printf %b "$@" && printf _) || exit $?; ' + + 'decoded=${decoded%_}; exec /bin/sh -c "$decoded"' + const chunkArguments = encodedChunks.map(shellEscape).join(' ') + return `exec /bin/sh -c ${shellEscape(decodeAndRun)} orca-command ${chunkArguments}` } export type SshExecOptions = { diff --git a/src/main/ssh/ssh-connection.test.ts b/src/main/ssh/ssh-connection.test.ts index d78c2ac15fe..e8c5d71234c 100644 --- a/src/main/ssh/ssh-connection.test.ts +++ b/src/main/ssh/ssh-connection.test.ts @@ -657,15 +657,19 @@ describe('SshConnection', () => { } }) - it('wraps exec commands in /bin/sh so non-POSIX login shells do not parse relay snippets', async () => { + it('wraps exec commands as a single line that csh/tcsh login shells cannot break', async () => { const conn = new SshConnection(createTarget(), createCallbacks()) await conn.connect() - await conn.exec("cd '/tmp' && ('/usr/bin/node' -e 'console.log(1)' || echo MISSING)") + const original = "cd '/tmp' && ('/usr/bin/node' -e 'console.log(1)' || echo MISSING)" + await conn.exec(original) - expect(clientInstances[0].lastExecCommand).toBe( - "exec /bin/sh -c 'cd '\\''/tmp'\\'' && ('\\''/usr/bin/node'\\'' -e '\\''console.log(1)'\\'' || echo MISSING)'" - ) + const wrapped = clientInstances[0].lastExecCommand! + // Why: sshd lets the login shell parse this first, so raw newlines let + // csh/tcsh split the command before /bin/sh receives it (issue #8701). + expect(wrapped).not.toContain('\n') + expect(wrapped).toMatch(/^exec \/bin\/sh -c '.*printf %b .*' orca-command /) + expect(wrapped).not.toContain('base64') }) it('can execute native remote commands without the POSIX shell wrapper', async () => { diff --git a/src/main/ssh/ssh-login-shell-command.ts b/src/main/ssh/ssh-login-shell-command.ts new file mode 100644 index 00000000000..3b44bafbb49 --- /dev/null +++ b/src/main/ssh/ssh-login-shell-command.ts @@ -0,0 +1,11 @@ +import { shellEscape } from './ssh-connection-utils' + +const COMMAND_ONLY_SHELLS = new Set(['sh', 'dash', 'csh', 'tcsh']) + +/** Build a command using the startup mode supported by the configured login shell. */ +export function buildSshLoginShellCommand(shell: string, command: string): string { + const shellName = shell.split('/').at(-1) + // Why: csh/tcsh reject combined -lc, while sh/dash do not need login mode here. + const mode = shellName && COMMAND_ONLY_SHELLS.has(shellName) ? '-c' : '-lc' + return `${shellEscape(shell)} ${mode} ${shellEscape(command)}` +} diff --git a/src/main/ssh/ssh-posix-command-wrapper.test.ts b/src/main/ssh/ssh-posix-command-wrapper.test.ts new file mode 100644 index 00000000000..1497dba4d40 --- /dev/null +++ b/src/main/ssh/ssh-posix-command-wrapper.test.ts @@ -0,0 +1,94 @@ +import { existsSync } from 'node:fs' +import { spawnSync } from 'node:child_process' +import { describe, expect, it } from 'vitest' +import { shellEscape, wrapRemoteCommandForPosixShell } from './ssh-connection-utils' + +describe('wrapRemoteCommandForPosixShell', () => { + it('emits one physical line without requiring a remote decoder binary', () => { + const wrapped = wrapRemoteCommandForPosixShell("printf 'a\\n'\nprintf 'b\\n'") + expect(wrapped).not.toContain('\n') + expect(wrapped).toContain('printf %b "$@"') + expect(wrapped).not.toContain('base64') + }) + + it('does not expand long commands made only of parser-safe ASCII', () => { + const command = `: #${'x'.repeat(7_000)}` + expect(wrapRemoteCommandForPosixShell(command).length - command.length).toBeLessThan(200) + }) + + it('does not octal-expand literal UTF-8 near SSH command-length limits', () => { + const command = `: #${'☃'.repeat(7_000)}` + expect(wrapRemoteCommandForPosixShell(command).length - command.length).toBeLessThan(200) + }) + + it('does not octal-expand quoted shell metacharacters near command-length limits', () => { + const command = `: #${'$"`'.repeat(2_000)}` + expect(wrapRemoteCommandForPosixShell(command).length - command.length).toBeLessThan(200) + }) + + it.skipIf(process.platform === 'win32')( + 'reconstructs multiline UTF-8 and shell metacharacters exactly', + () => { + const expected = "line one\nline 'two' \"$`\\ ! ☃\n" + const command = `printf '%s' ${shellEscape(expected)}` + const result = runThroughShell('/bin/sh', command) + + expect(result.status).toBe(0) + expect(result.stderr).toBe('') + expect(result.stdout).toBe(expected) + } + ) + + it.skipIf(process.platform === 'win32')('keeps stdin free for relay protocol bytes', () => { + const command = 'IFS= read -r frame; printf \'frame:%s\' "$frame"' + const result = runThroughShell('/bin/sh', command, 'rpc-payload\n') + + expect(result.status).toBe(0) + expect(result.stdout).toBe('frame:rpc-payload') + }) + + it.skipIf(process.platform === 'win32' || !existsSync('/bin/tcsh'))( + 'keeps every login-shell word bounded for long metacharacter-heavy commands', + () => { + const expected = '$"`'.repeat(2_000) + const command = `printf '%s' ${shellEscape(expected)}` + const result = runThroughShell('/bin/tcsh', command) + + expect(result.status).toBe(0) + expect(result.stderr).toBe('') + expect(result.stdout).toBe(expected) + } + ) + + for (const shell of [ + '/bin/sh', + '/bin/bash', + '/bin/dash', + '/bin/zsh', + '/bin/ksh', + '/bin/csh', + '/bin/tcsh', + '/usr/bin/fish', + '/usr/local/bin/fish', + '/opt/homebrew/bin/fish' + ]) { + it.skipIf(process.platform === 'win32' || !existsSync(shell))( + `survives the real ${shell} parser`, + () => { + const expected = "first\nsecond '$`\\n\\\\ ! ☃\n" + const command = `printf '%s' ${shellEscape(expected)}` + const result = runThroughShell(shell, command) + expect(result.status).toBe(0) + expect(result.stderr).toBe('') + expect(result.stdout).toBe(expected) + } + ) + } +}) + +function runThroughShell(shell: string, command: string, input?: string) { + return spawnSync(shell, ['-c', wrapRemoteCommandForPosixShell(command)], { + encoding: 'utf8', + input + }) +} diff --git a/src/main/ssh/ssh-remote-node-resolution.test.ts b/src/main/ssh/ssh-remote-node-resolution.test.ts index 84d9b9b48e7..109725e549c 100644 --- a/src/main/ssh/ssh-remote-node-resolution.test.ts +++ b/src/main/ssh/ssh-remote-node-resolution.test.ts @@ -272,6 +272,41 @@ describe('resolveRemoteNodePath', () => { }) }) + it('uses -c (not -lc) for a csh login shell, which rejects combined -lc', async () => { + // Why: csh/tcsh reject `-lc` and mis-handle `-l` here ("Bad : modifier", + // issue #8701), so the login-shell probe must drop to plain `-c`. + execCommandMock + .mockResolvedValueOnce('\n') // path probe: empty + .mockResolvedValueOnce('/bin/csh\n') // $SHELL + .mockResolvedValueOnce('/usr/local/bin/node\n') + .mockResolvedValueOnce('v20.0.0\n') + + await expect(resolveRemoteNodePath(conn)).resolves.toBe('/usr/local/bin/node') + expect(execCommandMock).toHaveBeenNthCalledWith(3, conn, `'/bin/csh' -c 'command -v node'`, { + wrapCommand: false, + timeoutMs: 8_000 + }) + }) + + it('uses -c (not -lc) for a tcsh login shell', async () => { + execCommandMock + .mockResolvedValueOnce('\n') // path probe: empty + .mockResolvedValueOnce('/usr/bin/tcsh\n') // $SHELL + .mockResolvedValueOnce('/usr/local/bin/node\n') + .mockResolvedValueOnce('v20.0.0\n') + + await expect(resolveRemoteNodePath(conn)).resolves.toBe('/usr/local/bin/node') + expect(execCommandMock).toHaveBeenNthCalledWith( + 3, + conn, + `'/usr/bin/tcsh' -c 'command -v node'`, + { + wrapCommand: false, + timeoutMs: 8_000 + } + ) + }) + // ── Failure ─────────────────────────────────────────────────────────── it('throws when both strategies find no usable node', async () => { diff --git a/src/main/ssh/ssh-remote-node-resolution.ts b/src/main/ssh/ssh-remote-node-resolution.ts index 52baa610d3f..01d6a0c7d48 100644 --- a/src/main/ssh/ssh-remote-node-resolution.ts +++ b/src/main/ssh/ssh-remote-node-resolution.ts @@ -9,6 +9,7 @@ import { } from './ssh-remote-node-install-guidance' import { execCommand } from './ssh-relay-deploy-helpers' import { isSshSessionLimitError } from './ssh-session-limit-error' +import { buildSshLoginShellCommand } from './ssh-login-shell-command' // Why: the relay requires Node.js 18+. Version managers like nvm keep every // installed version on disk, so a naive "highest version" glob can hand back @@ -150,7 +151,7 @@ async function tryResolveViaLoginShell( const nodePath = await execCommand( conn, - buildCommandInShell(shell, 'command -v node'), + buildSshLoginShellCommand(shell, 'command -v node'), commandOptions({ wrapCommand: false, timeoutMs: LOGIN_SHELL_PROBE_TIMEOUT_MS }, options) ) const candidate = nodePath.trim().split('\n')[0] @@ -171,14 +172,6 @@ async function tryResolveViaLoginShell( return null } -function buildCommandInShell(shell: string, command: string): string { - const shellName = shell.split('/').at(-1) - // Why: dash and POSIX sh do not require `-l`; when $SHELL falls back to - // /bin/sh, prefer a portable command over login-shell semantics. - const mode = shellName === 'sh' || shellName === 'dash' ? '-c' : '-lc' - return `${shellEscape(shell)} ${mode} ${shellEscape(command)}` -} - // Returns true if `nodePath` runs and reports Node >= MIN_NODE_MAJOR. // Caches nothing — this runs at most a few times per resolution (one per // candidate), and the exec round-trip dominates. diff --git a/src/main/ssh/ssh-system-fallback.test.ts b/src/main/ssh/ssh-system-fallback.test.ts index 36698e92b2f..c7f297be9a6 100644 --- a/src/main/ssh/ssh-system-fallback.test.ts +++ b/src/main/ssh/ssh-system-fallback.test.ts @@ -441,9 +441,18 @@ describe('spawnSystemSsh', () => { it('spawns a remote command through the system ssh target', () => { spawnSystemSshCommand(createTarget({ configHost: 'fdpass-host' }), 'echo hello') + // Why: the remote command stays on one line so csh/tcsh login shells cannot + // split it before /bin/sh receives it. + const args = spawnMock.mock.calls[0][1] as string[] + expect(args).toContain('--') + expect(args).toContain('deploy@fdpass-host') + const wrapped = args.at(-1)! + expect(wrapped).not.toContain('\n') + expect(wrapped).toContain('printf %b "$@"') + expect(wrapped).not.toContain('base64') expect(spawnMock).toHaveBeenCalledWith( SYSTEM_SSH_PATH, - expect.arrayContaining(['--', 'deploy@fdpass-host', "exec /bin/sh -c 'echo hello'"]), + expect.any(Array), expect.objectContaining({ stdio: ['pipe', 'pipe', 'pipe'] }) ) }) From ac6977201c5c995742d56a6dc780d3af1725e7e0 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 15 Jul 2026 19:04:35 +0000 Subject: [PATCH 05/66] release: v1.4.142-rc.7 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index efcbf53ac08..9dcd590eaaf 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "orca", - "version": "1.4.142-rc.6", + "version": "1.4.142-rc.7", "description": "Next-gen IDE for parallel agentic development", "homepage": "https://github.com/stablyai/orca", "author": "stablyai", From c8986ca52e3af9eceb2045b05127945cdc4077c7 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 15 Jul 2026 12:09:30 -0700 Subject: [PATCH 06/66] fix(agent-status): reap finished Claude named agents/teammates from the sidebar roster (#8825) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(agent-status): reap finished Claude named agents/teammates from the sidebar roster #8522 stopped one-shot subagents from squatting as idle rows, but named background agents (Workflow/orchestration/ultracode lanes and agent-teams teammates) still piled up permanent "Idle - " child rows for the rest of the session — the reported regression (11 idle rows under an "Orchestration Messages" pane, all idle 3-6h after finishing). Root cause, confirmed against live hook captures (claude 2.1.210): named agents get teammate-shaped ids (a-) AND now appear in Stop's `background_tasks` as `type: "teammate"` entries whose status stays "running" forever — even after the agent finished. The old code read that shape as a "resumable teammate", so SubagentStop only marked it idle and the fold never reaped it (a present teammate task kept hasTeammateTypedTask true). The rows never left. Fix: the roster now tracks only WORKING children. - SubagentStop removes the child outright (teammate-shaped or not) — it is the reliable finish signal; the teammate task's "running" status is not. - TeammateIdle removes by name as the fallback when a SubagentStop is lost. - A lead Stop's background_tasks still reaps unlisted children: hyphen-free one-shots always, and teammate-shaped rows once a complete inventory shows no teammate-typed task at all. A live named agent whose id never appears is kept only while a teammate-typed task is still present (the done-gate). - Hydration drops persisted idle snapshots so a restart can't re-pile them. Verified live in a dev Electron instance driving a real Claude TUI that spawns four named background agents: pre-fix the pane resolves to done with four persistent "Idle - " rows; post-fix each row disappears the instant its agent finishes and the roster drains to empty (done, zero child rows). Tests: roster + row-lifecycle + hook-listener suites rewritten to the working-only semantics, grounded in the captured 2.1.210 hook stream (126 passing). Typecheck + oxlint clean. * fix(agent-status): prune persisted idle Claude children * fix(agent-status): persist Claude idle-row hydration cleanup * fix(agent-status): avoid ambiguous teammate idle cleanup * fix(agent-status): reconcile replacement children at roster cap --- src/main/agent-hooks/server.test.ts | 56 +++ src/main/agent-hooks/server.ts | 43 +- src/main/claude/hook-settings.ts | 4 +- src/shared/agent-hook-listener.test.ts | 96 ++--- src/shared/agent-hook-listener.ts | 42 +- src/shared/claude-subagent-roster.test.ts | 368 ++++++++---------- src/shared/claude-subagent-roster.ts | 258 +++++------- .../claude-subagent-row-lifecycle.test.ts | 145 +++---- 8 files changed, 489 insertions(+), 523 deletions(-) diff --git a/src/main/agent-hooks/server.test.ts b/src/main/agent-hooks/server.test.ts index 7423be836dd..1239e6dd9eb 100644 --- a/src/main/agent-hooks/server.test.ts +++ b/src/main/agent-hooks/server.test.ts @@ -6046,6 +6046,62 @@ describe('Last-status persistence', () => { } }) + it('drops persisted idle Claude child rows from hydration replay', async () => { + mkdirSync(join(userDataPath, 'agent-hooks'), { recursive: true }) + const receivedAt = recentTs() + writeFileSync( + lastStatusPath(), + JSON.stringify({ + version: 2, + entries: { + [PANE]: { + paneKey: PANE, + tabId: 'tab-1', + worktreeId: 'wt-1', + receivedAt, + stateStartedAt: recentTs(-1000), + payload: { + state: 'done', + prompt: 'finished orchestration', + agentType: 'claude', + subagents: [ + { id: 'aweb-research-8a76b7d7', state: 'idle', startedAt: receivedAt - 5000 }, + { id: 'apr-history-9b87c6e6', state: 'idle', startedAt: receivedAt - 4000 } + ] + } + } + } + }), + 'utf8' + ) + + const server = new AgentHookServer() + await server.start({ env: 'production', userDataPath }) + try { + const listener = vi.fn() + server.setListener(listener) + + expect(listener).toHaveBeenCalledWith( + expect.objectContaining({ + payload: expect.objectContaining({ subagents: undefined }) + }) + ) + expect(server.getStatusSnapshot()).toEqual([ + expect.objectContaining({ + state: 'done', + prompt: 'finished orchestration', + subagents: undefined + }) + ]) + // Why: make the migration one-time; otherwise every launch reparses and + // re-prunes the same persisted idle rows. + const persisted = JSON.parse(readFileSync(lastStatusPath(), 'utf8')) + expect(persisted.entries[PANE].payload.subagents).toBeUndefined() + } finally { + server.stop() + } + }) + it('treats a corrupt file as empty hydration without throwing', async () => { mkdirSync(join(userDataPath, 'agent-hooks'), { recursive: true }) writeFileSync(lastStatusPath(), 'not-json{{', 'utf8') diff --git a/src/main/agent-hooks/server.ts b/src/main/agent-hooks/server.ts index 513735bd679..4011fdabda6 100644 --- a/src/main/agent-hooks/server.ts +++ b/src/main/agent-hooks/server.ts @@ -174,6 +174,24 @@ export function isValidPaneKey(value: unknown): value is string { ) } +function dropHydratedIdleClaudeSubagents( + payload: ParsedAgentStatusPayload +): ParsedAgentStatusPayload { + if ( + payload.agentType !== 'claude' || + !payload.subagents?.some((subagent) => subagent.state === 'idle') + ) { + return payload + } + const workingSubagents = payload.subagents.filter((subagent) => subagent.state === 'working') + // Why: older builds persisted finished Claude children as idle rows. Prune + // them from the replay payload itself so restart cannot resurrect the pile. + return { + ...payload, + subagents: workingSubagents.length > 0 ? workingSubagents : undefined + } +} + function sanitizeHydratedEntry( paneKey: string, rawEntry: unknown @@ -1758,6 +1776,7 @@ export class AgentHookServer { } let hydrated = 0 let dropped = 0 + let prunedLegacyClaudeSubagents = 0 // Why: bound disk growth — drop anything older than HYDRATE_MAX_AGE_MS so // entries from worktrees archived weeks ago do not pile up forever. Use // Date.now() once to keep the cutoff consistent across all entries this @@ -1771,11 +1790,15 @@ export class AgentHookServer { : { ...(rawEntry as Record), paneKey: resolvedPaneKey } const entry = sanitizeHydratedEntry(resolvedPaneKey, rawResolvedEntry) if (entry && entry.receivedAt >= ttlCutoff) { + const hydratedPayload = dropHydratedIdleClaudeSubagents(entry.payload) + if (hydratedPayload !== entry.payload) { + prunedLegacyClaudeSubagents += + (entry.payload.subagents?.length ?? 0) - (hydratedPayload.subagents?.length ?? 0) + entry.payload = hydratedPayload + } this.state.lastStatusByPaneKey.set(resolvedPaneKey, entry) - // Why: the in-memory subagent roster died with the previous process. - // Reseed it from the persisted snapshot so the next teammate-bearing - // Stop (whose task ids never match lifecycle ids) doesn't silently - // drop the replayed child rows. + // Why: preserve only working children across restart. Live activity + // confirms them; a later complete inventory may reap stale seeds. if (entry.payload.subagents) { seedClaudeSubagentRosterFromSnapshots( this.state, @@ -1793,18 +1816,16 @@ export class AgentHookServer { `[agent-hooks] last-status hydrate dropped ${dropped} entries (kept ${hydrated})` ) } - if (hydrated > 0 && dropped === 0) { + if (dropped > 0 || prunedLegacyClaudeSubagents > 0) { + // Why: persist load-time pruning once so legacy idle rows do not consume + // parse/filter work again on every launch. + this.runStatusPersist() + } else if (hydrated > 0) { // Why: prime from the raw on-disk bytes (not a re-serialization) so the // dedup is robust against future shape drift in serializeStatusFile. // Only prime when hydration was lossless — if entries were dropped // during sanitize, the in-memory map diverges from the on-disk bytes. this.lastWrittenJson = raw - } else if (dropped > 0) { - // Why: clean the stale on-disk file now so a user who has not run an - // agent in 8+ days does not re-drop the same entries on every cold - // boot. Synchronous variant is safe at start time and avoids - // unref'd-timer-during-quit edge cases. - this.runStatusPersist() } } diff --git a/src/main/claude/hook-settings.ts b/src/main/claude/hook-settings.ts index 0c7012b383a..8392d07f1b9 100644 --- a/src/main/claude/hook-settings.ts +++ b/src/main/claude/hook-settings.ts @@ -34,8 +34,8 @@ export const CLAUDE_EVENTS = [ { eventName: 'StopFailure', definition: { hooks: [{ type: 'command', command: '' }] } }, // Why: subagent/teammate lifecycle feeds the sidebar's child rows and keeps // a pane 'working' while background children outlive the lead's turn. - // TeammateIdle is required because idle-but-alive teammates still report - // status "running" in Stop's background_tasks. + // TeammateIdle retires the working-only row when SubagentStop is lost; + // idle teammates still report status "running" in Stop's background_tasks. // Older Claude builds ignore unregistered event names (StopFailure precedent). { eventName: 'SubagentStart', definition: { hooks: [{ type: 'command', command: '' }] } }, { eventName: 'SubagentStop', definition: { hooks: [{ type: 'command', command: '' }] } }, diff --git a/src/shared/agent-hook-listener.test.ts b/src/shared/agent-hook-listener.test.ts index efafcaa5b06..e9f9e265f62 100644 --- a/src/shared/agent-hook-listener.test.ts +++ b/src/shared/agent-hook-listener.test.ts @@ -2350,92 +2350,83 @@ describe('shared agent-hook-listener', () => { expect(stopped?.payload.state).toBe('done') }) - it('resolves teams-mode teammates to done despite background_tasks reporting running', () => { - // Why: this is the interactive agent-teams shape observed live — + it('removes a finished teammate/named agent on SubagentStop despite its task reading running', () => { + // Why: the interactive agent-teams / orchestration shape observed live — // lifecycle events use `a-` agent ids while background_tasks - // uses unrelated task ids and keeps idle-but-alive teammates as - // status "running". The unmatched task entry must neither create a - // duplicate child row nor keep the pane spinning forever. + // uses unrelated `type: "teammate"` task ids that report "running" + // forever, even after the named agent finished. The finished row must + // leave the sidebar at once (the reported "long idle list" symptom). claudeEvent({ hook_event_name: 'UserPromptSubmit', prompt: 'spawn probe' }) claudeEvent({ hook_event_name: 'SubagentStart', agent_id: 'aprobe1-6d3cb5b52120b7bf', agent_type: 'probe1' }) + const teammateTask = { + id: 'tlkjjs0jv', + type: 'teammate', + status: 'running', + description: 'Run the shell command: sleep 25.' + } const spawnStop = claudeEvent({ hook_event_name: 'Stop', - background_tasks: [ - { - id: 'tlkjjs0jv', - type: 'teammate', - status: 'running', - description: 'Run the shell command: sleep 25.' - } - ] + background_tasks: [teammateTask] }) expect(spawnStop?.payload.state).toBe('working') expect(spawnStop?.payload.subagents).toEqual([ expect.objectContaining({ id: 'aprobe1-6d3cb5b52120b7bf', state: 'working' }) ]) - claudeEvent({ + // SubagentStop is the reliable finish signal — the row goes even though + // its teammate task is still listed "running". + const stopped = claudeEvent({ hook_event_name: 'SubagentStop', agent_id: 'aprobe1-6d3cb5b52120b7bf', - agent_type: 'probe1' + agent_type: 'probe1', + background_tasks: [teammateTask] }) - const idled = claudeEvent({ + expect(stopped?.payload.subagents).toBeUndefined() + + claudeEvent({ hook_event_name: 'TeammateIdle', teammate_name: 'probe1', team_name: 'session-56c87269' }) - expect(idled?.payload.subagents).toEqual([ - expect.objectContaining({ id: 'aprobe1-6d3cb5b52120b7bf', state: 'idle' }) - ]) const wakeStop = claudeEvent({ hook_event_name: 'Stop', - background_tasks: [ - { - id: 'tlkjjs0jv', - type: 'teammate', - status: 'running', - description: 'Run the shell command: sleep 25.' - } - ] + background_tasks: [teammateTask] }) expect(wakeStop?.payload.state).toBe('done') - expect(wakeStop?.payload.subagents).toEqual([ - expect.objectContaining({ id: 'aprobe1-6d3cb5b52120b7bf', state: 'idle' }) - ]) + expect(wakeStop?.payload.subagents).toBeUndefined() }) - it('keeps a teammate whose name differs from its configured agent type', () => { + it('removes a working teammate via TeammateIdle when its id prefix matches the name', () => { claudeEvent({ hook_event_name: 'UserPromptSubmit', prompt: 'spawn reviewer' }) claudeEvent({ hook_event_name: 'SubagentStart', agent_id: 'areviewer-6d3cb5b52120b7bf', agent_type: 'security-reviewer' }) + // Lead turn ends while the teammate works; pane stays working. + const stop = claudeEvent({ + hook_event_name: 'Stop', + background_tasks: [{ id: 'trev', type: 'teammate', status: 'running' }] + }) + expect(stop?.payload.state).toBe('working') // Why: teammate name and agent type are separate Agent-tool inputs; the // lifecycle id embeds the former while the hook reports the latter. - claudeEvent({ - hook_event_name: 'SubagentStop', - agent_id: 'areviewer-6d3cb5b52120b7bf', - agent_type: 'security-reviewer' - }) + // TeammateIdle keyed by name reaps it via the id prefix (fallback when + // its SubagentStop was lost), so the finished row leaves and the pane + // can settle back to the lead's done state. const idled = claudeEvent({ hook_event_name: 'TeammateIdle', teammate_name: 'reviewer', team_name: 'session-x' }) - expect(idled?.payload.subagents).toEqual([ - expect.objectContaining({ - id: 'areviewer-6d3cb5b52120b7bf', - agentType: 'security-reviewer', - state: 'idle' - }) - ]) + expect(idled?.payload.subagents).toBeUndefined() + expect(idled?.payload.state).toBe('done') }) it('scopes subagent rosters per pane', () => { @@ -2679,9 +2670,8 @@ describe('shared agent-hook-listener', () => { teammate_name: 'lane-hooks', team_name: 'session-x' }) - expect(idled?.payload.subagents).toEqual([ - expect.objectContaining({ id: 'alane-hooks-6d3cb5b5', state: 'idle' }) - ]) + // Why: idle means finished — the exact-name match reaps the row. + expect(idled?.payload.subagents).toBeUndefined() }) it('keeps an inferred interrupt terminal across later child lifecycle events', () => { @@ -2703,7 +2693,11 @@ describe('shared agent-hook-listener', () => { expect(idled?.payload.interrupted).toBe(true) }) - it('seeds the roster from persisted snapshots so a teammate-bearing Stop keeps child rows', () => { + it('does not resurrect persisted idle child rows after a restart', () => { + // Why: the roster tracks only working children now. A persisted idle + // snapshot (from a build that kept idle rows) is a finished child, so + // hydration must drop it — otherwise restart would re-pile the exact + // squatting rows this fix removes. seedClaudeSubagentRosterFromSnapshots(state, PANE_KEY, [ { id: 'aprobe2-6d3cb5b52120b7bf', @@ -2720,13 +2714,7 @@ describe('shared agent-hook-listener', () => { ] }) expect(stop?.payload.state).toBe('done') - expect(stop?.payload.subagents).toEqual([ - expect.objectContaining({ - id: 'aprobe2-6d3cb5b52120b7bf', - agentType: 'security-reviewer', - state: 'idle' - }) - ]) + expect(stop?.payload.subagents).toBeUndefined() }) it('rebuilds a running one-shot subagent from background_tasks after restart', () => { diff --git a/src/shared/agent-hook-listener.ts b/src/shared/agent-hook-listener.ts index 5870fd5f43b..bde6baf4431 100644 --- a/src/shared/agent-hook-listener.ts +++ b/src/shared/agent-hook-listener.ts @@ -40,9 +40,8 @@ import { claudeTeammateIdMatchesName, finishClaudeSubagent, foldClaudeBackgroundTasksIntoRoster, - isClaudeTeammateLifecycleId, - markClaudeTeammateIdleByName, readClaudeBackgroundAgentTasks, + removeClaudeTeammateByName, upsertWorkingClaudeSubagent, type ClaudeSubagentRoster } from './claude-subagent-roster' @@ -2436,7 +2435,10 @@ function normalizeClaudeSubagentLifecycleEvent( if (!teammateName) { return null } - markClaudeTeammateIdleByName(roster, teammateName) + // Why: idle means not working, and only working children keep a row. This + // is the fallback finish signal for a named agent whose SubagentStop was + // lost — its background_tasks entry never stops reading "running". + removeClaudeTeammateByName(roster, teammateName) clearClaudePendingWaitForAgent(state, paneKey, (waitingAgentId) => claudeTeammateIdMatchesName(waitingAgentId, teammateName) ) @@ -2453,14 +2455,11 @@ function normalizeClaudeSubagentLifecycleEvent( Date.now() ) } else { - // Why: SubagentStop carries the session's task inventory; a stopping - // agent listed id-exact as a subagent task is a workflow/named one-shot - // (teammate lifecycle ids never appear there), not a resumable teammate. - const stopTasks = readClaudeBackgroundAgentTasks(hookPayload) - finishClaudeSubagent(roster, agentId, { - listedAsSubagentTask: - stopTasks.present && stopTasks.tasks.some((task) => !task.teammate && task.id === agentId) - }) + // Why: a finished child (one-shot, workflow lane, or named teammate) + // leaves the sidebar at once. SubagentStop is the reliable finish + // signal even for teammate-shaped ids — their background_tasks entries + // stay "running" forever — and a resumed teammate re-earns its row. + finishClaudeSubagent(roster, agentId) // Why: a blocked child that dies (killed, errored) without another tool // event would otherwise pin its permission/question wait on the pane // forever — nothing else references that agent again. @@ -2478,11 +2477,9 @@ export function markClaudeLeadTurnInterrupted(state: HookListenerState, paneKey: state.claudeLeadStateByPaneKey.set(paneKey, { state: 'done', interrupted: true }) } -/** Rebuild a pane's roster from a persisted status snapshot during hydration. - * Restart loses the in-memory roster while the renderer replays the child - * rows from disk; without reseeding, the next teammate-bearing Stop (whose - * task ids never match lifecycle ids) would silently drop those rows. Stale - * seeds self-heal: an empty background_tasks clears, activity refreshes. */ +/** Rebuild a pane's working roster from a persisted status snapshot. Live + * activity confirms a seed after restart; a complete task inventory may reap + * an unconfirmed seed whose finish hook arrived while Orca was offline. */ export function seedClaudeSubagentRosterFromSnapshots( state: HookListenerState, paneKey: string, @@ -2493,18 +2490,19 @@ export function seedClaudeSubagentRosterFromSnapshots( } const roster = getOrCreateClaudeSubagentRoster(state, paneKey) for (const snapshot of snapshots) { + // Why: the roster only tracks working children now. A persisted idle + // snapshot (from a build that kept idle rows) is a finished child — drop + // it so restart doesn't resurrect the stale pile this fix removes. + if (snapshot.state !== 'working') { + continue + } roster.set(snapshot.id, { - state: snapshot.state === 'working' ? 'working' : 'idle', startedAt: snapshot.startedAt, agentType: snapshot.agentType, description: snapshot.description, - // Why: teammate name and agent type can differ, but the provider id - // shape survives persistence and keeps the row across restart folds. - ...(isClaudeTeammateLifecycleId(snapshot.id) ? { teammate: true as const } : {}), // Why: the seed can be a phantom (child finished while Orca was down, // its SubagentStop lost). Let a PRESENT background_tasks list that - // omits the id remove it (or demote a teammate) instead of gating the - // pane 'working' forever. + // omits the id remove it instead of gating the pane 'working' forever. backgroundTasksAuthoritative: true }) } diff --git a/src/shared/claude-subagent-roster.test.ts b/src/shared/claude-subagent-roster.test.ts index 99cd9bbb8dc..7d91bd45576 100644 --- a/src/shared/claude-subagent-roster.test.ts +++ b/src/shared/claude-subagent-roster.test.ts @@ -6,117 +6,53 @@ import { claudeTeammateIdMatchesName, finishClaudeSubagent, foldClaudeBackgroundTasksIntoRoster, - markClaudeTeammateIdleByName, readClaudeBackgroundAgentTasks, + removeClaudeTeammateByName, upsertWorkingClaudeSubagent, type ClaudeSubagentRoster } from './claude-subagent-roster' +const task = ( + over: Partial['tasks'][number]> +) => ({ + id: 'a1', + agentType: undefined, + description: undefined, + running: true, + teammate: false, + ...over +}) + describe('claude-subagent-roster', () => { it('removes a finished one-shot subagent on stop', () => { const roster: ClaudeSubagentRoster = new Map() upsertWorkingClaudeSubagent(roster, 'a1', { agentType: 'general-purpose' }, 100) expect(claudeRosterHasWorkingSubagent(roster)).toBe(true) - // Why: retaining finished one-shots as idle rows piled up dozens of dead + // Why: retaining finished children as idle rows piled up dozens of dead // "Idle - general-purpose" sidebar rows over a long workflow session. finishClaudeSubagent(roster, 'a1') expect(roster.size).toBe(0) expect(claudeRosterToSnapshots(roster)).toBeUndefined() }) - it('idles a teammate on stop instead of removing it', () => { + it('removes a finished teammate-shaped named agent on stop', () => { const roster: ClaudeSubagentRoster = new Map() + // Why: named/workflow agents report teammate-shaped ids, and their + // background_tasks teammate entries never stop reading "running" — so + // SubagentStop is the only reliable finish signal and must remove the row. upsertWorkingClaudeSubagent(roster, 'aprobe1-6d3cb5b5', { agentType: 'probe1' }, 100) finishClaudeSubagent(roster, 'aprobe1-6d3cb5b5') - expect(roster.get('aprobe1-6d3cb5b5')).toMatchObject({ state: 'idle', teammate: true }) + expect(roster.has('aprobe1-6d3cb5b5')).toBe(false) }) - it('removes a finished workflow lane despite its teammate-shaped id when task-listed', () => { - const roster: ClaudeSubagentRoster = new Map() - // Why: workflow lanes get name-embedding ids (afinder-C-) like - // teammates, but their SubagentStop payload lists them id-exact as a - // subagent task — real teammate lifecycle ids never appear there. - upsertWorkingClaudeSubagent( - roster, - 'afinder-C-5d713c0781b7f8d2', - { agentType: 'finder-C' }, - 100 - ) - finishClaudeSubagent(roster, 'afinder-C-5d713c0781b7f8d2', { listedAsSubagentTask: true }) - expect(roster.size).toBe(0) - }) - - it('reclassifies a task-listed teammate-shaped entry as a one-shot', () => { - const roster: ClaudeSubagentRoster = new Map() - upsertWorkingClaudeSubagent( - roster, - 'av1-streaming-0b1c2d3e', - { agentType: 'v1-streaming' }, - 100 - ) - foldClaudeBackgroundTasksIntoRoster( - roster, - [ - { - id: 'av1-streaming-0b1c2d3e', - agentType: 'v1-streaming', - description: undefined, - running: true, - teammate: false - }, - { - id: 'tteam1', - agentType: undefined, - description: undefined, - running: true, - teammate: true - } - ], - 200 - ) - // A later stop without its own inventory still removes it: the fold - // already proved the id is a task id, not a teammate. - finishClaudeSubagent(roster, 'av1-streaming-0b1c2d3e') - expect(roster.has('av1-streaming-0b1c2d3e')).toBe(false) - }) - - it('removes teammate-shaped leftovers when a complete inventory lists no teammates', () => { - const roster: ClaudeSubagentRoster = new Map() - upsertWorkingClaudeSubagent(roster, 'acr-triage-1-c5a0588e', { agentType: 'cr-triage-1' }, 100) - finishClaudeSubagent(roster, 'acr-triage-1-c5a0588e') - expect(roster.get('acr-triage-1-c5a0588e')).toMatchObject({ state: 'idle' }) - upsertWorkingClaudeSubagent(roster, 'afix-main-11223344', { agentType: 'fix-main' }, 150) - - // Why: a teams session lists its teammates (even idle) as teammate-typed - // tasks; an inventory with none proves these name-shaped rows are dead - // workflow lanes, not resumable teammates. - foldClaudeBackgroundTasksIntoRoster( - roster, - [ - { - id: 'aunrelated0000001', - agentType: 'general-purpose', - description: undefined, - running: true, - teammate: false - } - ], - 200 - ) - expect(roster.has('acr-triage-1-c5a0588e')).toBe(false) - expect(roster.has('afix-main-11223344')).toBe(false) - expect(roster.has('aunrelated0000001')).toBe(true) - }) - - it('re-marks an idle teammate working without resetting startedAt', () => { + it('re-adds a resumed agent as working with a fresh startedAt', () => { const roster: ClaudeSubagentRoster = new Map() upsertWorkingClaudeSubagent(roster, 'aprobe1-6d3cb5b5', { agentType: 'probe1' }, 100) finishClaudeSubagent(roster, 'aprobe1-6d3cb5b5') upsertWorkingClaudeSubagent(roster, 'aprobe1-6d3cb5b5', { description: 'round two' }, 200) expect(roster.get('aprobe1-6d3cb5b5')).toMatchObject({ - state: 'working', - startedAt: 100, + startedAt: 200, description: 'round two' }) }) @@ -127,22 +63,39 @@ describe('claude-subagent-roster', () => { expect(roster.size).toBe(0) }) - it('caps roster size, evicting the oldest idle entry first', () => { + it('drops new spawns at the cap rather than evicting live children', () => { const roster: ClaudeSubagentRoster = new Map() - upsertWorkingClaudeSubagent(roster, 'aprobe1-6d3cb5b5', { agentType: 'probe1' }, 0) - for (let i = 1; i < AGENT_STATUS_MAX_SUBAGENTS; i++) { + for (let i = 0; i < AGENT_STATUS_MAX_SUBAGENTS; i++) { upsertWorkingClaudeSubagent(roster, `a${i}`, {}, i) } - // Why: all working — a new spawn cannot evict live children and is dropped. + // Why: every tracked entry is working — nothing is safe to evict, so the + // overflow spawn is dropped (it would be invisible past the wire cap). upsertWorkingClaudeSubagent(roster, 'overflow', {}, 999) expect(roster.has('overflow')).toBe(false) + expect(roster.size).toBe(AGENT_STATUS_MAX_SUBAGENTS) - // Why: only teammates hold idle entries now; an idle teammate is the - // eviction pool when a burst of new spawns hits the cap. - finishClaudeSubagent(roster, 'aprobe1-6d3cb5b5') + // Once a child finishes, a new spawn takes the freed slot. + finishClaudeSubagent(roster, 'a0') upsertWorkingClaudeSubagent(roster, 'replacement', {}, 1000) expect(roster.has('replacement')).toBe(true) - expect(roster.has('aprobe1-6d3cb5b5')).toBe(false) + expect(roster.size).toBe(AGENT_STATUS_MAX_SUBAGENTS) + }) + + it('reconciles stale entries before adding replacement tasks at the cap', () => { + const roster: ClaudeSubagentRoster = new Map() + for (let i = 0; i < AGENT_STATUS_MAX_SUBAGENTS; i++) { + upsertWorkingClaudeSubagent(roster, `a${i}`, {}, i) + } + const tasks = Array.from({ length: AGENT_STATUS_MAX_SUBAGENTS }, (_, index) => + task({ id: index === 0 ? 'replacement' : `a${index}` }) + ) + + // Why: a complete inventory can replace a stale child while the roster is + // full. Reap the stale slot first so the new live child keeps the done-gate. + foldClaudeBackgroundTasksIntoRoster(roster, tasks, 999) + + expect(roster.has('a0')).toBe(false) + expect(roster.has('replacement')).toBe(true) expect(roster.size).toBe(AGENT_STATUS_MAX_SUBAGENTS) }) @@ -197,70 +150,102 @@ describe('claude-subagent-roster', () => { expect(result.truncated).toBe(true) }) - it('folds background_tasks in without trusting ambiguous entries', () => { + it('trusts id-exact subagent matches and ignores teammate-typed entries', () => { const roster: ClaudeSubagentRoster = new Map() upsertWorkingClaudeSubagent(roster, 'a1', {}, 100) + // A named agent mid-run: teammate-shaped id, never a task id. upsertWorkingClaudeSubagent(roster, 'ateam-6d3cb5b5', { agentType: 'security-reviewer' }, 150) foldClaudeBackgroundTasksIntoRoster( roster, [ - { - id: 'a1', - agentType: 'general-purpose', - description: 'review loop', - running: true, - teammate: false - }, + task({ id: 'a1', agentType: 'general-purpose', description: 'review loop' }), // Why: teammate task ids never match lifecycle agent_ids; unmatched // teammate entries must not create phantom duplicate children. - { - id: 'tlkjjs0jv', - agentType: undefined, - description: 'teammate task', - running: true, - teammate: true - } + task({ id: 'tlkjjs0jv', description: 'teammate task', teammate: true }) ], 200 ) expect(roster.size).toBe(2) - // Why: id-exact matches are one-shot subagents whose run state IS reliable. - expect(roster.get('a1')).toMatchObject({ state: 'working', description: 'review loop' }) - // Why: the working lifecycle-tracked teammate is not listed by id, but - // omission proves nothing for teammates — it must survive the fold. - expect(roster.get('ateam-6d3cb5b5')).toMatchObject({ - state: 'working', - agentType: 'security-reviewer' - }) + expect(roster.get('a1')).toMatchObject({ description: 'review loop' }) + // Why: the working named agent is not listed by id, but a teammate-typed + // task is present — so omission proves nothing and it survives the fold. + expect(roster.get('ateam-6d3cb5b5')).toMatchObject({ agentType: 'security-reviewer' }) }) - it('removes an id-matched task reported not running', () => { + it('removes an id-matched subagent task reported not running', () => { const roster: ClaudeSubagentRoster = new Map() upsertWorkingClaudeSubagent(roster, 'a1', { agentType: 'general-purpose' }, 100) - foldClaudeBackgroundTasksIntoRoster( - roster, - [{ id: 'a1', agentType: undefined, description: undefined, running: false, teammate: false }], - 200 - ) + foldClaudeBackgroundTasksIntoRoster(roster, [task({ id: 'a1', running: false })], 200) expect(roster.size).toBe(0) }) it('removes a killed one-shot missing from a present list', () => { const roster: ClaudeSubagentRoster = new Map() // Why: a running one-shot is always listed id-exact at a lead Stop, so a - // working non-teammate missing from the list is dead (SubagentStop lost); - // keeping it pinned the pane 'working' forever. + // working hyphen-free child missing from the list is dead (SubagentStop + // lost); keeping it pinned the pane 'working' forever. upsertWorkingClaudeSubagent(roster, 'akilled0000000001', { agentType: 'general-purpose' }, 100) + foldClaudeBackgroundTasksIntoRoster(roster, [task({ id: 'other', teammate: true })], 200) + expect(roster.size).toBe(0) + }) + + it('keeps a live named agent whose teammate-typed id never appears in the list', () => { + const roster: ClaudeSubagentRoster = new Map() + // Why: this is the core regression — a running named agent (teammate-shaped + // id, lifecycle-tracked, never a task id) must survive a lead Stop whose + // list still shows teammate-typed tasks, or the pane resolves done early. + upsertWorkingClaudeSubagent( + roster, + 'aweb-research-8a76b7d7', + { agentType: 'web-research' }, + 100 + ) + foldClaudeBackgroundTasksIntoRoster( + roster, + [task({ id: 't6s2brfv7', description: 'named agent task', teammate: true })], + 200 + ) + expect(roster.has('aweb-research-8a76b7d7')).toBe(true) + }) + + it('removes teammate-shaped leftovers when a complete inventory lists no teammates', () => { + const roster: ClaudeSubagentRoster = new Map() + upsertWorkingClaudeSubagent(roster, 'acr-triage-1-c5a0588e', { agentType: 'cr-triage-1' }, 100) + // Why: a session with named agents/teammates always lists at least one + // teammate-typed task while any is alive; an inventory with none proves + // this teammate-shaped row is dead (its SubagentStop was lost). + foldClaudeBackgroundTasksIntoRoster( + roster, + [task({ id: 'aunrelated0000001', agentType: 'general-purpose' })], + 200 + ) + expect(roster.has('acr-triage-1-c5a0588e')).toBe(false) + expect(roster.has('aunrelated0000001')).toBe(true) + }) + + it('removes a leftover once a subagent-typed task listed its id id-exact', () => { + const roster: ClaudeSubagentRoster = new Map() + upsertWorkingClaudeSubagent( + roster, + 'av1-streaming-0b1c2d3e', + { agentType: 'v1-streaming' }, + 100 + ) + // A subagent-typed task lists it id-exact → tag it listedAsSubagentTask. foldClaudeBackgroundTasksIntoRoster( roster, [ - { id: 'other', agentType: undefined, description: undefined, running: true, teammate: true } + task({ id: 'av1-streaming-0b1c2d3e', agentType: 'v1-streaming' }), + task({ id: 'tteam1', teammate: true }) ], 200 ) - expect(roster.size).toBe(0) + // A later Stop omits it while a teammate-typed task is still present: the + // subagent-listing proof overrides the teammate-shape protection. + foldClaudeBackgroundTasksIntoRoster(roster, [task({ id: 'tteam1', teammate: true })], 300) + expect(roster.has('av1-streaming-0b1c2d3e')).toBe(false) }) it('retains an unlisted live child when the background task inventory was truncated', () => { @@ -285,25 +270,13 @@ describe('claude-subagent-roster', () => { foldClaudeBackgroundTasksIntoRoster( roster, [ - { - id: 'a9', - agentType: 'general-purpose', - description: 'long build', - running: true, - teammate: false - }, - { - id: 'gone', - agentType: undefined, - description: undefined, - running: false, - teammate: false - } + task({ id: 'a9', agentType: 'general-purpose', description: 'long build' }), + task({ id: 'gone', running: false }) ], 500 ) - expect(roster.get('a9')).toMatchObject({ state: 'working', startedAt: 500 }) - // Why: a finished unmatched one-shot leaves no reason to add an idle row. + expect(roster.get('a9')).toMatchObject({ startedAt: 500 }) + // Why: a finished unmatched one-shot leaves no reason to add a row. expect(roster.has('gone')).toBe(false) }) @@ -314,86 +287,46 @@ describe('claude-subagent-roster', () => { expect(roster.size).toBe(0) }) - it('removes non-teammate authoritative entries and keeps live teammates on omission', () => { + it('does not clear the roster on an empty but incomplete (truncated) inventory', () => { const roster: ClaudeSubagentRoster = new Map() - roster.set('a-phantom', { - state: 'working', - startedAt: 100, - backgroundTasksAuthoritative: true - }) - upsertWorkingClaudeSubagent(roster, 'ateam-6d3cb5b5', { agentType: 'security-reviewer' }, 150) - - foldClaudeBackgroundTasksIntoRoster( - roster, - [ - { id: 'other', agentType: undefined, description: undefined, running: true, teammate: true } - ], - 200 - ) - expect(roster.has('a-phantom')).toBe(false) - expect(roster.get('ateam-6d3cb5b5')).toMatchObject({ state: 'working' }) + upsertWorkingClaudeSubagent(roster, 'a1', {}, 100) + foldClaudeBackgroundTasksIntoRoster(roster, [], 200, { inventoryComplete: false }) + expect(roster.has('a1')).toBe(true) }) - it('demotes a seeded teammate phantom missing from a present list to idle', () => { + it('removes a seeded phantom missing from a present list', () => { const roster: ClaudeSubagentRoster = new Map() - // Why: a teammate seeded from a pre-restart snapshot may be dead, but its - // task id never appears in background_tasks — demote instead of delete so - // a live idle teammate keeps its row while a phantom stops gating the pane. + // A snapshot-seeded entry (child finished while Orca was down) is + // authoritative — a present list omitting it removes it even though its + // id is teammate-shaped. roster.set('aprobe1-6d3cb5b5', { - state: 'working', startedAt: 100, agentType: 'probe1', - teammate: true, backgroundTasksAuthoritative: true }) - foldClaudeBackgroundTasksIntoRoster( - roster, - [ - { id: 'other', agentType: undefined, description: undefined, running: true, teammate: true } - ], - 200 - ) - expect(roster.get('aprobe1-6d3cb5b5')).toMatchObject({ state: 'idle', teammate: true }) + foldClaudeBackgroundTasksIntoRoster(roster, [task({ id: 'other', teammate: true })], 200) + expect(roster.has('aprobe1-6d3cb5b5')).toBe(false) }) - it('removes fold-recreated entries missing from a later present list', () => { - const roster: ClaudeSubagentRoster = new Map() - foldClaudeBackgroundTasksIntoRoster( - roster, - [{ id: 'a9', agentType: undefined, description: undefined, running: true, teammate: false }], - 100 - ) - foldClaudeBackgroundTasksIntoRoster( - roster, - [ - { id: 'other', agentType: undefined, description: undefined, running: true, teammate: true } - ], - 200 - ) - expect(roster.has('a9')).toBe(false) - }) - - it('keeps a re-tracked working teammate missing from a present list', () => { + it('keeps a re-tracked working named agent missing from a present list', () => { const roster: ClaudeSubagentRoster = new Map() roster.set('aprobe1-6d3cb5b5', { - state: 'working', startedAt: 100, agentType: 'probe1', - teammate: true, backgroundTasksAuthoritative: true }) - // Why: live activity clears the authoritative flag; a busy teammate must - // not be demoted by a Stop that (as always) omits its lifecycle id. + // Why: live activity clears the authoritative flag; a busy named agent + // must not be reaped by a Stop that (as always) omits its lifecycle id. upsertWorkingClaudeSubagent(roster, 'aprobe1-6d3cb5b5', { agentType: 'probe1' }, 150) + foldClaudeBackgroundTasksIntoRoster(roster, [task({ id: 'other', teammate: true })], 200) + expect(roster.has('aprobe1-6d3cb5b5')).toBe(true) + }) - foldClaudeBackgroundTasksIntoRoster( - roster, - [ - { id: 'other', agentType: undefined, description: undefined, running: true, teammate: true } - ], - 200 - ) - expect(roster.get('aprobe1-6d3cb5b5')).toMatchObject({ state: 'working' }) + it('removes fold-recreated one-shots missing from a later present list', () => { + const roster: ClaudeSubagentRoster = new Map() + foldClaudeBackgroundTasksIntoRoster(roster, [task({ id: 'a9' })], 100) + foldClaudeBackgroundTasksIntoRoster(roster, [task({ id: 'other', teammate: true })], 200) + expect(roster.has('a9')).toBe(false) }) it('matches teammate ids by name only up to the hyphen-free suffix', () => { @@ -404,17 +337,29 @@ describe('claude-subagent-roster', () => { expect(claudeTeammateIdMatchesName('aprobe1', 'probe1')).toBe(false) }) - it('marks teammates idle by name via agent_type or agent_id prefix', () => { + it('removes teammates by the name embedded in agent_id', () => { const roster: ClaudeSubagentRoster = new Map() upsertWorkingClaudeSubagent(roster, 'aprobe1-6d3cb5b5', { agentType: 'probe1' }, 100) upsertWorkingClaudeSubagent(roster, 'aother-123', { agentType: 'other' }, 100) - expect(markClaudeTeammateIdleByName(roster, 'probe1')).toBe(true) - expect(roster.get('aprobe1-6d3cb5b5')?.state).toBe('idle') - expect(roster.get('aother-123')?.state).toBe('working') - // Why: repeat idles are no-ops so lifecycle refreshes don't churn state. - expect(markClaudeTeammateIdleByName(roster, 'probe1')).toBe(false) - expect(markClaudeTeammateIdleByName(roster, 'ghost')).toBe(false) + // Why: TeammateIdle is keyed by name — idle means finished, so the row goes. + expect(removeClaudeTeammateByName(roster, 'probe1')).toBe(true) + expect(roster.has('aprobe1-6d3cb5b5')).toBe(false) + expect(roster.has('aother-123')).toBe(true) + // Repeat/unknown removals are no-ops so lifecycle refreshes don't churn. + expect(removeClaudeTeammateByName(roster, 'probe1')).toBe(false) + expect(removeClaudeTeammateByName(roster, 'ghost')).toBe(false) + }) + + it('does not remove an unrelated one-shot whose agent_type matches the teammate name', () => { + const roster: ClaudeSubagentRoster = new Map() + // Why: a teammate's start hook may be missing (restart, cap, or lost + // delivery). Agent type is not identity, so its idle hook must not reap + // another live child that happens to use the same type name. + upsertWorkingClaudeSubagent(roster, 'aoneshot00000001', { agentType: 'reviewer' }, 100) + + expect(removeClaudeTeammateByName(roster, 'reviewer')).toBe(false) + expect(roster.has('aoneshot00000001')).toBe(true) }) it('serializes snapshots deterministically ordered by startedAt then id', () => { @@ -422,7 +367,10 @@ describe('claude-subagent-roster', () => { upsertWorkingClaudeSubagent(roster, 'b', {}, 200) upsertWorkingClaudeSubagent(roster, 'z', {}, 100) upsertWorkingClaudeSubagent(roster, 'a', {}, 100) - expect(claudeRosterToSnapshots(roster)?.map((s) => s.id)).toEqual(['a', 'z', 'b']) + const snapshots = claudeRosterToSnapshots(roster) + expect(snapshots?.map((s) => s.id)).toEqual(['a', 'z', 'b']) + // Why: only working children are tracked, so every emitted row is working. + expect(snapshots?.every((s) => s.state === 'working')).toBe(true) expect(claudeRosterToSnapshots(new Map())).toBeUndefined() }) }) diff --git a/src/shared/claude-subagent-roster.ts b/src/shared/claude-subagent-roster.ts index b0b945a76e2..a94cbae0aa0 100644 --- a/src/shared/claude-subagent-roster.ts +++ b/src/shared/claude-subagent-roster.ts @@ -5,28 +5,33 @@ import { AGENT_STATUS_MAX_SUBAGENTS, type AgentSubagentSnapshot } from './agent- * invisible in the emitted snapshots (which drop such ids). */ const CLAUDE_SUBAGENT_ID_MAX_LENGTH = 64 -/** Live subagents/teammates tracked for one Claude pane, keyed by the - * provider-assigned `agent_id` from SubagentStart/SubagentStop payloads. */ +/** Currently WORKING subagents/teammates tracked for one Claude pane, keyed + * by the provider-assigned `agent_id` from SubagentStart/SubagentStop + * payloads. The roster intentionally holds only working children: a child + * that finished leaves the sidebar immediately. Claude gives no other + * finish signal for named agents — their `background_tasks` teammate + * entries stay `status: "running"` forever, even after they complete + * (verified live on 2.1.210) — so retaining "idle" rows piled up dead + * entries for hours. A teammate resumed later re-earns its row via + * SubagentStart. */ export type ClaudeSubagentRoster = Map export type TrackedClaudeSubagent = { agentType?: string description?: string - state: 'working' | 'idle' startedAt: number - /** Known agent-teams teammate: its id embeds its name (`a-`) - * while one-shot ids are hyphen-free (`a`). Teammates are long-lived — - * SubagentStop means "finished a task", not "gone" — and their lifecycle - * ids never appear in background_tasks, so omission proves nothing. */ - teammate?: true /** The id came from a persisted snapshot or background_tasks, not live - * lifecycle events. Only matters for teammate-shaped seeds now: a PRESENT - * list omitting a seeded-working teammate demotes it to idle so a phantom - * seeded before restart can't gate the pane 'working' forever (teams - * sessions never send an empty list). Cleared once live activity re-tracks - * the id. Non-teammate entries omitted from a present list are removed - * outright regardless of this flag. */ + * lifecycle events, so it may be a phantom whose SubagentStop was never + * observed (Orca restart). A present complete task list omitting it + * removes it even when teammate-shaped, so it can't gate the pane + * 'working' forever. Cleared once live activity re-tracks the id. */ backgroundTasksAuthoritative?: boolean + /** A subagent-typed background task listed this lifecycle id id-exact + * (workflow/named lanes) — proof the task list tracks this id, so a later + * complete list omitting it means finished/killed even though the id is + * teammate-shaped. Never cleared: the listing mode of an id can't change + * mid-life. */ + listedAsSubagentTask?: true } /** One agent entry from the `background_tasks` array Claude attaches to Stop @@ -37,14 +42,15 @@ export type ClaudeBackgroundAgentTask = { agentType?: string description?: string running: boolean - /** True for `type: "teammate"` entries, whose ids never match lifecycle - * agent_ids and whose "running" status persists while idle. */ + /** True for `type: "teammate"` entries. Their ids never match lifecycle + * agent_ids and they report "running" permanently — even after the named + * agent finished — so they carry no per-agent state at all. */ teammate: boolean } -/** Agent-team lifecycle ids are `a-`. The teammate name - * and agent type are independent spawn fields, so the id shape is the only - * reliable discriminator available on SubagentStart/SubagentStop hooks. */ +/** Agent-team/named-agent lifecycle ids are `a-` while one-shot + * ids are hyphen-free (`a`). Such ids are never listed as task ids in + * `background_tasks`, so omission from the list proves nothing for them. */ export function isClaudeTeammateLifecycleId(id: string): boolean { const separator = id.lastIndexOf('-') return separator > 1 && id.startsWith('a') && /^[0-9a-f]+$/i.test(id.slice(separator + 1)) @@ -59,73 +65,33 @@ export function upsertWorkingClaudeSubagent( if (id.length === 0 || id.length > CLAUDE_SUBAGENT_ID_MAX_LENGTH) { return } - const teammate = isClaudeTeammateLifecycleId(id) const existing = roster.get(id) if (existing) { - existing.state = 'working' existing.agentType = fields.agentType ?? existing.agentType existing.description = fields.description ?? existing.description - if (teammate) { - existing.teammate = true - } // Why: live activity proves the lifecycle stream owns this id again; - // background_tasks absence must stop demoting it (teammate ids never - // appear there). The fold re-tags its own recreations after this call. + // background_tasks omission must stop reaping it (teammate-shaped ids + // never appear there). The fold re-tags its own recreations after this. existing.backgroundTasksAuthoritative = undefined return } - if (roster.size >= AGENT_STATUS_MAX_SUBAGENTS && !evictOldestIdleClaudeSubagent(roster)) { + // Why: beyond the wire cap extra rows would be invisible anyway; with only + // working entries tracked there is nothing safe to evict. + if (roster.size >= AGENT_STATUS_MAX_SUBAGENTS) { return } roster.set(id, { - state: 'working', startedAt: now, agentType: fields.agentType, - description: fields.description, - ...(teammate ? { teammate: true as const } : {}) + description: fields.description }) } -function evictOldestIdleClaudeSubagent(roster: ClaudeSubagentRoster): boolean { - let oldestId: string | null = null - let oldestStartedAt = Infinity - for (const [id, tracked] of roster) { - if (tracked.state === 'idle' && tracked.startedAt < oldestStartedAt) { - oldestId = id - oldestStartedAt = tracked.startedAt - } - } - if (oldestId === null) { - return false - } - roster.delete(oldestId) - return true -} - -/** SubagentStop: a finished one-shot subagent leaves the sidebar immediately — - * retaining it as an idle row made long workflow/ultracode sessions pile up - * dozens of dead rows. Teammates only idle (alive + resumable): SubagentStop - * fires each time a teammate finishes a task, not just at shutdown. - * - * Workflow/named one-shots share the teammate id shape (`a