diff --git a/00-review-context.md b/00-review-context.md new file mode 100644 index 00000000000..23f44d45a12 --- /dev/null +++ b/00-review-context.md @@ -0,0 +1,70 @@ +# Review Context + +## Branch Info + +- Base: origin/main +- Current: brennanb2025/cli-created-terminals-not-showing-agent-status + +## Changed Files Summary + +- M src/main/runtime/orca-runtime.test.ts +- M src/main/runtime/orca-runtime.ts +- M src/main/window/attach-main-window-services.ts +- M src/preload/api-types.ts +- M src/preload/index.ts +- M src/renderer/src/hooks/useIpcEvents.ts +- M src/renderer/src/store/slices/store-cascades.test.ts +- M src/renderer/src/store/slices/terminals.ts +- M src/shared/telemetry-events.ts + +## Changed Line Ranges (PR Scope) + + + +| File | Changed Lines | +| --------------------------------------------------- | ----------------------------------------------------------- | +| src/main/runtime/orca-runtime.test.ts | 452-461, 465-466, 526-527, 617-621, 1700-1706, 1716-1717, 1777-1778 | +| src/main/runtime/orca-runtime.ts | 313, 4413-4427, 4433, 4450-4456 | +| src/main/window/attach-main-window-services.ts | 266-270 | +| src/preload/api-types.ts | 1099 | +| src/preload/index.ts | 1860, 1872 | +| src/renderer/src/hooks/useIpcEvents.ts | 34, 230, 251-256, 995-1005 | +| src/renderer/src/store/slices/store-cascades.test.ts | 1616-1665 | +| src/renderer/src/store/slices/terminals.ts | 140-149, 332-358 | +| src/shared/telemetry-events.ts | 282-293, 544 | + +## Review Standards Reference + +- Follow /review-code standards +- Focus on: correctness, security, performance, maintainability +- Priority levels: Critical > High > Medium > Low + +## File Categories + +### Electron/Main +- src/main/runtime/orca-runtime.test.ts +- src/main/runtime/orca-runtime.ts +- src/main/window/attach-main-window-services.ts +- src/preload/api-types.ts +- src/preload/index.ts + +### Frontend/UI +- src/renderer/src/hooks/useIpcEvents.ts +- src/renderer/src/store/slices/store-cascades.test.ts +- src/renderer/src/store/slices/terminals.ts + +### Utility/Common +- src/shared/telemetry-events.ts + +## Skipped Issues (Do Not Re-validate) + + + + +(none) + +## Iteration State + +Current iteration: 1 +Last completed phase: Setup +Files fixed this iteration: [] diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 26f2b8d7970..2d4d704a8d0 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -449,10 +449,21 @@ describe('OrcaRuntimeService', () => { }) expect(result.handle).toMatch(/^term_/) expect(createTerminal).not.toHaveBeenCalled() + // Why: hook-based agent status keys off `${tabId}:${paneId}`, so main must + // pre-allocate the tabId and stamp ORCA_PANE_KEY/TAB_ID/WORKTREE_ID into + // the PTY env before spawn. The same tabId is then handed to the renderer + // via `revealTerminalSession` so adoption preserves attribution. See + // docs/cli-terminal-hook-pane-key.md. + const spawnCall = spawn.mock.calls[0]?.[0] as { env?: Record } | undefined + const spawnedEnv = spawnCall?.env ?? {} + expect(spawnedEnv.ORCA_TAB_ID).toMatch(/^[0-9a-f-]+$/) + expect(spawnedEnv.ORCA_PANE_KEY).toBe(`${spawnedEnv.ORCA_TAB_ID}:1`) + expect(spawnedEnv.ORCA_WORKTREE_ID).toBe(TEST_WORKTREE_ID) expect(revealTerminalSession).toHaveBeenCalledWith(TEST_WORKTREE_ID, { ptyId: 'pty-bg', title: 'worker', - activate: false + activate: false, + tabId: spawnedEnv.ORCA_TAB_ID }) }) @@ -512,7 +523,8 @@ describe('OrcaRuntimeService', () => { expect(revealTerminalSession).toHaveBeenCalledWith(TEST_WORKTREE_ID, { ptyId: 'pty-bg', title: null, - activate: false + activate: false, + tabId: expect.stringMatching(/^[0-9a-f-]+$/) }) expect(warn).toHaveBeenCalledWith( expect.stringContaining('[terminal-create] failed to create inactive tab for pty-bg:'), @@ -602,7 +614,11 @@ describe('OrcaRuntimeService', () => { tabId: 'tab-adopted', worktreeId: TEST_WORKTREE_ID }) - expect(revealTerminalSession).toHaveBeenCalledWith(TEST_WORKTREE_ID, { + // Why: createTerminal first reveal stamps activate/tabId; the focus reveal + // is the second call and uses the simpler {ptyId, title} shape. Assert the + // most recent call (the focus one) so the createTerminal-side reveal does + // not shadow this expectation. + expect(revealTerminalSession).toHaveBeenLastCalledWith(TEST_WORKTREE_ID, { ptyId: 'pty-bg', title: 'worker' }) @@ -1681,17 +1697,24 @@ describe('OrcaRuntimeService', () => { expect.objectContaining({ cwd: '/tmp/workspaces/runtime-hook-skip', command: 'bash /tmp/repo/.git/orca/setup-runner.sh', - env: { + // Why: createTerminal stamps ORCA_PANE_KEY/TAB_ID/WORKTREE_ID into the + // PTY env on top of the caller-supplied env so hook-based agent status + // can attribute hook events to a pane. See docs/cli-terminal-hook-pane-key.md. + env: expect.objectContaining({ ORCA_ROOT_PATH: '/tmp/repo', - ORCA_WORKTREE_PATH: '/tmp/workspaces/runtime-hook-skip' - }, + ORCA_WORKTREE_PATH: '/tmp/workspaces/runtime-hook-skip', + ORCA_TAB_ID: expect.stringMatching(/^[0-9a-f-]+$/), + ORCA_PANE_KEY: expect.stringMatching(/^[0-9a-f-]+:1$/), + ORCA_WORKTREE_ID: result.worktree.id + }), worktreeId: result.worktree.id }) ) expect(revealTerminalSession).toHaveBeenLastCalledWith(result.worktree.id, { ptyId: 'pty-setup', title: 'Setup', - activate: false + activate: false, + tabId: expect.stringMatching(/^[0-9a-f-]+$/) }) }) @@ -1751,7 +1774,8 @@ describe('OrcaRuntimeService', () => { expect(revealTerminalSession).toHaveBeenCalledWith(result.worktree.id, { ptyId: 'pty-created-worktree', title: null, - activate: false + activate: false, + tabId: expect.stringMatching(/^[0-9a-f-]+$/) }) }) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 3b38d134859..2c27cdff58f 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -310,7 +310,7 @@ type RuntimeNotifier = { createTerminal(worktreeId: string, opts: { command?: string; title?: string }): void revealTerminalSession?( worktreeId: string, - opts: { ptyId: string; title?: string | null; activate?: boolean } + opts: { ptyId: string; title?: string | null; activate?: boolean; tabId?: string } ): | Promise<{ tabId: string; title?: string | null }> | { tabId: string; title?: string | null } @@ -4410,12 +4410,27 @@ export class OrcaRuntimeService { const worktree = await this.resolveWorktreeSelector(worktreeSelector) const repo = this.store?.getRepo(worktree.repoId) const preAllocatedHandle = this.createPreAllocatedTerminalHandle() + // Why: mint tabId in main before spawn so paneKey is known at PTY env + // build time. Hook-based agent status (Claude/Codex/Cursor/Gemini) keys + // off `${tabId}:${paneId}` — without these vars set on the PTY, the + // hook payload arrives with an empty paneKey and the renderer cannot + // attribute the event. paneId is hard-coded to 1 because this path + // never splits and the renderer's nextPaneId starts at 1 for a fresh + // tab. See docs/cli-terminal-hook-pane-key.md. + const tabId = randomUUID() + const paneKey = `${tabId}:1` + const env = { + ...opts.env, + ORCA_PANE_KEY: paneKey, + ORCA_TAB_ID: tabId, + ORCA_WORKTREE_ID: worktree.id + } const result = await this.ptyController.spawn({ cols: 120, rows: 40, cwd: worktree.path, command: opts.command, - env: opts.env, + env, connectionId: repo?.connectionId ?? null, worktreeId: worktree.id, preAllocatedHandle @@ -4432,10 +4447,13 @@ export class OrcaRuntimeService { try { // Why: after the PTY is spawned, renderer tab adoption is best-effort; // failing here must not strand a live process without returning a handle. + // Pass the pre-minted tabId so the renderer adopts under the same id + // already baked into the PTY env — keeps paneKey hook attribution intact. await this.notifier.revealTerminalSession(worktree.id, { ptyId: result.id, title: opts.title ?? null, - activate: false + activate: false, + tabId }) surface = 'visible' } catch (err) { diff --git a/src/main/window/attach-main-window-services.ts b/src/main/window/attach-main-window-services.ts index 8f90f5fbf37..d98d01e74f7 100644 --- a/src/main/window/attach-main-window-services.ts +++ b/src/main/window/attach-main-window-services.ts @@ -263,7 +263,11 @@ function registerRuntimeWindowLifecycle( worktreeId, ptyId: opts.ptyId, title: opts.title ?? undefined, - activate: opts.activate !== false + activate: opts.activate !== false, + // Why: pre-minted tabId from main keeps the renderer's tab id aligned + // with the paneKey baked into the PTY env at spawn time, so hook + // events route to the right slot. See docs/cli-terminal-hook-pane-key.md. + ...(opts.tabId ? { tabId: opts.tabId } : {}) }) }), splitTerminal: (tabId, paneRuntimeId, opts) => { diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 05bf69c1fd5..5ab89b68813 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -1096,6 +1096,7 @@ export type PreloadApi = { title?: string ptyId?: string activate?: boolean + tabId?: string }) => void ) => () => void onRequestTerminalCreate: ( diff --git a/src/preload/index.ts b/src/preload/index.ts index 602f1391b0c..380d646ebf4 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -1857,6 +1857,7 @@ const api = { title?: string ptyId?: string activate?: boolean + tabId?: string }) => void ): (() => void) => { const listener = ( @@ -1868,6 +1869,7 @@ const api = { title?: string ptyId?: string activate?: boolean + tabId?: string } ) => callback(data) ipcRenderer.on('ui:createTerminal', listener) diff --git a/src/renderer/src/hooks/useIpcEvents.ts b/src/renderer/src/hooks/useIpcEvents.ts index 2d2da81d78d..efa62a0acba 100644 --- a/src/renderer/src/hooks/useIpcEvents.ts +++ b/src/renderer/src/hooks/useIpcEvents.ts @@ -31,6 +31,7 @@ import { setDriverForPty } from '@/lib/pane-manager/mobile-driver-state' import { destroyPersistentWebview } from '@/components/browser-pane/webview-registry' import { attachMobileMarkdownBridge } from '@/runtime/mobile-markdown-bridge' import { detectLanguage } from '@/lib/language-detect' +import { track } from '@/lib/telemetry' export { resolveZoomTarget } from './resolve-zoom-target' @@ -226,7 +227,7 @@ export function useIpcEvents(): void { unsubs.push( window.api.ui.onCreateTerminal( - ({ requestId, worktreeId, command, title, ptyId, activate }) => { + ({ requestId, worktreeId, command, title, ptyId, activate, tabId }) => { try { const store = useAppStore.getState() const shouldActivate = activate !== false @@ -247,7 +248,12 @@ export function useIpcEvents(): void { ) ?? store.createTab(worktreeId, undefined, undefined, { initialPtyId: ptyId, - activate: shouldActivate + activate: shouldActivate, + // Why: tabId hint comes from CLI-spawned PTYs whose env + // already has paneKey=`${tabId}:1` baked in. Adopting the + // tab under the same id keeps hook-event attribution working; + // see docs/cli-terminal-hook-pane-key.md. + ...(tabId ? { id: tabId } : {}) })) : store.createTab(worktreeId) if (shouldActivate) { @@ -986,6 +992,17 @@ export function useIpcEvents(): void { } const { exists, title } = resolvePaneKey(store, data.paneKey) if (!exists) { + // Why: regression signal — pre-fix, CLI-spawned terminals routinely + // shipped hook events with an empty/unknown paneKey because main + // could not stamp ORCA_PANE_KEY into the PTY env before the tab id + // existed. Post-fix this should be near-zero in normal use; a + // non-zero rate means a paneKey is escaping into the receiver + // without a matching renderer tab. Gated on workspaceSessionReady + // above so startup-race pings don't false-positive before tabs + // hydrate. See docs/cli-terminal-hook-pane-key.md. + track('agent_hook_unattributed', { + reason: data.paneKey ? 'unknown_tab_id' : 'empty_pane_key' + }) return } store.setAgentStatus(data.paneKey, payload, title, { diff --git a/src/renderer/src/store/slices/store-cascades.test.ts b/src/renderer/src/store/slices/store-cascades.test.ts index ed4f7463128..9b733a15d35 100644 --- a/src/renderer/src/store/slices/store-cascades.test.ts +++ b/src/renderer/src/store/slices/store-cascades.test.ts @@ -1613,3 +1613,53 @@ describe('shutdownWorktreeTerminals (sleep) — agent status hygiene', () => { expect(s.acknowledgedAgentsByPaneKey['tab-1:0']).toBeUndefined() }) }) + +// Why: CLI-spawned background terminals stamp ORCA_PANE_KEY=`${tabId}:1` into +// the PTY env at spawn time. The renderer must adopt the tab under the same +// id so hook events route to the correct slot. See +// docs/cli-terminal-hook-pane-key.md. +describe('createTab tabId hint', () => { + it('uses the supplied id when no collision exists', () => { + const store = createTestStore() + const wt = 'repo1::/path/wt-hint' + seedStore(store, { + worktreesByRepo: { + repo1: [makeWorktree({ id: wt, repoId: 'repo1', path: '/path/wt-hint' })] + }, + groupsByWorktree: {}, + activeGroupIdByWorktree: {}, + unifiedTabsByWorktree: {} + }) + + const hintedId = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' + const tab = store.getState().createTab(wt, undefined, undefined, { id: hintedId }) + + expect(tab.id).toBe(hintedId) + }) + + it('falls back to a fresh id on collision and warns', () => { + const store = createTestStore() + const wt = 'repo1::/path/wt-collision' + const existingId = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' + seedStore(store, { + worktreesByRepo: { + repo1: [makeWorktree({ id: wt, repoId: 'repo1', path: '/path/wt-collision' })] + }, + tabsByWorktree: { + [wt]: [makeTab({ id: existingId, worktreeId: wt })] + }, + groupsByWorktree: {}, + activeGroupIdByWorktree: {}, + unifiedTabsByWorktree: {} + }) + + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const tab = store.getState().createTab(wt, undefined, undefined, { id: existingId }) + expect(tab.id).not.toBe(existingId) + expect(warn).toHaveBeenCalledWith(expect.stringContaining(existingId)) + } finally { + warn.mockRestore() + } + }) +}) diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 639bb8cd310..744fd4e67f1 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -137,7 +137,16 @@ export type TerminalSlice = { worktreeId: string, targetGroupId?: string, shellOverride?: string, - options?: { pendingActivationSpawn?: boolean; initialPtyId?: string; activate?: boolean } + options?: { + pendingActivationSpawn?: boolean + initialPtyId?: string + activate?: boolean + /** Pre-allocated tab id (e.g. minted by main for CLI-spawned terminals + * whose PTY env already carries `paneKey=`${tabId}:1``). Falls back to + * minting a fresh id when omitted or when the supplied id collides + * with an existing tab on this worktree. */ + id?: string + } ) => TerminalTab closeTab: (tabId: string) => void reorderTabs: (worktreeId: string, tabIds: string[]) => void @@ -321,7 +330,6 @@ export const createTerminalSlice: StateCreator }), createTab: (worktreeId, targetGroupId, shellOverride, options) => { - const id = globalThis.crypto.randomUUID() let tab!: TerminalTab set((s) => { const orphanTerminalIds = getOrphanTerminalIds(s, worktreeId) @@ -329,6 +337,25 @@ export const createTerminalSlice: StateCreator const existing = (s.tabsByWorktree[worktreeId] ?? []).filter( (entry) => !orphanTerminalIds.has(entry.id) ) + // Why: caller-supplied id (e.g. main pre-allocates the tabId for CLI + // background terminals so the paneKey env baked into the PTY matches + // the renderer's tab id). Fall back to minting if the id collides — a + // collision would alias two distinct PTYs to one tab id and silently + // corrupt agent-status routing. Hook attribution degrades for that + // single terminal because paneKey is already baked into PTY env, but + // the rest of the tab works normally. See docs/cli-terminal-hook-pane-key.md. + const idCollides = + options?.id !== undefined && + Object.values(s.tabsByWorktree).some((tabs) => + tabs.some((entry) => entry.id === options.id) + ) + if (options?.id !== undefined && idCollides) { + console.warn( + `[createTab] tabId hint ${options.id} already exists; minting a fresh id (hook attribution will degrade for this terminal)` + ) + } + const id = + options?.id !== undefined && !idCollides ? options.id : globalThis.crypto.randomUUID() const shouldActivate = options?.activate !== false const nextOrdinal = getNextTerminalOrdinal(existing) const defaultTitle = `Terminal ${nextOrdinal}` diff --git a/src/shared/telemetry-events.ts b/src/shared/telemetry-events.ts index 5999b0de0f9..e977af82067 100644 --- a/src/shared/telemetry-events.ts +++ b/src/shared/telemetry-events.ts @@ -279,6 +279,18 @@ const agentHookInstallFailedSchema = z }) .strict() +// Why: regression signal for paneKey attribution. A hook event whose paneKey +// does not correspond to any tab in `tabsByWorktree` indicates the renderer +// could not route the event to a pane. Pre-fix this fired routinely for +// CLI-spawned terminals (empty paneKey); post-fix it should be near-zero in +// normal use. The lone `reason` field discriminates between the known +// failure modes: an empty/missing paneKey on the wire (pre-fix CLI shape) +// vs. a paneKey whose tab id no longer exists in `tabsByWorktree` (renderer +// adopted a different id). See docs/cli-terminal-hook-pane-key.md. +const agentHookUnattributedSchema = z + .object({ reason: z.enum(['empty_pane_key', 'unknown_tab_id']) }) + .strict() + // ── Onboarding ────────────────────────────────────────────────────────── // // Closed enums only — no raw paths, repo names, clone URLs, or error @@ -529,6 +541,7 @@ export const eventSchemas = { agent_started: agentStartedSchema, agent_error: agentErrorSchema, agent_hook_install_failed: agentHookInstallFailedSchema, + agent_hook_unattributed: agentHookUnattributedSchema, settings_changed: settingsChangedSchema,