From 9f4ea42493883ca2c86145bd6bd630761a151226 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:37:07 -0700 Subject: [PATCH 01/43] fix(agent-status): say what an OpenCode permission request is waiting on (STA-3160) (#14614) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(agent-status): say what an OpenCode permission request is waiting on (STA-3160) A permission.asked arrives as hook_event_name PermissionRequest, but extractOpenCodeToolFields had no branch for it, so the pane reported a bare {state:'waiting'} with no tool or command. The user could see that OpenCode was blocked but not on what. Read the fields @opencode-ai/sdk fixes for EventPermissionAsked: 'permission' names the request, and metadata/patterns carry the command or paths it covers. The normalizer is shared with mimo-code, so both are covered. * fix(agent-status): show the OpenCode permission on the row, and retire it after (STA-3160) Live validation against opencode 1.18.18 showed the original change populated toolName/toolInput on a `waiting` entry that no surface rendered, while leaving the answered permission cached for the rest of the pane's session. Retire the tool fields on every OpenCode-family event except PermissionRequest. isNewTurnEvent is false for this family, so resolveToolState otherwise inherits one answered permission onto every later frame and the row reads a resolved command as the live tool. Reproduced end to end: after approving `rm -rf build/`, an unrelated later turn still reported it. Read `filepath` from permission metadata. The SDK types metadata as Record, so its keys come from each tool; a live opencode 1.18.18 sends `filepath` (one word) for `edit`, which the previous key list missed. The fallback to `patterns` covered it by accident, and the test that claimed to cover it used `metadata: {}` — a shape OpenCode never emits. Tests now use captured payloads for bash, edit and webfetch. Show tool fields on `waiting` as well as `working`. All three consumers gated on `working`, so a permission request rendered nothing at all; before/after of the sidebar was pixel-identical. The rule now lives in one place (showsAgentToolPreview) because a gate duplicated across three surfaces is a gate that drifts. --- .../dashboard/DashboardAgentRow.test.tsx | 39 +++ .../dashboard/DashboardAgentRow.tsx | 12 +- .../dashboard/DashboardAgentRowToolStep.tsx | 9 +- .../worktree-card-compact-agent-row.tsx | 13 +- .../src/lib/activity-thread-display.test.ts | 38 +++ .../src/lib/activity-thread-display.ts | 13 +- .../src/lib/agent-row-tool-preview.test.ts | 51 ++++ .../src/lib/agent-row-tool-preview.ts | 32 +++ src/shared/agent-hook-listener.ts | 49 +++- src/shared/opencode-permission-status.test.ts | 255 ++++++++++++++++++ 10 files changed, 483 insertions(+), 28 deletions(-) create mode 100644 src/renderer/src/lib/agent-row-tool-preview.test.ts create mode 100644 src/renderer/src/lib/agent-row-tool-preview.ts create mode 100644 src/shared/opencode-permission-status.test.ts diff --git a/src/renderer/src/components/dashboard/DashboardAgentRow.test.tsx b/src/renderer/src/components/dashboard/DashboardAgentRow.test.tsx index 5be1c78d574..4ce41472d49 100644 --- a/src/renderer/src/components/dashboard/DashboardAgentRow.test.tsx +++ b/src/renderer/src/components/dashboard/DashboardAgentRow.test.tsx @@ -348,6 +348,45 @@ describe('DashboardAgentRow', () => { expect(activeToolMarkup).toContain('ListDir') }) + it('names what a blocked approval is waiting on', () => { + const markup = renderRow( + makeAgent( + { state: 'waiting' }, + { state: 'waiting', toolName: 'bash', toolInput: 'rm -rf build/' } + ) + ) + + // Why: a permission request parks the row on 'waiting'; without the tool line the row + // is a bare amber dot that cannot say what the user is being asked to approve. + expect(markup).toContain('lucide-wrench') + expect(markup).toContain('bash') + expect(markup).toContain('rm -rf build/') + }) + + it('leaves a wait without tool metadata unchanged', () => { + const markup = renderRow(makeAgent({ state: 'waiting' }, { state: 'waiting' })) + + // Why: the height placeholder exists because a working row's tool line is imminent. + // A question-style wait has nothing coming, so reserving the row would just add a gap. + expect(markup).not.toContain('data-agent-row-tool-slot=""') + expect(markup).not.toContain('lucide-wrench') + }) + + it('keeps a resolved tool off a finished row', () => { + for (const state of ['done', 'blocked', 'idle'] as const) { + const markup = renderRow( + makeAgent( + { state }, + // Why: 'idle' is a row-only state; the entry it is derived from still reports 'done'. + { state: state === 'idle' ? 'done' : state, toolName: 'bash', toolInput: 'rm -rf build/' } + ) + ) + + expect(markup).not.toContain('lucide-wrench') + expect(markup).not.toContain('rm -rf build/') + } + }) + it('renders orchestration child rows with a connector and tree level', () => { const markup = renderRow( makeAgent({ diff --git a/src/renderer/src/components/dashboard/DashboardAgentRow.tsx b/src/renderer/src/components/dashboard/DashboardAgentRow.tsx index 460c251b98b..96c52f12d22 100644 --- a/src/renderer/src/components/dashboard/DashboardAgentRow.tsx +++ b/src/renderer/src/components/dashboard/DashboardAgentRow.tsx @@ -8,6 +8,7 @@ import { DashboardAgentChildDisclosure } from './DashboardAgentChildDisclosure' import { DashboardAgentRowMessage } from './DashboardAgentRowMessage' import { DashboardAgentRowTrailingControls } from './DashboardAgentRowTrailingControls' import { DashboardAgentRowToolStep } from './DashboardAgentRowToolStep' +import { showsAgentToolPreview } from '@/lib/agent-row-tool-preview' import type { AgentStatusState } from '../../../../shared/agent-status-types' import type { DashboardAgentRow as DashboardAgentRowData } from './useDashboardData' import { getAgentRowPrimaryText } from '@/lib/agent-row-primary-text' @@ -144,10 +145,12 @@ const DashboardAgentRow = React.memo(function DashboardAgentRow({ // Why: prompt is '' when unknown, so fall back to the state label to keep the row labeled. const displayLabel = prompt || agentStateLabel(asDotState(agent.state)) const model = agent.entry.model?.trim() ?? '' - // Why: gate tool fields on 'working' — a stale tool line on a done row reads as still-running. const isWorking = agent.state === 'working' - const toolName = isWorking ? (agent.entry.toolName?.trim() ?? '') : '' - const toolInput = isWorking ? (agent.entry.toolInput?.trim() ?? '') : '' + // Why: 'working' names the running tool and 'waiting' names what an approval is blocked on; + // anywhere else a leftover tool line reads as still-running. See showsAgentToolPreview. + const showsTool = showsAgentToolPreview(agent.state) + const toolName = showsTool ? (agent.entry.toolName?.trim() ?? '') : '' + const toolInput = showsTool ? (agent.entry.toolInput?.trim() ?? '') : '' const lastAssistantMessage = agent.entry.lastAssistantMessage?.trim() ?? '' const isInterrupted = agent.entry.interrupted === true const lineage = agent.lineage @@ -300,7 +303,8 @@ const DashboardAgentRow = React.memo(function DashboardAgentRow({ diff --git a/src/renderer/src/components/dashboard/DashboardAgentRowToolStep.tsx b/src/renderer/src/components/dashboard/DashboardAgentRowToolStep.tsx index 1be38acde0e..5362212bb20 100644 --- a/src/renderer/src/components/dashboard/DashboardAgentRowToolStep.tsx +++ b/src/renderer/src/components/dashboard/DashboardAgentRowToolStep.tsx @@ -3,18 +3,21 @@ import { cn } from '@/lib/utils' type DashboardAgentRowToolStepProps = { expanded: boolean - isWorking: boolean + showsTool: boolean + /** Hold the slot open while tool metadata is still in flight — see the empty branch below. */ + reservesHeight: boolean toolName: string toolInput: string } export function DashboardAgentRowToolStep({ expanded, - isWorking, + showsTool, + reservesHeight, toolName, toolInput }: DashboardAgentRowToolStepProps): React.JSX.Element | null { - if (!isWorking) { + if (!showsTool || (!toolName && !reservesHeight)) { return null } diff --git a/src/renderer/src/components/sidebar/worktree-card-compact-agent-row.tsx b/src/renderer/src/components/sidebar/worktree-card-compact-agent-row.tsx index ac60427fea6..c285d4769af 100644 --- a/src/renderer/src/components/sidebar/worktree-card-compact-agent-row.tsx +++ b/src/renderer/src/components/sidebar/worktree-card-compact-agent-row.tsx @@ -8,6 +8,7 @@ import { cn } from '@/lib/utils' import { getAgentDotState } from './worktree-card-agent-summary' import { translate } from '@/i18n/i18n' import { getAgentRowPrimaryText } from '@/lib/agent-row-primary-text' +import { formatAgentToolPreview } from '@/lib/agent-row-tool-preview' import { useAgentRowConversationName } from '@/components/dashboard/use-agent-row-conversation-name' import { lastEnteredDoneAt } from '@/components/dashboard/agent-finished-timestamp' import CacheTimer, { usePromptCacheCountdownForPane } from './CacheTimer' @@ -40,15 +41,9 @@ function getCompactAgentSecondary(agent: DashboardAgentRowData): string { if (agent.entry.interrupted === true) { return 'Interrupted by user' } - if (agent.state === 'working') { - const toolName = agent.entry.toolName?.trim() ?? '' - const toolInput = agent.entry.toolInput?.trim() ?? '' - if (toolName && toolInput) { - return `${toolName}: ${toolInput}` - } - if (toolName) { - return toolName - } + const toolPreview = formatAgentToolPreview(agent.entry, agent.state) + if (toolPreview) { + return toolPreview } const lastAssistantMessage = agent.entry.lastAssistantMessage?.trim() if (lastAssistantMessage) { diff --git a/src/renderer/src/lib/activity-thread-display.test.ts b/src/renderer/src/lib/activity-thread-display.test.ts index 1728c8ab5ca..6beb4387c13 100644 --- a/src/renderer/src/lib/activity-thread-display.test.ts +++ b/src/renderer/src/lib/activity-thread-display.test.ts @@ -200,6 +200,44 @@ describe('getActivityThreadStatusPreview', () => { }) ).toBe('Interrupted by user') }) + + it('names what a blocked approval is waiting on', () => { + // Why: a permission request parks the row on 'waiting', and the tool fields are the + // only thing that says what the user is being asked to approve (STA-3160). + expect( + getActivityThreadStatusPreview({ + state: 'waiting', + toolName: 'bash', + toolInput: 'rm -rf build/', + prompt: 'Clean the build' + }) + ).toBe('bash: rm -rf build/') + }) + + it('keeps a resolved tool off a finished row', () => { + // Why: tool fields outlive the turn that set them; showing them once the agent is done + // or idle reads as work still in flight. + for (const state of ['done', 'blocked'] as const) { + expect( + getActivityThreadStatusPreview({ + state, + toolName: 'bash', + toolInput: 'rm -rf build/', + prompt: 'Clean the build' + }) + ).toBe('') + } + }) + + it('shows nothing on a wait that carries no tool', () => { + // Why: not every wait is an approval — a question-style wait must not borrow tool text. + expect( + getActivityThreadStatusPreview({ + state: 'waiting', + prompt: 'Pick a branch' + }) + ).toBe('') + }) }) describe('resolveActivityThreadStatusPreview', () => { diff --git a/src/renderer/src/lib/activity-thread-display.ts b/src/renderer/src/lib/activity-thread-display.ts index ea6603edeac..7dd5f663370 100644 --- a/src/renderer/src/lib/activity-thread-display.ts +++ b/src/renderer/src/lib/activity-thread-display.ts @@ -10,6 +10,7 @@ import { isOrcaDispatchPrompt, orchestrationLabelsMatchLiveDispatch } from './agent-row-primary-text' +import { formatAgentToolPreview } from './agent-row-tool-preview' // Why: follow-up replies ("yes", "ok proceed") are valid hook prompts but are // terrible scan labels for a cross-worktree agent list — treat them as non-titles. @@ -164,15 +165,9 @@ export function getActivityThreadStatusPreview( return 'Interrupted by user' } const state = agentState ?? entry.state - if (state === 'working') { - const toolName = entry.toolName?.trim() ?? '' - const toolInput = entry.toolInput?.trim() ?? '' - if (toolName && toolInput) { - return `${toolName}: ${toolInput}` - } - if (toolName) { - return toolName - } + const toolPreview = formatAgentToolPreview(entry, state) + if (toolPreview) { + return toolPreview } const assistant = entry.lastAssistantMessage?.trim() ?? '' if (assistant && !isMislabeledUserPrompt(assistant, entry)) { diff --git a/src/renderer/src/lib/agent-row-tool-preview.test.ts b/src/renderer/src/lib/agent-row-tool-preview.test.ts new file mode 100644 index 00000000000..b9267afe5e6 --- /dev/null +++ b/src/renderer/src/lib/agent-row-tool-preview.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from 'vitest' +import { AGENT_STATUS_STATES } from '../../../shared/agent-status-types' +import type { AgentRowState } from './agent-row-tool-preview' +import { formatAgentToolPreview, showsAgentToolPreview } from './agent-row-tool-preview' + +const TOOL = { toolName: 'bash', toolInput: 'rm -rf build/' } +const ROW_STATES: readonly AgentRowState[] = [...AGENT_STATUS_STATES, 'idle'] + +describe('showsAgentToolPreview', () => { + it('covers exactly the two states whose tool fields describe live work', () => { + // Why: enumerate every row state so a new one has to make this decision explicitly + // rather than defaulting into showing a stale tool. + const showing = ROW_STATES.filter((state) => showsAgentToolPreview(state)) + + expect(showing).toEqual(['working', 'waiting']) + }) + + it('shows nothing when the state is unknown', () => { + expect(showsAgentToolPreview(null)).toBe(false) + expect(showsAgentToolPreview(undefined)).toBe(false) + }) +}) + +describe('formatAgentToolPreview', () => { + it('names what a blocked approval is waiting on', () => { + expect(formatAgentToolPreview(TOOL, 'waiting')).toBe('bash: rm -rf build/') + }) + + it('names the running tool while working', () => { + expect(formatAgentToolPreview(TOOL, 'working')).toBe('bash: rm -rf build/') + }) + + it('keeps a resolved tool off every other state', () => { + for (const state of ROW_STATES.filter((candidate) => !showsAgentToolPreview(candidate))) { + expect(formatAgentToolPreview(TOOL, state)).toBe('') + } + }) + + it('shows nothing on a wait that carries no tool', () => { + // Why: a question-style wait sets no tool fields; it must not borrow an earlier one. + expect(formatAgentToolPreview({}, 'waiting')).toBe('') + }) + + it('falls back to the bare tool name when the input is not previewable', () => { + expect(formatAgentToolPreview({ toolName: 'edit' }, 'waiting')).toBe('edit') + }) + + it('ignores whitespace-only fields', () => { + expect(formatAgentToolPreview({ toolName: ' ', toolInput: ' ' }, 'working')).toBe('') + }) +}) diff --git a/src/renderer/src/lib/agent-row-tool-preview.ts b/src/renderer/src/lib/agent-row-tool-preview.ts new file mode 100644 index 00000000000..1ac4c478af9 --- /dev/null +++ b/src/renderer/src/lib/agent-row-tool-preview.ts @@ -0,0 +1,32 @@ +import type { AgentStatusEntry, AgentStatusState } from '../../../shared/agent-status-types' + +/** Row states, which add the renderer-only 'idle' to the hook-reported statuses. */ +export type AgentRowState = AgentStatusState | 'idle' + +/** + * States whose cached tool fields describe live work: 'working' names the tool the agent is + * running, 'waiting' names the tool a permission request is blocked on. Tool fields outlive + * the turn that set them, so on any other state they read as work still in flight. + * + * Shared because three surfaces render this line — the sidebar's compact row, the dashboard + * row, and the activity thread — and a rule that only two of them relax is a rule that lies. + */ +export function showsAgentToolPreview(state: AgentRowState | null | undefined): boolean { + return state === 'working' || state === 'waiting' +} + +/** One-line `tool: input` preview, or '' when the state or the entry has nothing to show. */ +export function formatAgentToolPreview( + entry: Pick, + state: AgentRowState | null | undefined +): string { + if (!showsAgentToolPreview(state)) { + return '' + } + const toolName = entry.toolName?.trim() ?? '' + const toolInput = entry.toolInput?.trim() ?? '' + if (toolName && toolInput) { + return `${toolName}: ${toolInput}` + } + return toolName +} diff --git a/src/shared/agent-hook-listener.ts b/src/shared/agent-hook-listener.ts index e471c55d25e..61202afec0e 100644 --- a/src/shared/agent-hook-listener.ts +++ b/src/shared/agent-hook-listener.ts @@ -1787,6 +1787,17 @@ function extractAmpToolFields( return {} } +/** + * Retires any cached tool fields. PermissionRequest is the only OpenCode-family event that + * carries them, and isNewTurnEvent is false for this family, so nothing else ever resets the + * cache — without an explicit retire, resolveToolState inherits one answered permission onto + * every later frame in the pane and the row reads a resolved command as the live tool. + */ +const OPENCODE_TOOL_FIELDS_RETIRED: ToolSnapshot = { + hasToolUpdate: true, + hasToolInputField: true +} + function extractOpenCodeToolFields( eventName: unknown, hookPayload: Record @@ -1794,7 +1805,7 @@ function extractOpenCodeToolFields( if (eventName === 'MessagePart' && hookPayload.role === 'assistant') { const text = readString(hookPayload, 'text') if (text) { - return { lastAssistantMessage: capOpenCodeHookText(text) } + return { ...OPENCODE_TOOL_FIELDS_RETIRED, lastAssistantMessage: capOpenCodeHookText(text) } } } if (eventName === 'AskUserQuestion') { @@ -1803,11 +1814,43 @@ function extractOpenCodeToolFields( ? hookPayload.tool_input : stripHookEnvelopeKeys(hookPayload) return { - hasToolUpdate: true, + ...OPENCODE_TOOL_FIELDS_RETIRED, interactivePrompt: deriveInteractivePrompt('AskUserQuestion', toolInputSource) } } - return {} + if (eventName === 'PermissionRequest') { + // Why: the payload is permission.asked's event.properties — `permission` names what is + // being requested and `metadata`/`patterns` say which command or path it covers. Without + // them the row is a bare 'waiting' that cannot tell the user what to approve. + // The SDK types `metadata` as Record, so these keys come from the tools + // themselves: bash sends `command`, edit sends `filepath`, webfetch sends `url` (verified + // against opencode 1.18.18). `file_path`/`path` cover tools that spell it the common way. + // `diff` is deliberately absent — edit ships the whole patch and it would swamp the row. + const metadata = hookPayload.metadata + const metadataInput = + metadata !== null && typeof metadata === 'object' && !Array.isArray(metadata) + ? readFirstString(metadata as Record, [ + 'command', + 'filepath', + 'file_path', + 'path', + 'url' + ]) + : undefined + const patterns = Array.isArray(hookPayload.patterns) + ? hookPayload.patterns.filter( + (pattern): pattern is string => typeof pattern === 'string' && pattern.length > 0 + ) + : [] + return toolUpdate( + { + toolName: readString(hookPayload, 'permission'), + toolInput: metadataInput ?? (patterns.length > 0 ? patterns.join(', ') : undefined) + }, + { hasToolInputField: hasAnyOwnField(hookPayload, ['metadata', 'patterns']) } + ) + } + return OPENCODE_TOOL_FIELDS_RETIRED } function extractCursorToolFields( diff --git a/src/shared/opencode-permission-status.test.ts b/src/shared/opencode-permission-status.test.ts new file mode 100644 index 00000000000..6ed57e71fac --- /dev/null +++ b/src/shared/opencode-permission-status.test.ts @@ -0,0 +1,255 @@ +import { beforeEach, describe, expect, it } from 'vitest' +import { createHookListenerState, normalizeHookPayload } from './agent-hook-listener' +import { makePaneKey } from './stable-pane-id' + +const PANE_KEY = makePaneKey('tab-1', '11111111-1111-4111-8111-111111111111') + +/** + * OpenCode reports an approval prompt as `permission.asked`; the status plugin forwards + * that event's `properties` under `hook_event_name: 'PermissionRequest'`. The shape is + * fixed by @opencode-ai/sdk 1.18.18 (`EventPermissionAsked`): `permission` names what is + * being requested, `patterns` carries the concrete commands/paths it applies to, and + * `metadata` holds tool-specific detail. + * + * Only those outer names are typed — `metadata` is `Record`, so the keys + * inside it come from each tool. The payloads below were captured from a live opencode + * 1.18.18 rather than invented, because the first version of this suite guessed a shape + * (`metadata: {}`) that OpenCode never emits and so never exercised the real `edit` path. + * + * `normalizeOpenCodeFamilyEvent` serves opencode and mimo-code from one path, so every + * case here runs for both. + */ +describe('OpenCode-family permission request status', () => { + let state: ReturnType + + beforeEach(() => { + state = createHookListenerState() + }) + + const SOURCES = ['opencode', 'mimo-code'] as const + + function permissionEvent( + source: (typeof SOURCES)[number], + properties: Record + ): ReturnType { + return normalizeHookPayload( + state, + source, + { + paneKey: PANE_KEY, + payload: { hook_event_name: 'PermissionRequest', ...properties } + }, + 'production' + ) + } + + function lifecycleEvent( + source: (typeof SOURCES)[number], + eventName: string, + properties: Record = {} + ): ReturnType { + return normalizeHookPayload( + state, + source, + { paneKey: PANE_KEY, payload: { hook_event_name: eventName, ...properties } }, + 'production' + ) + } + + const BASH_PERMISSION = { + id: 'per_01', + sessionID: 'ses_root', + permission: 'bash', + patterns: ['rm -rf build/'], + metadata: { command: 'rm -rf build/' }, + always: [] + } + + it.each(SOURCES)('reports a permission request as waiting for %s', (source) => { + const event = permissionEvent(source, BASH_PERMISSION) + + expect(event?.payload.state).toBe('waiting') + expect(event?.payload.agentType).toBe(source) + }) + + it.each(SOURCES)('names the requested permission as the tool for %s', (source) => { + const event = permissionEvent(source, BASH_PERMISSION) + + // Why: a bare `waiting` row cannot tell the user what is blocked; the permission + // name is the only field that always identifies it (STA-3160). + expect(event?.payload.toolName).toBe('bash') + }) + + it.each(SOURCES)('surfaces the blocked command as tool input for %s', (source) => { + const event = permissionEvent(source, BASH_PERMISSION) + + expect(event?.payload.toolInput).toBe('rm -rf build/') + }) + + it.each(SOURCES)('names the edited file for %s', (source) => { + // Captured verbatim from opencode 1.18.18 — an `edit` request keys its path as + // `filepath` (one word) and ships the whole patch alongside it. + const event = permissionEvent(source, { + id: 'per_02', + sessionID: 'ses_root', + permission: 'edit', + patterns: ['private/tmp/oc-perm-probe/notes.md'], + metadata: { + filepath: '/private/tmp/oc-perm-probe/notes.md', + diff: '@@ -0,0 +1,1 @@\n+hello world\n\\ No newline at end of file\n' + }, + always: ['*'] + }) + + expect(event?.payload.toolName).toBe('edit') + expect(event?.payload.toolInput).toBe('/private/tmp/oc-perm-probe/notes.md') + }) + + it.each(SOURCES)('never previews a diff as the blocked target for %s', (source) => { + const event = permissionEvent(source, { + id: 'per_03', + sessionID: 'ses_root', + permission: 'edit', + patterns: ['src/a.ts'], + metadata: { diff: '@@ -1 +1 @@\n-before\n+after\n' }, + always: [] + }) + + // Why: `diff` is the largest field OpenCode sends and would swamp a one-line row. + expect(event?.payload.toolInput).not.toContain('@@') + }) + + it.each(SOURCES)('names the fetched url for %s', (source) => { + // Captured verbatim from opencode 1.18.18. + const event = permissionEvent(source, { + id: 'per_04', + sessionID: 'ses_root', + permission: 'webfetch', + patterns: ['https://example.com'], + metadata: { url: 'https://example.com', format: 'markdown' }, + always: ['*'] + }) + + expect(event?.payload.toolName).toBe('webfetch') + expect(event?.payload.toolInput).toBe('https://example.com') + }) + + it.each(SOURCES)('falls back to patterns when metadata is unrecognized for %s', (source) => { + // Why: `metadata` is typed `Record` by the SDK, so a tool Orca has + // never seen (an MCP server's, say) can key its detail anything — patterns is the one + // field every permission carries. + const event = permissionEvent(source, { + id: 'per_05', + sessionID: 'ses_root', + permission: 'some_mcp_tool', + patterns: ['src/a.ts', 'src/b.ts'], + metadata: { unrecognizedKey: 'ignored' }, + always: [] + }) + + expect(event?.payload.toolName).toBe('some_mcp_tool') + expect(event?.payload.toolInput).toBe('src/a.ts, src/b.ts') + }) + + it.each(SOURCES)( + 'still reports waiting when the payload names no permission for %s', + (source) => { + // Why: an unrecognized/absent permission must not downgrade the blocked state — the + // user still has to answer, they just get less detail. + const event = permissionEvent(source, { id: 'per_06', sessionID: 'ses_root' }) + + expect(event?.payload.state).toBe('waiting') + expect(event?.payload.toolName).toBeUndefined() + } + ) + + it.each(SOURCES)('leaves the AskUserQuestion route untouched for %s', (source) => { + const properties = { questions: [{ question: 'Choose', options: ['x', 'y'] }] } + const event = normalizeHookPayload( + state, + source, + { + paneKey: PANE_KEY, + payload: { hook_event_name: 'AskUserQuestion', ...properties } + }, + 'production' + ) + + expect(event?.payload.state).toBe('waiting') + expect(event?.payload.interactivePrompt).toBe(JSON.stringify(properties)) + }) + + it.each(SOURCES)('clears permission metadata once the turn resumes for %s', (source) => { + permissionEvent(source, BASH_PERMISSION) + const resumed = lifecycleEvent(source, 'SessionBusy') + + // Why: a stale approval card must not outlive the approval — once work resumes the + // row is working again and the blocked command is no longer what the pane is on. + expect(resumed?.payload.state).toBe('working') + expect(resumed?.payload.toolName).toBeUndefined() + expect(resumed?.payload.toolInput).toBeUndefined() + }) + + it.each(SOURCES)('retires the permission when the session goes idle for %s', (source) => { + permissionEvent(source, BASH_PERMISSION) + const idle = lifecycleEvent(source, 'SessionIdle') + + expect(idle?.payload.state).toBe('done') + expect(idle?.payload.toolName).toBeUndefined() + expect(idle?.payload.toolInput).toBeUndefined() + }) + + it.each(SOURCES)( + 'does not label a later question with the answered permission for %s', + (source) => { + permissionEvent(source, BASH_PERMISSION) + const question = lifecycleEvent(source, 'AskUserQuestion', { + questions: [{ question: 'Choose', options: ['x', 'y'] }] + }) + + expect(question?.payload.state).toBe('waiting') + expect(question?.payload.toolName).toBeUndefined() + expect(question?.payload.toolInput).toBeUndefined() + } + ) + + it.each(SOURCES)('does not attribute the answered permission to a reply for %s', (source) => { + permissionEvent(source, BASH_PERMISSION) + const part = lifecycleEvent(source, 'MessagePart', { role: 'assistant', text: 'Ran it.' }) + + expect(part?.payload.lastAssistantMessage).toBe('Ran it.') + expect(part?.payload.toolName).toBeUndefined() + expect(part?.payload.toolInput).toBeUndefined() + }) + + it.each(SOURCES)('does not carry an answered permission into a later wait for %s', (source) => { + // Why: the row now shows tool fields on 'waiting' as well as 'working' (that is the whole + // point of STA-3160), so a later non-permission wait must arrive with none — otherwise + // relaxing the row gate turns an answered command into the question's caption. + permissionEvent(source, BASH_PERMISSION) + lifecycleEvent(source, 'SessionBusy') + lifecycleEvent(source, 'SessionIdle') + const laterQuestion = lifecycleEvent(source, 'AskUserQuestion', { + questions: [{ question: 'Which branch?', options: ['main', 'dev'] }] + }) + + expect(laterQuestion?.payload.state).toBe('waiting') + expect(laterQuestion?.payload.toolInput).toBeUndefined() + expect(laterQuestion?.payload.toolName).toBeUndefined() + }) + + it.each(SOURCES)('does not carry an answered permission into a later turn for %s', (source) => { + // Why: isNewTurnEvent is false for this family, so nothing else ever resets the cached + // tool. Without an explicit retire, one permission pins its command to every later + // working frame in the pane — the exact stale-tool-line the row gate guards against. + permissionEvent(source, BASH_PERMISSION) + lifecycleEvent(source, 'SessionBusy') + lifecycleEvent(source, 'SessionIdle') + const laterTurn = lifecycleEvent(source, 'SessionBusy') + + expect(laterTurn?.payload.state).toBe('working') + // Why: assert the command first — it is the string a user would misread as live work. + expect(laterTurn?.payload.toolInput).toBeUndefined() + expect(laterTurn?.payload.toolName).toBeUndefined() + }) +}) From 15efc87e35c5e00b3ba3922ef9e9bc55769599a0 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:38:59 -0700 Subject: [PATCH 02/43] fix(agent-hooks): bind agent status to the pane its session was spawned into (STA-2069) (#14615) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(agent-hooks): bind agent status to the pane the session was spawned into (STA-2069) Claude Code >= 2.1.206 hosts TUI sessions as workers under a shared daemon, and the daemon forwards only its own allowlisted env — so hook posts carry whichever pane first started the daemon, not the pane the user is in. Pin a minted --session-id at spawn where Orca still knows the pane, record sessionId -> pane, and correct the posted key at both hook ingest seams. Co-authored-by: Brian Dai <43929761+BrianDai22@users.noreply.github.com> * fix(agent-hooks): pin the session id in root-option position, not appended Appending `--session-id ` broke every `claude ` launch: `--session-id` is a ROOT option, so `claude mcp list --session-id ` exits with "error: unknown option '--session-id'". Splice it immediately after the executable token instead, which is valid for both a bare session and a subcommand, and is already before claude's own `--` terminator. Also write the binding-key separator as an escape rather than a raw NUL byte, which made the file a binary blob in git. Close three hunks that no test could fail on: the pty.ts spawn call site that records the binding, the relay seam's worktreeId override, and the already-correct-pane early-return that suppresses a worktree restamp. --------- Co-authored-by: Brian Dai <43929761+BrianDai22@users.noreply.github.com> --- .../agent-session-pane-bindings.test.ts | 66 +++++ .../agent-session-pane-bindings.ts | 84 ++++++ ...ver-agent-session-pane-attribution.test.ts | 247 ++++++++++++++++++ src/main/agent-hooks/server.ts | 94 ++++++- src/main/ipc/pty-ipc-mock-registry.ts | 6 +- src/main/ipc/pty-ipc-suite-environment.ts | 4 + .../pty-login-shell-startup-commands.test.ts | 58 +++- src/main/ipc/pty.ts | 71 ++++- src/main/pty/spawn-startup-shell.test.ts | 57 ++++ src/main/pty/spawn-startup-shell.ts | 25 ++ src/shared/agent-hook-listener.ts | 38 +++ src/shared/agent-resume-launch-command.ts | 66 +---- src/shared/claude-launch-executable-token.ts | 38 +++ .../claude-session-pin-launch-command.test.ts | 104 ++++++++ .../claude-session-pin-launch-command.ts | 86 ++++++ src/shared/tui-agent-startup-shell.ts | 39 +++ 16 files changed, 1011 insertions(+), 72 deletions(-) create mode 100644 src/main/agent-hooks/agent-session-pane-bindings.test.ts create mode 100644 src/main/agent-hooks/agent-session-pane-bindings.ts create mode 100644 src/main/agent-hooks/server-agent-session-pane-attribution.test.ts create mode 100644 src/main/pty/spawn-startup-shell.test.ts create mode 100644 src/main/pty/spawn-startup-shell.ts create mode 100644 src/shared/claude-launch-executable-token.ts create mode 100644 src/shared/claude-session-pin-launch-command.test.ts create mode 100644 src/shared/claude-session-pin-launch-command.ts diff --git a/src/main/agent-hooks/agent-session-pane-bindings.test.ts b/src/main/agent-hooks/agent-session-pane-bindings.test.ts new file mode 100644 index 00000000000..fa70d892325 --- /dev/null +++ b/src/main/agent-hooks/agent-session-pane-bindings.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, it } from 'vitest' + +import { AgentSessionPaneBindings } from './agent-session-pane-bindings' +import { makePaneKey } from '../../shared/stable-pane-id' + +const PANE_A = makePaneKey('tab-1', 'aaaaaaaa-1111-4111-8111-111111111111') +const PANE_B = makePaneKey('tab-1', 'bbbbbbbb-2222-4222-8222-222222222222') + +describe('AgentSessionPaneBindings', () => { + it('resolves only the source it was bound under', () => { + const bindings = new AgentSessionPaneBindings() + bindings.bind('claude', 'sess-1', { paneKey: PANE_A, ptyId: 'pty-1' }) + + expect(bindings.resolve('claude', 'sess-1')?.paneKey).toBe(PANE_A) + expect(bindings.resolve('codex', 'sess-1')).toBeNull() + expect(bindings.resolve('claude', 'sess-2')).toBeNull() + expect(bindings.resolve('claude', null)).toBeNull() + }) + + it('follows a session that re-binds to another pane', () => { + const bindings = new AgentSessionPaneBindings() + bindings.bind('claude', 'sess-1', { paneKey: PANE_A, ptyId: 'pty-1' }) + bindings.bind('claude', 'sess-1', { paneKey: PANE_B, ptyId: 'pty-2' }) + + expect(bindings.resolve('claude', 'sess-1')?.paneKey).toBe(PANE_B) + expect(bindings.size()).toBe(1) + }) + + it('drops every binding a dying PTY established, and only those', () => { + const bindings = new AgentSessionPaneBindings() + bindings.bind('claude', 'sess-1', { paneKey: PANE_A, ptyId: 'pty-1' }) + bindings.bind('claude', 'sess-2', { paneKey: PANE_A, ptyId: 'pty-1' }) + bindings.bind('claude', 'sess-3', { paneKey: PANE_B, ptyId: 'pty-2' }) + + bindings.clearForPty('pty-1') + + expect(bindings.resolve('claude', 'sess-1')).toBeNull() + expect(bindings.resolve('claude', 'sess-2')).toBeNull() + expect(bindings.resolve('claude', 'sess-3')?.paneKey).toBe(PANE_B) + }) + + it('evicts least-recently-bound sessions past the cap, keeping a re-bound one alive', () => { + const bindings = new AgentSessionPaneBindings() + bindings.bind('claude', 'sess-0', { paneKey: PANE_A, ptyId: 'pty-0' }) + for (let i = 1; i < 512; i += 1) { + bindings.bind('claude', `sess-${i}`, { paneKey: PANE_B, ptyId: `pty-${i}` }) + } + // Touch the oldest so it is no longer the eviction candidate. + bindings.bind('claude', 'sess-0', { paneKey: PANE_A, ptyId: 'pty-0' }) + bindings.bind('claude', 'sess-512', { paneKey: PANE_B, ptyId: 'pty-512' }) + + expect(bindings.size()).toBe(512) + expect(bindings.resolve('claude', 'sess-0')?.paneKey).toBe(PANE_A) + expect(bindings.resolve('claude', 'sess-1')).toBeNull() + expect(bindings.resolve('claude', 'sess-512')?.paneKey).toBe(PANE_B) + }) + + it('ignores incomplete bindings rather than storing an unroutable row', () => { + const bindings = new AgentSessionPaneBindings() + bindings.bind('claude', ' ', { paneKey: PANE_A, ptyId: 'pty-1' }) + bindings.bind('claude', 'sess-1', { paneKey: '', ptyId: 'pty-1' }) + bindings.bind('claude', 'sess-2', { paneKey: PANE_A, ptyId: '' }) + + expect(bindings.size()).toBe(0) + }) +}) diff --git a/src/main/agent-hooks/agent-session-pane-bindings.ts b/src/main/agent-hooks/agent-session-pane-bindings.ts new file mode 100644 index 00000000000..df26b0a1a11 --- /dev/null +++ b/src/main/agent-hooks/agent-session-pane-bindings.ts @@ -0,0 +1,84 @@ +import type { AgentHookSource } from '../../shared/agent-hook-relay' + +export type AgentSessionPaneBinding = { + paneKey: string + /** The PTY whose spawn established this binding; the binding dies with it. */ + ptyId: string + /** Why carried: the inherited env names the DAEMON's workspace too, so a + * corrected event would otherwise keep filing itself under the wrong one. */ + worktreeId?: string +} + +// Why: one entry per pinned session. Generous headroom over any realistic live +// count while still bounding a caller that leaks registrations. +const MAX_BINDINGS = 512 + +/** + * Maps a provider session id to the pane Orca spawned it into. + * + * Why this layer exists: a pane's identity reaches an agent's hooks only + * through `ORCA_PANE_KEY` in the process environment, and a CLI that hosts + * sessions inside a shared prewarmed daemon runs its hooks in a worker that + * inherited the DAEMON's env — whichever pane happened to start the daemon, + * possibly days ago in another workspace. The posted pane key is then simply + * wrong, and the hook payload carries no other pane coordinate, so no + * listener-side heuristic can recover the right one. The binding is therefore + * established where Orca still knows both halves — at spawn, where it mints the + * session id and owns the pane — and consulted at ingest. + * + * Session-scoped rather than process-scoped on purpose: the daemon's worker is + * not the pane's PTY and can outlive it, so process ancestry proves nothing. + * Transport-agnostic for the same reason — a remote host's daemon inherits a + * stale key exactly like a local one, and both ingest seams resolve here. + */ +export class AgentSessionPaneBindings { + private bindings = new Map() + + private static key(source: AgentHookSource, sessionId: string): string { + // Why: source-scoped so two CLIs cannot collide on a shared id namespace. + return `${source}\u0000${sessionId}` + } + + bind(source: AgentHookSource, sessionId: string, binding: AgentSessionPaneBinding): void { + const sessionKey = sessionId.trim() + if (!sessionKey || !binding.paneKey || !binding.ptyId) { + return + } + const key = AgentSessionPaneBindings.key(source, sessionKey) + // Why: delete first so re-binding an existing session moves it to the end + // of the insertion order — eviction stays least-recently-bound. + this.bindings.delete(key) + this.bindings.set(key, binding) + while (this.bindings.size > MAX_BINDINGS) { + const oldest = this.bindings.keys().next().value + if (oldest === undefined) { + break + } + this.bindings.delete(oldest) + } + } + + resolve( + source: AgentHookSource, + sessionId: string | null | undefined + ): AgentSessionPaneBinding | null { + if (!sessionId) { + return null + } + return this.bindings.get(AgentSessionPaneBindings.key(source, sessionId.trim())) ?? null + } + + /** Drops every binding a dying PTY established, so a reused id space cannot + * re-route a later session onto a pane that no longer runs it. */ + clearForPty(ptyId: string): void { + for (const [key, binding] of this.bindings) { + if (binding.ptyId === ptyId) { + this.bindings.delete(key) + } + } + } + + size(): number { + return this.bindings.size + } +} diff --git a/src/main/agent-hooks/server-agent-session-pane-attribution.test.ts b/src/main/agent-hooks/server-agent-session-pane-attribution.test.ts new file mode 100644 index 00000000000..b567ad22e59 --- /dev/null +++ b/src/main/agent-hooks/server-agent-session-pane-attribution.test.ts @@ -0,0 +1,247 @@ +import { describe, expect, it, vi } from 'vitest' + +import { AgentHookServer, isValidPaneKey, _internals } from './server' +import { + createHookListenerState, + normalizeHookPayload, + readHookBodyProviderSessionId +} from '../../shared/agent-hook-listener' +import { makePaneKey } from '../../shared/stable-pane-id' + +vi.mock('../telemetry/client', () => ({ track: vi.fn() })) +vi.mock('../telemetry/cohort-classifier', () => ({ getCohortAtEmit: () => ({}) })) + +// Why: split panes live in ONE tab — same tabId, different leaf. The daemon's +// inherited key names a pane in a different workspace entirely. +const SPLIT_LEAF_A = 'aaaaaaaa-1111-4111-8111-111111111111' +const SPLIT_LEAF_B = 'bbbbbbbb-2222-4222-8222-222222222222' +const DAEMON_LEAF = 'cccccccc-3333-4333-8333-333333333333' + +const SPLIT_PANE_A = makePaneKey('tab-split', SPLIT_LEAF_A) +const SPLIT_PANE_B = makePaneKey('tab-split', SPLIT_LEAF_B) +/** The pane that first spawned the shared daemon; every later worker inherits its env. */ +const DAEMON_PANE = makePaneKey('tab-daemon', DAEMON_LEAF) + +const SESSION_A = '0192a4b1-1111-4111-8111-aaaaaaaaaaaa' +const SESSION_B = '0192a4b1-2222-4222-8222-bbbbbbbbbbbb' +const UNPINNED_SESSION = '0192a4b1-9999-4999-8999-999999999999' + +function buildDaemonHostedBody(sessionId: string, prompt: string): Record { + return { + // The daemon worker's env, NOT the pane the user is typing in. + paneKey: DAEMON_PANE, + tabId: 'tab-daemon', + worktreeId: 'wt-daemon', + env: 'production', + payload: { + hook_event_name: 'UserPromptSubmit', + prompt, + session_id: sessionId + } + } +} + +async function withServer( + run: (server: AgentHookServer, post: (body: unknown) => Promise) => Promise +): Promise { + _internals.resetCachesForTests() + const server = new AgentHookServer() + await server.start({ env: 'production' }) + try { + const env = server.buildPtyEnv() + const post = async (body: unknown): Promise => { + const response = await fetch(`http://127.0.0.1:${env.ORCA_AGENT_HOOK_PORT}/hook/claude`, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + 'X-Orca-Agent-Hook-Token': env.ORCA_AGENT_HOOK_TOKEN + }, + body: JSON.stringify(body) + }) + return response.status + } + await run(server, post) + } finally { + server.stop() + } +} + +function panesInSnapshot(server: AgentHookServer): { paneKey: string; prompt?: string }[] { + return server + .getStatusSnapshot() + .map((entry) => ({ paneKey: entry.paneKey, prompt: entry.prompt })) +} + +describe('shared-daemon pane attribution (local hook path)', () => { + it('routes a daemon-hosted turn to the pane the session was spawned into, not the inherited pane key', async () => { + await withServer(async (server, post) => { + server.bindAgentSessionPane('claude', SESSION_A, { paneKey: SPLIT_PANE_A, ptyId: 'pty-a' }) + + expect(await post(buildDaemonHostedBody(SESSION_A, 'ship the fix'))).toBe(204) + + expect(panesInSnapshot(server)).toEqual([{ paneKey: SPLIT_PANE_A, prompt: 'ship the fix' }]) + }) + }) + + it('re-files a corrected event under the workspace it was spawned in, not the daemon’s', async () => { + await withServer(async (server, post) => { + server.bindAgentSessionPane('claude', SESSION_A, { + paneKey: SPLIT_PANE_A, + ptyId: 'pty-a', + worktreeId: 'wt-real' + }) + + expect(await post(buildDaemonHostedBody(SESSION_A, 'cross-workspace turn'))).toBe(204) + + // 'wt-daemon' is the daemon's inherited workspace and must not survive. + expect(server.getStatusSnapshot()).toEqual([ + expect.objectContaining({ paneKey: SPLIT_PANE_A, worktreeId: 'wt-real' }) + ]) + }) + }) + + it('keeps two split panes in one tab on their own rows when both post the same inherited key', async () => { + await withServer(async (server, post) => { + server.bindAgentSessionPane('claude', SESSION_A, { paneKey: SPLIT_PANE_A, ptyId: 'pty-a' }) + server.bindAgentSessionPane('claude', SESSION_B, { paneKey: SPLIT_PANE_B, ptyId: 'pty-b' }) + + expect(await post(buildDaemonHostedBody(SESSION_A, 'left pane work'))).toBe(204) + expect(await post(buildDaemonHostedBody(SESSION_B, 'right pane work'))).toBe(204) + + // Without per-session binding both turns collapse onto DAEMON_PANE and the + // second prompt clobbers the first. + expect(panesInSnapshot(server).sort((a, b) => a.paneKey.localeCompare(b.paneKey))).toEqual([ + { paneKey: SPLIT_PANE_A, prompt: 'left pane work' }, + { paneKey: SPLIT_PANE_B, prompt: 'right pane work' } + ]) + }) + }) + + it('leaves an unpinned session on the pane it posted', async () => { + await withServer(async (server, post) => { + server.bindAgentSessionPane('claude', SESSION_A, { paneKey: SPLIT_PANE_A, ptyId: 'pty-a' }) + + expect(await post(buildDaemonHostedBody(UNPINNED_SESSION, 'typed claude'))).toBe(204) + + // A hand-typed `claude` in a plain shell has no pin; today's behavior stands. + expect(panesInSnapshot(server)).toEqual([{ paneKey: DAEMON_PANE, prompt: 'typed claude' }]) + }) + }) + + it('stops re-routing once the pinning PTY exits', async () => { + await withServer(async (server, post) => { + server.bindAgentSessionPane('claude', SESSION_A, { paneKey: SPLIT_PANE_A, ptyId: 'pty-a' }) + server.clearAgentSessionPaneBindingsForPty('pty-a') + + expect(await post(buildDaemonHostedBody(SESSION_A, 'after pane closed'))).toBe(204) + + expect(panesInSnapshot(server)).toEqual([ + { paneKey: DAEMON_PANE, prompt: 'after pane closed' } + ]) + }) + }) + + it('does not let one agent binding capture another agent that reuses the session id', async () => { + await withServer(async (server, post) => { + // Same id, different hook source: bindings are source-scoped. + server.bindAgentSessionPane('codex', SESSION_A, { paneKey: SPLIT_PANE_A, ptyId: 'pty-a' }) + + expect(await post(buildDaemonHostedBody(SESSION_A, 'claude turn'))).toBe(204) + + expect(panesInSnapshot(server)).toEqual([{ paneKey: DAEMON_PANE, prompt: 'claude turn' }]) + }) + }) + + it('refuses to bind a pane key the status pipeline cannot route', async () => { + // Why #14018 reported an unroutable "$$:L$$" key: any key that is not + // `:` is rejected at the hook boundary, so a binding must + // never be able to introduce one. + const remintedShape = '$$q7v2m9c4:L$$' + expect(isValidPaneKey(remintedShape)).toBe(false) + + await withServer(async (server, post) => { + server.bindAgentSessionPane('claude', SESSION_A, { paneKey: remintedShape, ptyId: 'pty-a' }) + + expect(await post(buildDaemonHostedBody(SESSION_A, 'unroutable target'))).toBe(204) + + expect(panesInSnapshot(server)).toEqual([ + { paneKey: DAEMON_PANE, prompt: 'unroutable target' } + ]) + }) + }) +}) + +describe('shared-daemon pane attribution (relay path)', () => { + it('routes a remote daemon-hosted turn to the spawn-pinned pane', () => { + _internals.resetCachesForTests() + const server = new AgentHookServer() + server.bindAgentSessionPane('claude', SESSION_A, { + paneKey: SPLIT_PANE_A, + ptyId: 'pty-remote', + worktreeId: 'wt-real' + }) + + const event = normalizeHookPayload( + createHookListenerState(), + 'claude', + buildDaemonHostedBody(SESSION_A, 'remote turn'), + 'production' + ) + if (!event) { + throw new Error('normalizeHookPayload rejected a known-good relay fixture') + } + expect(event.providerSession?.id).toBe(SESSION_A) + server.ingestRemote({ ...event, source: 'claude' }, 'conn-1') + + expect(panesInSnapshot(server)).toEqual([{ paneKey: SPLIT_PANE_A, prompt: 'remote turn' }]) + // Why asserted here too: the relay seam has its own worktree override, and + // 'wt-daemon' is the workspace the stale env named. + expect(server.getStatusSnapshot()[0]?.worktreeId).toBe('wt-real') + }) +}) + +describe('an already-correct pane key', () => { + it('is left alone rather than overwritten from the binding', async () => { + // Why this matters beyond an early-out: the binding also carries a + // worktreeId. A session that posts its own (correct) pane must not have its + // workspace restamped from a binding recorded at spawn time. + await withServer(async (server, post) => { + server.bindAgentSessionPane('claude', SESSION_A, { + paneKey: DAEMON_PANE, + ptyId: 'pty-a', + worktreeId: 'wt-at-spawn' + }) + + expect(await post(buildDaemonHostedBody(SESSION_A, 'posted its own pane'))).toBe(204) + + expect(server.getStatusSnapshot()).toEqual([ + expect.objectContaining({ paneKey: DAEMON_PANE, worktreeId: 'wt-daemon' }) + ]) + }) + }) +}) + +describe('readHookBodyProviderSessionId', () => { + it('reads the id from both the object and JSON-string payload forms', () => { + const body = buildDaemonHostedBody(SESSION_A, 'x') + expect(readHookBodyProviderSessionId('claude', body)).toBe(SESSION_A) + expect( + readHookBodyProviderSessionId('claude', { ...body, payload: JSON.stringify(body.payload) }) + ).toBe(SESSION_A) + }) + + it('ignores a Codex child hook, whose session id belongs to the child not the pane', () => { + expect( + readHookBodyProviderSessionId('codex', { + paneKey: DAEMON_PANE, + payload: { session_id: SESSION_A, agent_id: 'child-1' } + }) + ).toBeNull() + }) + + it('returns null for malformed payloads instead of throwing', () => { + expect(readHookBodyProviderSessionId('claude', null)).toBeNull() + expect(readHookBodyProviderSessionId('claude', { payload: '{not json' })).toBeNull() + expect(readHookBodyProviderSessionId('claude', { payload: 7 })).toBeNull() + }) +}) diff --git a/src/main/agent-hooks/server.ts b/src/main/agent-hooks/server.ts index b301e99d6df..7bb86a34142 100644 --- a/src/main/agent-hooks/server.ts +++ b/src/main/agent-hooks/server.ts @@ -32,6 +32,7 @@ import { resolveCachedClaudeCompactOwnership, resolveHookSource, preparePendingGrokResultDiscovery, + readHookBodyProviderSessionId, seedClaudeLeadTurnFromPersistedStatus, seedClaudeSubagentRosterFromSnapshots, seedCodexStateFromSnapshot, @@ -91,6 +92,10 @@ import { type AgentProviderSessionMetadata } from '../../shared/agent-session-resume' import { isCommandCodeNewTurnWhileWorking } from '../../shared/command-code-turn-boundary' +import { + AgentSessionPaneBindings, + type AgentSessionPaneBinding +} from './agent-session-pane-bindings' export type { AgentHookSource } @@ -703,6 +708,9 @@ export class AgentHookServer { }) // Why: hydrated rows give UI continuity but aren't evidence of live agent work in this runtime. private runtimeObservedStatusPaneKeys = new Set() + // Why: a daemon-hosted session's hooks post the daemon's inherited pane key. + // Spawn-time session pins are the only evidence of the true pane. + private agentSessionPaneBindings = new AgentSessionPaneBindings() private hydratedAuthorityCommitments: readonly AgentHookAuthorityEvidence[] = Object.freeze([]) private hydratedLaunchTokenHashByPaneKey = new Map() private persistedAuthorityCommitmentsByPaneKey = new Map() @@ -1959,6 +1967,66 @@ export class AgentHookServer { return changed } + /** Records the pane a freshly spawned agent session belongs to. Called from + * the spawn path, which is the last place both halves are known: the session + * id Orca minted into the launch command, and the pane it launched into. */ + bindAgentSessionPane( + source: AgentHookSource, + sessionId: string, + binding: AgentSessionPaneBinding + ): void { + if (!isValidPaneKey(binding.paneKey)) { + return + } + this.agentSessionPaneBindings.bind(source, sessionId, binding) + } + + clearAgentSessionPaneBindingsForPty(ptyId: string): void { + this.agentSessionPaneBindings.clearForPty(ptyId) + } + + /** The pane a session was spawned into, when it differs from the key the + * event posted. Null means "no correction" — an unpinned session (a + * hand-typed `claude`, an agent Orca never launched) keeps today's behavior, + * so this can only ever move status ONTO a pane Orca itself started. */ + private resolveBoundPaneOverride( + source: AgentHookSource, + sessionId: string | null, + postedPaneKey: unknown + ): AgentSessionPaneBinding | null { + const bound = this.agentSessionPaneBindings.resolve(source, sessionId) + if (!bound || bound.paneKey === postedPaneKey) { + return null + } + return bound + } + + private normalizeHookBodyAgentSessionPane(source: AgentHookSource, body: unknown): unknown { + if (typeof body !== 'object' || body === null) { + return body + } + const record = body as Record + const bound = this.resolveBoundPaneOverride( + source, + readHookBodyProviderSessionId(source, body), + typeof record.paneKey === 'string' ? record.paneKey.trim() : '' + ) + if (!bound) { + return body + } + // Why: the posted key came from the worker's inherited env; the spawn-time + // binding names the pane that actually owns this session. Rewrite tabId + // with it too, or normalizeHookPayload's tabId/paneKey agreement check + // rejects the event outright — and the worktree, since the same inherited + // env named the daemon's workspace. + return { + ...record, + paneKey: bound.paneKey, + tabId: parsePaneKey(bound.paneKey)?.tabId, + ...(bound.worktreeId ? { worktreeId: bound.worktreeId } : {}) + } + } + private normalizeHookBodyPaneKeyAlias(body: unknown): unknown { if (typeof body !== 'object' || body === null) { return body @@ -2147,7 +2215,18 @@ export class AgentHookServer { } // Why: trim paneKey to match the HTTP path, else remote-vs-local events for one pane diverge. const physicalPaneKey = envelope.paneKey.trim() - const paneKey = this.resolvePaneKeyAlias(physicalPaneKey) + const source = isAgentHookSource(envelope.source) ? envelope.source : undefined + // Why: a remote host's shared agent daemon inherits a stale ORCA_PANE_KEY + // exactly like a local one, so SSH/WSL panes need the same spawn-time + // session binding, resolved before pane-migration aliasing applies on top. + const boundPane = source + ? this.resolveBoundPaneOverride( + source, + normalizeAgentProviderSession(envelope.providerSession)?.id ?? null, + physicalPaneKey + ) + : null + const paneKey = this.resolvePaneKeyAlias(boundPane?.paneKey ?? physicalPaneKey) const parsedPaneKey = parsePaneKey(paneKey) if (paneKey.length === 0) { track('agent_hook_unattributed', { reason: 'empty_pane_key' }) @@ -2182,7 +2261,6 @@ export class AgentHookServer { typeof envelope.hookEventName === 'string' && envelope.hookEventName.trim().length > 0 ? envelope.hookEventName.trim() : undefined - const source = isAgentHookSource(envelope.source) ? envelope.source : undefined const providerPromptId = source === 'claude' ? normalizeClaudePromptId(envelope.providerPromptId) : undefined const compactTrigger = @@ -2197,10 +2275,13 @@ export class AgentHookServer { if (statusDisposition === 'suppress') { return } + // Why: a corrected pane brings its own workspace — the envelope's came from + // the same inherited env that named the wrong pane. const worktreeId = - envelope.worktreeId !== undefined && envelope.worktreeId.trim().length > 0 + boundPane?.worktreeId ?? + (envelope.worktreeId !== undefined && envelope.worktreeId.trim().length > 0 ? envelope.worktreeId.trim() - : undefined + : undefined) const promptInteractionKey = typeof envelope.promptInteractionKey === 'string' && envelope.promptInteractionKey.trim().length > 0 @@ -2399,7 +2480,10 @@ export class AgentHookServer { } trackEmptyPaneKeyHook(body) - const aliasedBody = this.normalizeHookBodyPaneKeyAlias(body) + // Why: session attribution runs first — it corrects an inherited key to + // the real pane, and pane-migration aliasing then applies on top of it. + const attributedBody = this.normalizeHookBodyAgentSessionPane(source, body) + const aliasedBody = this.normalizeHookBodyPaneKeyAlias(attributedBody) const normalized = this.normalizeLocalHookPayload(source, aliasedBody) const statusDisposition = normalized.event ? this.getAgentStatusDisposition(normalized.event.paneKey, { diff --git a/src/main/ipc/pty-ipc-mock-registry.ts b/src/main/ipc/pty-ipc-mock-registry.ts index 4a1b48972be..c99c0e6f6a6 100644 --- a/src/main/ipc/pty-ipc-mock-registry.ts +++ b/src/main/ipc/pty-ipc-mock-registry.ts @@ -37,6 +37,8 @@ export const unregisterPtyMock: Mock = vi.fn() export const setMigrationUnsupportedPtyMock: Mock = vi.fn() export const clearMigrationUnsupportedPtyMock: Mock = vi.fn() export const clearMigrationUnsupportedPtysForPaneKeyMock: Mock = vi.fn() +export const bindAgentSessionPaneMock: Mock = vi.fn() +export const clearAgentSessionPaneBindingsForPtyMock: Mock = vi.fn() export const clearPaneKeyAliasesForPtyMock: Mock = vi.fn() export const recordCodexPaneAccountMock: Mock = vi.fn() export const forgetCodexPaneAccountMock: Mock = vi.fn() @@ -117,7 +119,9 @@ export const agentHookServerModuleMock = () => ({ buildPtyEnv: buildAgentHookEnvMock, clearPaneState: clearAgentHookPaneStateMock, registerPaneKeyAlias: registerPaneKeyAliasMock, - clearPaneKeyAliasesForPty: clearPaneKeyAliasesForPtyMock + clearPaneKeyAliasesForPty: clearPaneKeyAliasesForPtyMock, + bindAgentSessionPane: bindAgentSessionPaneMock, + clearAgentSessionPaneBindingsForPty: clearAgentSessionPaneBindingsForPtyMock } }) diff --git a/src/main/ipc/pty-ipc-suite-environment.ts b/src/main/ipc/pty-ipc-suite-environment.ts index d1be8000551..ddf9e1a4787 100644 --- a/src/main/ipc/pty-ipc-suite-environment.ts +++ b/src/main/ipc/pty-ipc-suite-environment.ts @@ -32,6 +32,8 @@ import { setMigrationUnsupportedPtyMock, clearMigrationUnsupportedPtyMock, clearMigrationUnsupportedPtysForPaneKeyMock, + bindAgentSessionPaneMock, + clearAgentSessionPaneBindingsForPtyMock, clearPaneKeyAliasesForPtyMock, recordCodexPaneAccountMock, forgetCodexPaneAccountMock, @@ -123,6 +125,8 @@ export function createPtyIpcSuiteEnvironment(): PtyIpcSuiteEnvironment { setMigrationUnsupportedPtyMock.mockReset() clearMigrationUnsupportedPtyMock.mockReset() clearMigrationUnsupportedPtysForPaneKeyMock.mockReset() + bindAgentSessionPaneMock.mockReset() + clearAgentSessionPaneBindingsForPtyMock.mockReset() clearPaneKeyAliasesForPtyMock.mockReset() recordCodexPaneAccountMock.mockReset() forgetCodexPaneAccountMock.mockReset() diff --git a/src/main/ipc/pty-login-shell-startup-commands.test.ts b/src/main/ipc/pty-login-shell-startup-commands.test.ts index bd74321fe89..0c548093788 100644 --- a/src/main/ipc/pty-login-shell-startup-commands.test.ts +++ b/src/main/ipc/pty-login-shell-startup-commands.test.ts @@ -2,7 +2,8 @@ import { describe, expect, it, vi } from 'vitest' import { loginPreflightExecFileMock, spawnMock, - openCodeBuildPtyEnvMock + openCodeBuildPtyEnvMock, + bindAgentSessionPaneMock } from './pty-ipc-mock-registry' import { posixOnlyIt } from './pty-ipc-test-constants' import { setupPtyIpcSuite } from './pty-ipc-test-harness' @@ -229,7 +230,60 @@ describe('registerPtyHandlers', () => { mockProc.emitData('\x1b]133;A\x07% ') await Promise.resolve() vi.runAllTimers() - expect(mockProc.proc.write).toHaveBeenCalledWith('claude\n') + // Why the pin: a Claude launch carries a minted --session-id so a + // daemon-hosted session's hooks can still be traced to this pane (#9236). + expect(mockProc.proc.write).toHaveBeenCalledTimes(1) + expect(mockProc.proc.write.mock.calls[0][0]).toMatch( + /^claude --session-id [0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\n$/ + ) + } finally { + vi.useRealTimers() + } + } + ) + posixOnlyIt( + 'binds the minted session id to the spawning pane, so hooks can be traced back to it', + async () => { + // Why this asserts the SPAWN side: the hook server's own tests bind by + // hand, so without this the wiring that actually records the pane — + // the only thing that makes the correction fire in production — is + // deletable with every suite still green. + vi.useFakeTimers() + const mockProc = createMockProc() + spawnMock.mockReturnValue(mockProc.proc) + const tabId = 'tab-1' + const leafId = 'aaaaaaaa-1111-4111-8111-111111111111' + const paneKey = `${tabId}:${leafId}` + + try { + registerPtyHandlers(mainWindow as never) + await handlers.get('pty:spawn')!(null, { + cols: 80, + rows: 24, + cwd: '/tmp', + command: 'claude', + tabId, + leafId, + worktreeId: 'wt-real', + env: { ORCA_PANE_KEY: paneKey, ORCA_TAB_ID: tabId, ORCA_WORKTREE_ID: 'wt-real' } + }) + + mockProc.emitData('last login: today\r\n') + vi.runOnlyPendingTimers() + mockProc.emitData('\x1b]133;A\x07% ') + await Promise.resolve() + vi.runAllTimers() + + const written = String(mockProc.proc.write.mock.calls[0][0]) + const mintedId = /--session-id ([0-9a-f-]{36})/.exec(written)?.[1] + expect(mintedId).toBeDefined() + // The id on the command line and the id recorded against the pane must + // be the same one, or the correction can never resolve. + expect(bindAgentSessionPaneMock).toHaveBeenCalledWith( + 'claude', + mintedId, + expect.objectContaining({ paneKey, worktreeId: 'wt-real' }) + ) } finally { vi.useRealTimers() } diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 3c3e148ffb2..0414cd514fa 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -118,6 +118,9 @@ import { markClaudePtyExited, markClaudePtySpawned } from '../claude-accounts/live-pty-gate' +import { pinClaudeLaunchSessionId } from '../../shared/claude-session-pin-launch-command' +import type { AgentStartupShell } from '../../shared/tui-agent-startup-shell' +import { resolveSpawnStartupShell } from '../pty/spawn-startup-shell' import { ensureLinuxTerminalOrcaCliShimDir } from '../cli/linux-terminal-orca-cli-shim' import { isLegacyTerminalShimPathEntry, @@ -1996,6 +1999,31 @@ function isClaudeLaunchCommand(command: string | undefined): boolean { ) } +type ClaudeSessionPinPlan = { command: string | undefined; sessionId: string | null } + +/** Mints a session id into a Claude launch so hook events can be traced back to + * the pane Orca spawned them into. + * + * Why: Claude Code >= 2.1.206 hosts TUI sessions as workers under a shared + * daemon, and the daemon forwards only its own allowlisted env — so a worker's + * `ORCA_PANE_KEY` names whichever pane first started the daemon, not the pane + * the user is looking at (#9236). The session id is the only pane-independent + * identity a hook payload carries, so binding it here is what lets the hook + * server route status to the true pane. Pinning is best-effort by design: a + * command it cannot splice safely is left untouched and keeps today's behavior. + */ +function planClaudeSessionPin( + command: string | undefined, + shell: AgentStartupShell +): ClaudeSessionPinPlan { + if (command === undefined || !isClaudeLaunchCommand(command)) { + return { command, sessionId: null } + } + const sessionId = randomUUID() + const pinned = pinClaudeLaunchSessionId(command, sessionId, shell) + return pinned ? { command: pinned, sessionId } : { command, sessionId: null } +} + function routesFreshSpawnsToLocalProvider(provider: IPtyProvider): boolean { return provider.routesFreshSpawnsToLocalProvider === true } @@ -2123,6 +2151,9 @@ export function clearProviderPtyState( piTitlebarExtensionService.clearPty(id) // Why: SSH exit/teardown paths bypass pty.ts's local onExit but still must release Claude account-switch guards. markClaudePtyExited(id) + // Why: a dead PTY's session pin must not re-route a later session onto a pane + // it no longer runs. + agentHookServer.clearAgentSessionPaneBindingsForPty(id) ptySizes.delete(id) ptyIncarnationById.delete(id) lastInputAtByPty.delete(id) @@ -4653,7 +4684,18 @@ export function registerPtyHandlers( const codexResumeHome = codexResumeLaunch.codexResumeHome // Why: the drop still applies here, but this controller's result has no field for // notifyResumeUnavailable — runtime/relay panes start fresh without the notice. - const launchCommand = codexResumeLaunch.command + // Why not gated on isClaudeLaunch: that flag is local-only (it guards + // account switching); a remote host's daemon inherits a stale pane key too. + const claudeSessionPin = planClaudeSessionPin( + codexResumeLaunch.command, + resolveSpawnStartupShell({ + connectionId: args.connectionId, + windowsWslDistro: terminalRuntimeOptions.terminalWindowsWslDistro, + shellOverride: daemonShellOverride, + platform: process.platform + }) + ) + const launchCommand = claudeSessionPin.command const claudeAuth = isClaudeLaunch && prepareClaudeAuth ? await prepareClaudeAuth(codexSelectionTarget) : null if (isClaudeLaunch && isClaudeAuthSwitchInProgress()) { @@ -5423,6 +5465,13 @@ export function registerPtyHandlers( } // Why: runtime-owned CLI PTYs bypass the renderer pty:spawn handler; record paneKey here too since hook titles and cache cleanup need this reverse lookup. const paneKey = rememberPaneKeyForPty(result.id, env?.ORCA_PANE_KEY) + if (claudeSessionPin.sessionId && paneKey) { + agentHookServer.bindAgentSessionPane('claude', claudeSessionPin.sessionId, { + paneKey, + ptyId: result.id, + worktreeId: args.worktreeId + }) + } const pendingSerializer = paneKey ? pendingByPaneKey.get(paneKey) : undefined const inheritRendererReadiness = result.isReattach === true && @@ -6438,7 +6487,18 @@ export function registerPtyHandlers( ? await resolveCodexResumeLaunch(args.command, codexResumePreparation) : noCodexResumeLaunch(preAdoptedStablePane ? undefined : args.command) const codexResumeHome = codexResumeLaunch.codexResumeHome - const launchCommand = codexResumeLaunch.command + // Why not gated on isClaudeLaunch: that flag is local-only (it guards + // account switching); a remote host's daemon inherits a stale pane key too. + const claudeSessionPin = planClaudeSessionPin( + codexResumeLaunch.command, + resolveSpawnStartupShell({ + connectionId: args.connectionId, + windowsWslDistro: terminalRuntimeOptions.terminalWindowsWslDistro, + shellOverride: initialShellOverride, + platform: process.platform + }) + ) + const launchCommand = claudeSessionPin.command baseEnv = stripSequencedStartupResumeArgv(baseEnv, codexResumeLaunch) // Why: declared after the strip so a local-provider spawn cannot capture the // pre-strip env — only the daemon branch below re-derives this from baseEnv. @@ -7090,6 +7150,13 @@ export function registerPtyHandlers( const rememberedPaneKey = validatedPaneKey ? rememberPaneKeyForPty(result.id, validatedPaneKey) : null + if (claudeSessionPin.sessionId && rememberedPaneKey) { + agentHookServer.bindAgentSessionPane('claude', claudeSessionPin.sessionId, { + paneKey: rememberedPaneKey, + ptyId: result.id, + worktreeId: args.worktreeId + }) + } if (legacySpawnPaneKey && migrationUnsupportedPaneKey) { agentHookServer.registerPaneKeyAlias( legacySpawnPaneKey.paneKey, diff --git a/src/main/pty/spawn-startup-shell.test.ts b/src/main/pty/spawn-startup-shell.test.ts new file mode 100644 index 00000000000..a819dceab17 --- /dev/null +++ b/src/main/pty/spawn-startup-shell.test.ts @@ -0,0 +1,57 @@ +import { describe, expect, it } from 'vitest' + +import { resolveSpawnStartupShell } from './spawn-startup-shell' + +const base = { + connectionId: null, + windowsWslDistro: null, + shellOverride: undefined, + platform: 'darwin' as NodeJS.Platform +} + +describe('resolveSpawnStartupShell', () => { + it('uses posix on macOS and Linux', () => { + expect(resolveSpawnStartupShell(base)).toBe('posix') + expect(resolveSpawnStartupShell({ ...base, platform: 'linux' })).toBe('posix') + }) + + it('uses posix for an SSH pane even when the client is on Windows', () => { + expect(resolveSpawnStartupShell({ ...base, platform: 'win32', connectionId: 'conn-1' })).toBe( + 'posix' + ) + }) + + it('uses posix for a WSL pane on a Windows host', () => { + expect( + resolveSpawnStartupShell({ ...base, platform: 'win32', windowsWslDistro: 'Ubuntu' }) + ).toBe('posix') + }) + + it('defaults a native Windows pane to powershell', () => { + expect(resolveSpawnStartupShell({ ...base, platform: 'win32' })).toBe('powershell') + expect( + resolveSpawnStartupShell({ + ...base, + platform: 'win32', + shellOverride: 'C:\\Program Files\\PowerShell\\7\\pwsh.exe' + }) + ).toBe('powershell') + }) + + it('recognizes cmd and Git Bash overrides on Windows', () => { + expect( + resolveSpawnStartupShell({ + ...base, + platform: 'win32', + shellOverride: 'C:\\Windows\\System32\\cmd.exe' + }) + ).toBe('cmd') + expect( + resolveSpawnStartupShell({ + ...base, + platform: 'win32', + shellOverride: 'C:\\Program Files\\Git\\bin\\bash.exe' + }) + ).toBe('posix') + }) +}) diff --git a/src/main/pty/spawn-startup-shell.ts b/src/main/pty/spawn-startup-shell.ts new file mode 100644 index 00000000000..9c9d49a5dc9 --- /dev/null +++ b/src/main/pty/spawn-startup-shell.ts @@ -0,0 +1,25 @@ +import type { AgentStartupShell } from '../../shared/tui-agent-startup-shell' + +/** The dialect that parses a spawn's launch command. + * + * Why not just `process.platform`: a WSL pane and an SSH pane both run a POSIX + * shell while Orca itself is on Windows, and a Windows pane can be pointed at + * cmd or Git Bash instead of PowerShell. Callers that splice into the command + * text must model the shell that will actually parse it. + */ +export function resolveSpawnStartupShell(opts: { + connectionId: string | null | undefined + windowsWslDistro: string | null | undefined + shellOverride: string | undefined + platform: NodeJS.Platform +}): AgentStartupShell { + if (opts.connectionId || opts.windowsWslDistro || opts.platform !== 'win32') { + return 'posix' + } + const base = + opts.shellOverride?.trim().replaceAll('\\', '/').split('/').pop()?.toLowerCase() ?? '' + if (/^(?:ba|z|k|da|)sh(?:\.exe)?$/.test(base)) { + return 'posix' + } + return base.startsWith('cmd') ? 'cmd' : 'powershell' +} diff --git a/src/shared/agent-hook-listener.ts b/src/shared/agent-hook-listener.ts index 61202afec0e..933e3de10d8 100644 --- a/src/shared/agent-hook-listener.ts +++ b/src/shared/agent-hook-listener.ts @@ -4349,6 +4349,44 @@ function readStringField(record: Record, key: string): string | return trimmed.length > 0 ? trimmed : undefined } +/** + * The provider session id a raw hook body reports, read exactly as + * `normalizeHookPayload` reads it (same JSON guard, same per-source extractor, + * same Codex child carve-out). + * + * Why it is exposed: pane attribution has to run BEFORE normalization. + * `normalizeHookPayload` keys its prompt/compaction state machine on the posted + * pane key, so correcting the key afterwards would leave that state on the + * wrong pane. Callers that re-derive the session id themselves drift from this + * parser the moment a source changes shape. + */ +export function readHookBodyProviderSessionId( + source: AgentHookSource, + body: unknown +): string | null { + if (typeof body !== 'object' || body === null) { + return null + } + const rawPayload = (body as Record).payload + let hookPayload: unknown = rawPayload + if (typeof rawPayload === 'string') { + try { + hookPayload = parseAgentHookJson(rawPayload) + } catch { + return null + } + } + if (typeof hookPayload !== 'object' || hookPayload === null) { + return null + } + const record = hookPayload as Record + // Why: Codex child hooks expose the child's session_id on the parent's pane. + if (source === 'codex' && readString(record, 'agent_id')) { + return null + } + return extractAgentProviderSession(source, record)?.id ?? null +} + export function normalizeHookPayload( state: HookListenerState, source: AgentHookSource, diff --git a/src/shared/agent-resume-launch-command.ts b/src/shared/agent-resume-launch-command.ts index cda40878c28..a9a7ac5d5b0 100644 --- a/src/shared/agent-resume-launch-command.ts +++ b/src/shared/agent-resume-launch-command.ts @@ -1,6 +1,7 @@ import type { ResumableTuiAgent } from './agent-session-resume' +import { findClaudeExecutableIndex } from './claude-launch-executable-token' import { - isPosixStartupShell, + isFullyModelableStartupCommand, quoteStartupArg, tokenizeStartupCommand, type AgentStartupShell @@ -20,40 +21,6 @@ function isClaudeResumeSelector(token: string): boolean { return token === '-r' || token.startsWith('-r=') || token === '-c' || token.startsWith('-c=') } -function isClaudeExecutableToken(token: string): boolean { - const base = token.split(/[\\/]/).pop() ?? '' - return /^claude(\.(exe|cmd|bat|ps1))?$/i.test(base) -} - -/** Accepts a claude token only in command position — index 0, right after a - * wrapper's `--`, behind PowerShell's `&` call operator, or preceded solely by - * NAME=value assignments — so an argument that merely ends in /claude (an ssh - * key, a project dir) can never be mistaken for the executable. */ -function findClaudeExecutableIndex(tokens: readonly string[], shell: AgentStartupShell): number { - let commandPosition = true - for (let i = 0; i < tokens.length; i += 1) { - const token = tokens[i] - if (commandPosition) { - if (isClaudeExecutableToken(token)) { - return i - } - if ( - // Why: `NAME=value cmd` is sh-family syntax (fish included, 3.1+); on - // cmd/PowerShell such a token is a bogus executable name, not a prefix. - (isPosixStartupShell(shell) && /^[A-Za-z_][A-Za-z0-9_]*=/.test(token)) || - (shell === 'powershell' && token === '&' && i === 0) - ) { - continue - } - commandPosition = false - } - if (token === '--') { - commandPosition = true - } - } - return -1 -} - /** Joins the resolved base command with the agent's resume argv. Claude goes * through the selector guard below; other agents keep plain appending. */ export function buildAgentResumeLaunchCommand( @@ -100,33 +67,8 @@ export function buildClaudeResumeLaunchCommand( if (claudeIndex === -1) { return appended } - // Why: any token the tokenizer cannot model for this shell — an operator, - // comment, expansion, or cmd single-quoted region — means the splice could - // cut live syntax or misread a literal as a selector. The whole base must - // be modelable, including the executable itself; only PowerShell's leading - // call operator is a known-safe divergent token. - for (let i = 0; i <= tokens.length; i += 1) { - const gapStart = i === 0 ? 0 : spans[i - 1].end - const gapEnd = i === tokens.length ? baseCommand.length : spans[i].start - if (!/^[ \t]*$/.test(baseCommand.slice(gapStart, gapEnd))) { - return appended - } - if (i === tokens.length) { - break - } - // Why: a bare `--%` makes PowerShell pass the rest of the line to the - // child literally, so appended quoting would arrive as literal bytes. A - // quoted `--%` can also stop parsing, but only before a parameter token, - // where the base is already mangled with or without the guard. - if (shell === 'powershell' && baseCommand.slice(spans[i].start, spans[i].end) === '--%') { - return appended - } - if (spans[i].divergesFromShell) { - const isCallOperator = shell === 'powershell' && i === 0 && tokens[i] === '&' - if (!isCallOperator) { - return appended - } - } + if (!isFullyModelableStartupCommand(baseCommand, tokens, spans, shell)) { + return appended } const cuts: { start: number; end: number }[] = [] let terminatorStart: number | null = null diff --git a/src/shared/claude-launch-executable-token.ts b/src/shared/claude-launch-executable-token.ts new file mode 100644 index 00000000000..664efd0d53f --- /dev/null +++ b/src/shared/claude-launch-executable-token.ts @@ -0,0 +1,38 @@ +import { isPosixStartupShell, type AgentStartupShell } from './tui-agent-startup-shell' + +export function isClaudeExecutableToken(token: string): boolean { + const base = token.split(/[\\/]/).pop() ?? '' + return /^claude(\.(exe|cmd|bat|ps1))?$/i.test(base) +} + +/** Accepts a claude token only in command position — index 0, right after a + * wrapper's `--`, behind PowerShell's `&` call operator, or preceded solely by + * NAME=value assignments — so an argument that merely ends in /claude (an ssh + * key, a project dir) can never be mistaken for the executable. */ +export function findClaudeExecutableIndex( + tokens: readonly string[], + shell: AgentStartupShell +): number { + let commandPosition = true + for (let i = 0; i < tokens.length; i += 1) { + const token = tokens[i] + if (commandPosition) { + if (isClaudeExecutableToken(token)) { + return i + } + if ( + // Why: `NAME=value cmd` is sh-family syntax (fish included, 3.1+); on + // cmd/PowerShell such a token is a bogus executable name, not a prefix. + (isPosixStartupShell(shell) && /^[A-Za-z_][A-Za-z0-9_]*=/.test(token)) || + (shell === 'powershell' && token === '&' && i === 0) + ) { + continue + } + commandPosition = false + } + if (token === '--') { + commandPosition = true + } + } + return -1 +} diff --git a/src/shared/claude-session-pin-launch-command.test.ts b/src/shared/claude-session-pin-launch-command.test.ts new file mode 100644 index 00000000000..16e310c608d --- /dev/null +++ b/src/shared/claude-session-pin-launch-command.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, it } from 'vitest' + +import { pinClaudeLaunchSessionId } from './claude-session-pin-launch-command' + +const SESSION = '0192a4b1-1111-4111-8111-aaaaaaaaaaaa' + +describe('pinClaudeLaunchSessionId', () => { + it('pins a bare launch', () => { + expect(pinClaudeLaunchSessionId('claude', SESSION, 'posix')).toBe( + `claude --session-id ${SESSION}` + ) + }) + + it('pins path-qualified and Windows executables', () => { + expect(pinClaudeLaunchSessionId('/usr/local/bin/claude --model opus', SESSION, 'posix')).toBe( + `/usr/local/bin/claude --session-id ${SESSION} --model opus` + ) + expect(pinClaudeLaunchSessionId('claude.cmd', SESSION, 'cmd')).toBe( + `claude.cmd --session-id ${SESSION}` + ) + expect(pinClaudeLaunchSessionId('& claude', SESSION, 'powershell')).toBe( + `& claude --session-id ${SESSION}` + ) + }) + + it('pins behind posix env assignments, which are still command position', () => { + expect(pinClaudeLaunchSessionId('FOO=1 claude', SESSION, 'posix')).toBe( + `FOO=1 claude --session-id ${SESSION}` + ) + }) + + it('keeps the pin in option position, before claude’s own -- terminator', () => { + expect(pinClaudeLaunchSessionId('claude -- --weird', SESSION, 'posix')).toBe( + `claude --session-id ${SESSION} -- --weird` + ) + }) + + // Why: `--session-id` is a ROOT option. `claude mcp list --session-id ` + // exits with "error: unknown option '--session-id'", so a trailing pin would + // break a launch that works today — the pin must precede the subcommand. + it.each([ + ['mcp list', 'claude mcp list'], + ['doctor', 'claude doctor'], + ['update', 'claude update'], + ['setup-token', 'claude setup-token'], + ['install stable', 'claude install stable'] + ])('pins ahead of the %s subcommand, never after it', (_label, command) => { + const rest = command.slice('claude'.length) + expect(pinClaudeLaunchSessionId(command, SESSION, 'posix')).toBe( + `claude --session-id ${SESSION}${rest}` + ) + }) + + it('preserves the base bytes verbatim, including quoting', () => { + expect(pinClaudeLaunchSessionId('claude "fix the bug"', SESSION, 'posix')).toBe( + `claude --session-id ${SESSION} "fix the bug"` + ) + }) + + it('leaves a selector that follows claude’s own -- terminator alone', () => { + // Why: past `--` the tokens are the child's argv, not claude flags, so they + // are not a competing selector and must not suppress the pin. + expect(pinClaudeLaunchSessionId('claude -- --resume abc', SESSION, 'posix')).toBe( + `claude --session-id ${SESSION} -- --resume abc` + ) + }) + + it.each([ + ['--session-id', `claude --session-id ${SESSION}`], + ['--session-id=', `claude --session-id=${SESSION}`], + ['--resume', 'claude --resume abc'], + ['--resume=', 'claude --resume=abc'], + ['--continue', 'claude --continue'], + ['--continue=', 'claude --continue=abc'], + ['--fork-session', 'claude --fork-session'], + ['-r', 'claude -r abc'], + ['-r=', 'claude -r=abc'], + ['-c', 'claude -c'], + ['-c=', 'claude -c=abc'] + ])('refuses to compete with an existing %s selector', (_label, command) => { + expect(pinClaudeLaunchSessionId(command, SESSION, 'posix')).toBeNull() + }) + + it.each([ + ['compound', 'claude && echo done'], + ['sequenced', 'claude; echo done'], + ['piped', 'claude | tee log'], + ['redirected', 'claude > out.txt'], + ['wrapper-launched', 'nvm exec claude'], + ['not claude at all', 'codex'], + ['a path that merely ends in claude', 'ssh -i ~/.ssh/claude host'] + ])('refuses to pin a %s command', (_label, command) => { + expect(pinClaudeLaunchSessionId(command, SESSION, 'posix')).toBeNull() + }) + + it('refuses a PowerShell --% stop-parsing command', () => { + expect(pinClaudeLaunchSessionId('claude --% --raw', SESSION, 'powershell')).toBeNull() + }) + + it('refuses a session id that is not a UUID', () => { + expect(pinClaudeLaunchSessionId('claude', 'not-a-uuid', 'posix')).toBeNull() + expect(pinClaudeLaunchSessionId('claude', '', 'posix')).toBeNull() + }) +}) diff --git a/src/shared/claude-session-pin-launch-command.ts b/src/shared/claude-session-pin-launch-command.ts new file mode 100644 index 00000000000..1d53d979d63 --- /dev/null +++ b/src/shared/claude-session-pin-launch-command.ts @@ -0,0 +1,86 @@ +import { findClaudeExecutableIndex } from './claude-launch-executable-token' +import { + isFullyModelableStartupCommand, + tokenizeStartupCommand, + type AgentStartupShell +} from './tui-agent-startup-shell' + +/** Every flag that makes the CLI choose its own session identity. If the user's + * command already carries one, Orca must not add a competing `--session-id`. */ +function isClaudeSessionSelector(token: string): boolean { + return ( + token === '--session-id' || + token.startsWith('--session-id=') || + token === '--resume' || + token.startsWith('--resume=') || + token === '--continue' || + token.startsWith('--continue=') || + token === '--fork-session' || + token === '-r' || + token.startsWith('-r=') || + token === '-c' || + token.startsWith('-c=') + ) +} + +const SESSION_ID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/ + +/** + * Splices `--session-id ` into a Claude launch so the host keeps a + * process-independent handle on the session it is about to start. + * + * Why this exists: Claude Code >= 2.1.206 hosts TUI sessions as workers under a + * shared daemon, and the daemon forwards only its own allowlisted env — so a + * pane's `ORCA_PANE_KEY` never reaches the worker that runs the hooks. The + * session id is the one identity that does survive into the hook payload, so + * pinning it at spawn is what lets the host recover the true pane later. + * + * Fails CLOSED — returns null rather than a best-effort command. Pinning is an + * attribution improvement, never a launch requirement, so any doubt about the + * command's shape (untokenizable, wrapper-launched, compound, redirected, + * already session-selecting) leaves the user's command byte-for-byte alone. + * Bytes outside the insertion point are always preserved verbatim; the command + * is spliced by source span and never re-quoted. + */ +export function pinClaudeLaunchSessionId( + baseCommand: string, + sessionId: string, + shell: AgentStartupShell +): string | null { + if (!SESSION_ID_RE.test(sessionId)) { + return null + } + const tokenized = tokenizeStartupCommand(baseCommand, shell) + if (!tokenized.ok) { + return null + } + const { tokens, spans } = tokenized + const claudeIndex = findClaudeExecutableIndex(tokens, shell) + if (claudeIndex === -1) { + return null + } + // Why: modelability is what rules out compounds, pipelines and redirects — + // their operators tokenize as shell-divergent, so `claude && deploy` can + // never take a pin that would land on the wrong word. + if (!isFullyModelableStartupCommand(baseCommand, tokens, spans, shell)) { + return null + } + for (let i = claudeIndex + 1; i < tokens.length; i += 1) { + const token = tokens[i] + // Why: `--` is claude's own terminator, so anything after it is the child's + // argv, not a claude flag — a selector there is not ours to compete with. + if (token === '--') { + break + } + if (isClaudeSessionSelector(token)) { + return null + } + } + // Why immediately after the executable rather than appended: `--session-id` is + // a ROOT option, and `claude ` (`mcp list`, `doctor`, `update`) + // rejects unknown options, so a trailing pin makes those launches fail + // outright. Root position is also before claude's own `--` terminator, which + // a trailing pin would fall past. + const insertAt = spans[claudeIndex].end + return `${baseCommand.slice(0, insertAt)} --session-id ${sessionId}${baseCommand.slice(insertAt)}` +} diff --git a/src/shared/tui-agent-startup-shell.ts b/src/shared/tui-agent-startup-shell.ts index 94df0f80ac5..d6dcd32ff33 100644 --- a/src/shared/tui-agent-startup-shell.ts +++ b/src/shared/tui-agent-startup-shell.ts @@ -162,6 +162,45 @@ export function tokenizeStartupCommand( : tokenizeCustomCommandTemplate(value) } +/** True when every byte of `value` is accounted for by `tokens`/`spans` under + * `shell` — the precondition for splicing an argument in by source span. + * + * Why: any token the tokenizer cannot model for this shell — an operator, + * comment, expansion, or cmd single-quoted region — means a splice could cut + * live syntax or misread a literal as a flag. The whole command must be + * modelable, including the executable itself; only PowerShell's leading call + * operator is a known-safe divergent token. A bare `--%` makes PowerShell pass + * the rest of the line to the child literally, so spliced quoting would arrive + * as literal bytes. + */ +export function isFullyModelableStartupCommand( + value: string, + tokens: readonly string[], + spans: readonly CommandTokenSpan[], + shell: AgentStartupShell +): boolean { + for (let i = 0; i <= tokens.length; i += 1) { + const gapStart = i === 0 ? 0 : spans[i - 1].end + const gapEnd = i === tokens.length ? value.length : spans[i].start + if (!/^[ \t]*$/.test(value.slice(gapStart, gapEnd))) { + return false + } + if (i === tokens.length) { + break + } + if (shell === 'powershell' && value.slice(spans[i].start, spans[i].end) === '--%') { + return false + } + if (spans[i].divergesFromShell) { + const isCallOperator = shell === 'powershell' && i === 0 && tokens[i] === '&' + if (!isCallOperator) { + return false + } + } + } + return true +} + export function resolveStartupShell( platform: NodeJS.Platform, shell?: AgentStartupShell From 24e662adc1180abf23d8882733a170cfc8224894 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:40:01 -0700 Subject: [PATCH 03/43] feat(ssh): verify host keys, and restore panes correctly across a reconnect (#14844) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs(ssh): design for real host key verification (STA-4319) Today's ssh2 verifier records a fingerprint and returns true — every host key is accepted, with no known_hosts consult and no change detection anywhere in src/main/ssh/. Scope is per-connection, so exec, SFTP, port forwarding, the watcher and relay deploy all ride that one unverified handshake, and the ProxyJump path puts the final hop — the topology most likely to cross untrusted network — on ssh2 specifically. Decisions worth calling out: - Read the user's known_hosts as a trust source but NEVER write to it. That file is shared with every other SSH tool on the machine; appending means line endings, permissions, concurrent writers and a corruption blast radius well beyond us. Accepted keys go to our own per-target store. Reading theirs is also the entire migration story: most developers already have their hosts there. - Mismatch is scoped to the SAME key type. A host with only an RSA entry that presents ed25519 is unknown, not changed. ssh2 negotiates ed25519 first, so without this we would fire a change-of-key alarm at nearly every existing user on their first upgraded connect — training them to dismiss the one warning that is supposed to mean something. Flagged in review as the decision I am least sure of; a downgrade-vector argument against it is being tested. - Changed key hard-fails with no override button; recovery is a separate explicit action, offered only when OUR store is what disagreed, because forgetting our record cannot unblock a known_hosts conflict. - Background reconnects deny rather than prompt. A dialog the user cannot place in context only teaches click-through. Two traps are documented because either would make the fix silently do nothing: an async verifier returns a Promise, which ssh2 reads as truthy and accepts immediately; and the existing test mock invokes hostVerifier with one argument and ignores the return, so it would pass against a verifier that never decides. Design only — no behaviour change. The doc is added to the tracked-reference allowlist in .gitignore alongside the other docs/reference entries. * docs(ssh): revise the host key design after security and migration review Three things the reviews changed, kept visible rather than quietly edited out. THREAT MODEL WAS WRONG IN THREE PLACES. Jump hosts are not the worst case — they are already safe: shouldUseSystemSshTransport branches on exactly the inputs resolveEffectiveProxy does, and attemptConnect returns after the system probe, so ProxyJump goes through OpenSSH and is verified. Agent forwarding was overstated (gated on the user's ForwardAgent). Credential theft was understated: any auth error counts as agent fallback, so a MITM walks the user to the password AND private-key passphrase prompts, and cachedPassword replays without prompting. The relay claim was backwards — the attacker owns their own machine; the real impact is the return direction, where they become the host our workspace trusts. TYPE SCOPING IS A DOWNGRADE VECTOR WITHOUT ALGORITHM ORDERING. This was the decision I flagged as least certain and asked to have argued both ways. OpenSSH is safe only because order_hostkeyalgs() puts known types first and RFC 4253 gives the client's order priority. ssh2 negotiates ed25519 first regardless, so an attacker who cannot forge the RSA key on file just presents ed25519 and gets a friendly first-contact prompt instead of a hard failure. Keep scoping, but set algorithms.serverHostKey to lead with the types on file — and add a sixth outcome for 'unknown type, known host', which must never read as first contact. SHIP THE DEFENCE BEFORE THE DIALOG. Startup restore fires eager connects for all targets in parallel with a 15s timeout while a prompt would live 120s; ephemeral VM targets present a new key every launch; paired-web connects run on the host desktop, so the dialog opens on someone else's screen. Phase 1 is therefore no modal at all: consult known_hosts and our store, match connects, unknown persists with accept-new semantics, mismatch and revoked hard-fail. That is the whole MITM defence with none of the migration risk. Also folded in, verified live against OpenSSH 10.2p1: the without-port fallback (bracketed lookup first, then bare, where the second pass can only yield match or unknown — otherwise a bare line plus a non-default port produces a spurious prompt); hashed entries hash the candidate form; multiple files union; a cert-authority line does not match a plain key. IPv6 and bracket parsing moved INTO scope — that is a parser requirement, not a scope call, and getting it wrong produces the prompt-training harm the design exists to avoid. * feat(ssh): parse and match OpenSSH known_hosts The matcher half of STA-4319. No behaviour change yet — nothing calls this. Hand-rolled because no maintained JS implementation exists, and written against behaviour observed from OpenSSH 10.2p1 rather than inferred from the man page. Three of those behaviours a reasonable reading gets wrong: - A non-default port is TWO ordered lookups, not one candidate set: '[host]:port' first, then bare host ('checking without port identifier' in ssh -v). The fallback pass can only yield match or unknown — OpenSSH downgrades a wrong key there rather than reporting a change. Collapse them and anyone holding a bare line who connects off-port gets a spurious first-contact result; treat the fallback as authoritative and they get a false change-of-key alarm. - Revocation resolves in its own pass so the verdict cannot depend on line order. Verified both orderings. - A cert-authority line never matches a plain host key; it only validates certificates. A normal line alongside it still decides. Mismatch is scoped to the same key type, and a host known by a DIFFERENT type returns unknown-type-known-host rather than plain unknown — an attacker who cannot forge the key on file must not get a friendly first-contact result by presenting another type. That outcome is only half the defence; the other half (leading serverHostKey with known types) lands with the wiring. 47 tests from vectors executed against real sshd, including ssh-keygen -H hashed entries. Each of six mutations reddens it: collapsing the passes, letting the fallback report mismatch, dropping type scoping, resolving revocation in line order, honouring an unrecognised marker, and skipping the blob/type agreement check. * feat(ssh): decide what to do with a presented host key The policy half of STA-4319, kept separate from the ssh2 wiring so it is testable without a handshake and injected rather than importing its sources, so a test states its own trust state instead of writing files. Phase 1 ships no dialog — a test asserts the decision is never 'prompt'. Startup restore opens every previously-active target at once, ephemeral VM targets would ask every launch, and paired-web connects run on the host desktop where the dialog would appear on someone else's screen. Ordering that matters: revocation outranks everything including StrictHostKeyChecking=no, because a revoked key is a statement that this key is known-bad rather than merely unrecognised. known_hosts is named before our own store on a change, because its remedy (ssh-keygen -R) is the one that also unblocks ssh and git — pointing at a remedy that cannot work is worse than none. Two carve-outs with reasons: an ephemeral runtime target accepts WITHOUT recording, since a fresh VM presents a new key every launch and a stored record would accumulate per launch and eventually read as a spurious change; and when ssh -G ran on the HOME-divergent path that suppresses /etc/ssh/ssh_config, an unknown host is denied, because a site-wide policy may forbid it and being laxer than ssh is the one outcome that is never acceptable. Rejection text deliberately avoids 'authentication failed' and 'permission denied': the reconnect ladder classifies on those substrings, so a denial phrased that way is retried forever against a decision that will never change. Pinned by a test. * feat(ssh): build the host key verifier and the algorithm order that makes it safe Still not wired into the handshake — that lands next. This is the piece that turns a decision into an ssh2 callback, plus the half of the design that is easy to forget because it lives in a different config field. The verifier MUST be a plain function returning undefined. ssh2 does 'const ret = verifier(key, verify); if (ret !== undefined) verify(ret)', so an async function returns a Promise — neither undefined nor falsy — and ssh2 accepts the key immediately while ignoring whatever the callback later decides. Making this async would silently restore exactly the accept-everything behaviour the module exists to remove, so a test asserts the return value is undefined. orderServerHostKeyAlgorithms is what makes type-scoped matching safe rather than a downgrade. RFC 4253 gives the client's algorithm order priority, so leading with the types we already hold for a host denies a server the choice of presenting some other type to convert a hard failure into first contact. Without it, an attacker who cannot forge the key on file just offers a different algorithm. Revoked entries never contribute to that order. Also fails closed on two paths that would otherwise hang or over-trust: a key whose own length-prefixed header cannot be read is refused rather than reasoned about, and a throw from any dependency denies, because ssh2 may not catch an exception raised inside the verifier and the handshake would hang instead of failing. 18 tests. Includes the two negative cases that matter — first-contact keys are recorded, but keys we already know, rejected keys, ephemeral runtime targets and a lax StrictHostKeyChecking are not. * fix(ssh): promote every RSA signature algorithm for a known ssh-rsa key A known_hosts entry names the KEY type, which is not the negotiated ALGORITHM name. One ssh-rsa key is offered as rsa-sha2-512, rsa-sha2-256 or ssh-rsa depending on the signature algorithm, so matching the literal name only would leave a host we know by RSA ordered behind ed25519 — precisely the ordering this function exists to prevent, and precisely the population (RSA-era known_hosts entries) it was written for. Verified from ssh2's own negotiation while wiring this: kex.js iterates the CLIENT list and takes the first entry the server also offers, so client order does decide, as RFC 4253 says. ssh2's default order leads with ed25519 and places the RSA algorithms fifth through seventh. * fix(ssh): verify host keys instead of accepting every one (STA-4319) The actual fix. ssh-connection's verifier recorded a fingerprint and returned true, so every ssh2 connection accepted every host key — no known_hosts consult, no change detection. It now consults the user's known_hosts plus our own store and refuses a changed, revoked or unverifiable key. Phase 1 by design: no dialog. Unknown hosts are accepted and recorded (accept-new semantics), because startup restore opens every previously-active target at once, ephemeral VM targets present a new key each launch, and paired-web connects run on the host desktop where a prompt would appear on someone else's screen. The MITM defence lands now; the prompt is Phase 2. Also sets algorithms.serverHostKey to lead with the types already known for the host. Without it the type-scoped matching is a downgrade — an attacker who cannot forge the key on file just presents another type and turns a hard failure into first contact. Verified from ssh2's kex.js that the client list decides. Denial replaces ssh2's generic handshake error with the specific reason, because the reconnect ladder cannot distinguish a generic failure from a transient fault and would retry forever against a decision that will never change. An unreadable trust store degrades to known_hosts only rather than failing the connect: a changed key is still refused, and a host trusted only by us falls back to first contact and is re-recorded, reaching the same decision. The ssh2 mock now uses the callback form and aborts the handshake on denial. As written it called hostVerifier(key) with one argument and ignored the result, so it would have passed against a verifier that never decides — flagged in the design as a mock that had to change, not a test to quietly rewrite. Two new tests pin the wiring rather than the module: an unidentifiable blob is refused, and a well-formed key is accepted. Note for review: commit 2d2a0880bac unintentionally swept in two modules built concurrently (ssh-known-hosts-source, ssh-host-key-store) because I staged with 'git add -A'; its message describes only the verifier. Both are covered by their own tests, but the attribution in that commit is wrong. 1461 SSH tests pass. * fix(ssh): bind the host key store to the active profile at startup Without this the store reports nothing trusted on every launch. Safe — known_hosts still decides, and a host trusted only by us degrades to first contact and is re-recorded — but it silently discarded our own accept records, so the store the design calls for was not actually in use. Bound beside the profile Store, since it is a sidecar of the same data file. Also records why the paired-web carve-out the migration review asked for is NOT implemented in Phase 1, rather than leaving it looking forgotten. That carve-out exists to stop a web client waiting out the 120s prompt timeout — a hang only reachable if a prompt exists, and Phase 1 has none, which the decision function pins with a test asserting it never returns 'prompt'. An RPC connect therefore behaves exactly like a local one. Adding a fail-fast path now would introduce a failure mode for a hang that cannot occur; it becomes load-bearing when the dialog lands and is listed under Phase 2. Noted there for whoever builds Phase 2: runtime/rpc/methods/ssh.ts already swallows the specific error and rethrows a generic one, so the host-key reason will not reach a web user without a change there too. Full unit suite: 52,352 pass. The 8 failures are the known environment baseline (5 osc8, 2 IME) plus one browser-cookie suite-ordering flake that passes in isolation — none in src/main/ssh, and none related to this change. * fix(ssh): close two downgrades the implementation review found Both were in my wiring, not the design, and two reviewers found the first independently. 1. OUR STORE WAS TYPE-DOWNGRADABLE. The inline lookup filtered by key type first and could only answer match/mismatch/unknown, so a record of a DIFFERENT type for the same endpoint read as "unknown". A host learned on first contact — ed25519, since ssh2 proposes it first — and absent from known_hosts could then be impersonated by presenting RSA: both sources say unknown, so accept-and-remember, silently. That is exactly the downgrade D3 says the design cannot ship without, applied to the records we create ourselves. The store's own isTrusted already computed the right answer and had no production caller. Stored types now also feed the algorithm ordering, without which the guard is only half present. 2. WE KEYED ON THE ORCA LABEL, NOT THE DIALED HOST. "ssh -G" echoes its own argument back as its hostname field when no Host block matches, so for a manual target that field IS the Orca label — the one name D2 forbids keying on, and one ssh never wrote. We consulted no entries at all, so an impersonated host read as first contact. Now keys on the dialed host, which buildConnectConfig has already resolved through HostName, with HostKeyAlias still winning. That inverted an existing test rather than deleting it: "uses the resolved hostname, never the Orca label" encoded an assumption disproved against OpenSSH 10.2p1, so it is renamed and reversed with the reason recorded in the test. 3. NO READABLE SOURCE IS NOT FIRST CONTACT. Every known_hosts file failing to read was indistinguishable from "this host is unknown", so a changed key would be accepted the one time we could not check. The loader now reports how many files it could read, and zero readable sources with an empty store takes the strict path instead of recording trust. 4. A superseded attempt's rejection could replace the live attempt's error, and substituting a new Error drops ssh2's code, so a transient ECONNRESET would stop being classified as retryable. The rejection is now local to its attempt. 5. displayHost was the Orca label, so a mismatch could print "ssh-keygen -R