diff --git a/src/main/skills/discovery.test.ts b/src/main/skills/discovery.test.ts index dcee058f8e2..68ff5e9d017 100644 --- a/src/main/skills/discovery.test.ts +++ b/src/main/skills/discovery.test.ts @@ -57,6 +57,32 @@ describe('skill discovery', () => { expect(rootPaths).toContain('/workspace/current/.claude/skills') }) + it('scans each provider home skill root that npx skills --global writes to', () => { + const roots = buildSkillDiscoverySources({ + homeDir: '/home/test', + cwd: '/workspace/current' + }) + + const rootPaths = roots.map((root) => root.path.replace(/\\/g, '/')) + expect(rootPaths).toEqual( + expect.arrayContaining([ + '/home/test/.grok/skills', + '/home/test/.config/opencode/skills', + '/home/test/.pi/agent/skills', + '/home/test/.gemini/skills', + '/home/test/.gemini/antigravity/skills', + '/home/test/.cursor/skills' + ]) + ) + // Why: these live outside ~/.agents/skills, so they must carry the shared + // agent-skills provider to feed per-agent orchestration coverage. + for (const root of roots) { + if (root.path.replace(/\\/g, '/') === '/home/test/.grok/skills') { + expect(root.providers).toEqual(['agent-skills']) + } + } + }) + it('discovers skill packages through symlinked skill directories', async () => { const root = await mkdtemp(join(tmpdir(), 'orca-skills-')) const home = join(root, 'home') @@ -77,6 +103,27 @@ describe('skill discovery', () => { expect(skill?.directoryPath).toBe(linkedSkill) }) + it('discovers a symlinked skill inside a provider home root (#8256/#8503)', async () => { + const root = await mkdtemp(join(tmpdir(), 'orca-skills-')) + const home = join(root, 'home') + const realSkill = join(root, 'central-skills', 'orchestration') + const linkedSkill = join(home, '.pi', 'agent', 'skills', 'orchestration') + await mkdir(realSkill, { recursive: true }) + await mkdir(join(home, '.pi', 'agent', 'skills'), { recursive: true }) + await writeFile(join(realSkill, 'SKILL.md'), '# orchestration\n\nCoordinate agents.') + await symlink(realSkill, linkedSkill, process.platform === 'win32' ? 'junction' : 'dir') + + const result = await discoverSkills({ + homeDir: home, + cwd: join(root, 'missing-cwd') + }) + + const skill = result.skills.find((entry) => entry.name === 'orchestration') + expect(skill?.sourceKind).toBe('home') + expect(skill?.directoryPath).toBe(linkedSkill) + expect(skill?.providers).toEqual(['agent-skills']) + }) + it('discovers worktree .agents skill symlinks from the requested cwd', async () => { const root = await mkdtemp(join(tmpdir(), 'orca-skills-')) const home = join(root, 'home') diff --git a/src/main/skills/skill-discovery-sources.ts b/src/main/skills/skill-discovery-sources.ts index c1bb1072b2e..bec34117c4c 100644 --- a/src/main/skills/skill-discovery-sources.ts +++ b/src/main/skills/skill-discovery-sources.ts @@ -41,7 +41,23 @@ export function buildSkillDiscoverySources( join(home, '.codex', 'plugins', 'cache'), 'plugin', ['codex', 'agent-skills'] - ) + ), + // Why: `npx skills add --global` writes into each agent's own home skills + // directory, so coverage misses them unless we scan every provider root. + source('home-grok', 'Grok home', join(home, '.grok', 'skills'), 'home', ['agent-skills']), + source('home-opencode', 'OpenCode home', join(home, '.config', 'opencode', 'skills'), 'home', [ + 'agent-skills' + ]), + source('home-pi', 'Pi home', join(home, '.pi', 'agent', 'skills'), 'home', ['agent-skills']), + source('home-gemini', 'Gemini home', join(home, '.gemini', 'skills'), 'home', ['agent-skills']), + source( + 'home-antigravity', + 'Antigravity home', + join(home, '.gemini', 'antigravity', 'skills'), + 'home', + ['agent-skills'] + ), + source('home-cursor', 'Cursor home', join(home, '.cursor', 'skills'), 'home', ['agent-skills']) ] const projectPaths = new Set() diff --git a/src/renderer/src/components/repo/NestedRepoChecklist.tsx b/src/renderer/src/components/repo/NestedRepoChecklist.tsx index dd20b9781aa..95ea1a41897 100644 --- a/src/renderer/src/components/repo/NestedRepoChecklist.tsx +++ b/src/renderer/src/components/repo/NestedRepoChecklist.tsx @@ -2,7 +2,7 @@ import { useCallback, useMemo, type Dispatch, type SetStateAction } from 'react' import { GitBranch } from 'lucide-react' import type { NestedRepoScanResult } from '../../../../shared/types' import { cn } from '@/lib/utils' -import { getRepoDisplayLabelsByPath } from '@/lib/repo-display-labels' +import { getRepoDisplayLabelKey, getRepoDisplayLabelsByPath } from '@/lib/repo-display-labels' import { translate } from '@/i18n/i18n' function NestedRepoSelectAllRow({ @@ -118,7 +118,7 @@ export function NestedRepoChecklist({ selectedPaths.has(repo.path) ? 'text-foreground' : 'text-muted-foreground' )} > - {displayLabelsByPath.get(repo.path) ?? repo.displayName} + {displayLabelsByPath.get(getRepoDisplayLabelKey(repo)) ?? repo.displayName} diff --git a/src/renderer/src/components/sidebar/worktree-list-groups.ts b/src/renderer/src/components/sidebar/worktree-list-groups.ts index e7fb1749ed4..df31f09f8fb 100644 --- a/src/renderer/src/components/sidebar/worktree-list-groups.ts +++ b/src/renderer/src/components/sidebar/worktree-list-groups.ts @@ -32,7 +32,7 @@ import { import { cloneDefaultWorkspaceStatuses } from '../../../../shared/workspace-statuses' import type { AppState } from '../../store/types' import { getGitHubPRCacheKey, getLegacyGitHubPRCacheKey } from '../../store/slices/github-cache-key' -import { getRepoDisplayLabelsByPath } from '@/lib/repo-display-labels' +import { getRepoDisplayLabelKey, getRepoDisplayLabelsByPath } from '@/lib/repo-display-labels' import { translate } from '@/i18n/i18n' import { getExecutionHostLabel, getRepoExecutionHostId } from '../../../../shared/execution-host' import { parseWslUncPath } from '../../../../shared/wsl-paths' @@ -709,7 +709,9 @@ function withRepoSectionDisplayLabels(entries: readonly OrderedGroupEntry[]): Or const labelsByPath = getRepoDisplayLabelsByPath(repos) return entries.map(([key, group]) => [ key, - group.repo ? { ...group, label: labelsByPath.get(group.repo.path) ?? group.label } : group + group.repo + ? { ...group, label: labelsByPath.get(getRepoDisplayLabelKey(group.repo)) ?? group.label } + : group ]) } diff --git a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.test.ts b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.test.ts index d48f509c9ac..6a4c8b197cf 100644 --- a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.test.ts +++ b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.test.ts @@ -40,6 +40,7 @@ function createRejectableDeferred(): { } const HOOK_DONE_QUIET_MS = 1_500 +const CODEX_ATTENTION_QUIET_MS = 1_500 describe('agent completion coordinator', () => { beforeEach(() => { @@ -1638,6 +1639,283 @@ describe('agent completion coordinator', () => { expect(dispatchCompletion).toHaveBeenCalledTimes(1) }) + it('cancels the debounced Codex attention notification when work resumes in the quiet window', () => { + // Why: Codex fires PermissionRequest at the human-input boundary *before* the + // approval decision. Under "Approve for me" the review agent approves and + // Codex resumes within the quiet window, so the OS notification must be + // debounced and canceled — no false "approval required" banner (issue #8387). + const dispatchAttention = vi.fn() + const dispatchHookLifecycle = vi.fn() + const coordinator = createAgentCompletionCoordinator({ + paneKey: 'tab-1:leaf-1', + getPtyId: () => 'pty-1', + getSettings: () => null, + inspectProcess: vi.fn(), + dispatchCompletion: vi.fn(), + dispatchAttention, + dispatchHookLifecycle, + isLive: () => true + }) + + const turn = { prompt: 'fix the bug', agentType: 'codex' as const } + coordinator.observeHookStatus({ state: 'working', ...turn }) + coordinator.observeHookStatus({ + state: 'waiting', + ...turn, + toolName: 'exec_command', + toolInput: 'git status' + }) + + // Visual status still updates immediately even though the notification waits. + expect(dispatchHookLifecycle).toHaveBeenCalledWith( + expect.objectContaining({ state: 'waiting', agentType: 'codex' }) + ) + expect(dispatchAttention).not.toHaveBeenCalled() + + coordinator.observeHookStatus({ + state: 'working', + ...turn, + toolName: 'exec_command', + toolInput: 'git status' + }) + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) + + expect(dispatchAttention).not.toHaveBeenCalled() + }) + + it('dispatches the debounced Codex attention notification after the quiet window elapses', () => { + const dispatchAttention = vi.fn() + const coordinator = createAgentCompletionCoordinator({ + paneKey: 'tab-1:leaf-1', + getPtyId: () => 'pty-1', + getSettings: () => null, + inspectProcess: vi.fn(), + dispatchCompletion: vi.fn(), + dispatchAttention, + isLive: () => true + }) + + const turn = { prompt: 'fix the bug', agentType: 'codex' as const } + coordinator.observeHookStatus({ state: 'working', ...turn }) + coordinator.observeHookStatus({ + state: 'waiting', + ...turn, + toolName: 'exec_command', + toolInput: 'apply patch' + }) + expect(dispatchAttention).not.toHaveBeenCalled() + + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) + + expect(dispatchAttention).toHaveBeenCalledTimes(1) + expect(dispatchAttention).toHaveBeenCalledWith( + 'codex', + expect.objectContaining({ + source: 'hook', + agentStatus: expect.objectContaining({ + state: 'waiting', + agentType: 'codex', + toolInput: 'apply patch' + }) + }) + ) + }) + + it('dispatches a non-Codex attention notification immediately without debounce', () => { + const dispatchAttention = vi.fn() + const coordinator = createAgentCompletionCoordinator({ + paneKey: 'tab-1:leaf-1', + getPtyId: () => 'pty-1', + getSettings: () => null, + inspectProcess: vi.fn(), + dispatchCompletion: vi.fn(), + dispatchAttention, + isLive: () => true + }) + + const turn = { prompt: 'fix the bug', agentType: 'cursor' as const } + coordinator.observeHookStatus({ state: 'working', ...turn }) + coordinator.observeHookStatus({ + state: 'waiting', + ...turn, + toolName: 'Shell', + toolInput: 'pnpm test' + }) + + expect(dispatchAttention).toHaveBeenCalledTimes(1) + // Non-Codex attention must not arm the debounce timer at all. + expect(vi.getTimerCount()).toBe(0) + }) + + it('debounces a blocked Codex pause like waiting and fires after the quiet window', () => { + const dispatchAttention = vi.fn() + const coordinator = createAgentCompletionCoordinator({ + paneKey: 'tab-1:leaf-1', + getPtyId: () => 'pty-1', + getSettings: () => null, + inspectProcess: vi.fn(), + dispatchCompletion: vi.fn(), + dispatchAttention, + isLive: () => true + }) + + const turn = { prompt: 'fix the bug', agentType: 'codex' as const } + coordinator.observeHookStatus({ state: 'working', ...turn }) + coordinator.observeHookStatus({ state: 'blocked', ...turn, toolName: 'exec_command' }) + expect(dispatchAttention).not.toHaveBeenCalled() + + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) + expect(dispatchAttention).toHaveBeenCalledTimes(1) + }) + + it('cancels the debounced Codex attention when a completion lands in the window (no double notify)', () => { + const dispatchAttention = vi.fn() + const dispatchCompletion = vi.fn() + const coordinator = createAgentCompletionCoordinator({ + paneKey: 'tab-1:leaf-1', + getPtyId: () => 'pty-1', + getSettings: () => null, + inspectProcess: vi.fn(), + dispatchCompletion, + dispatchAttention, + isLive: () => true + }) + + const turn = { prompt: 'fix the bug', agentType: 'codex' as const } + coordinator.observeHookStatus({ state: 'working', ...turn }) + coordinator.observeHookStatus({ state: 'waiting', ...turn, toolName: 'exec_command' }) + // A 'done' completing the turn inside the window must cancel the pending + // attention so the pause never co-fires with the completion notification. + coordinator.observeHookStatus({ state: 'done', ...turn }) + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) + + expect(dispatchAttention).not.toHaveBeenCalled() + expect(dispatchCompletion).toHaveBeenCalledTimes(1) + expect(vi.getTimerCount()).toBe(0) + }) + + it('clears the pending Codex attention timer on dispose (no leak, no late fire)', () => { + const dispatchAttention = vi.fn() + const coordinator = createAgentCompletionCoordinator({ + paneKey: 'tab-1:leaf-1', + getPtyId: () => 'pty-1', + getSettings: () => null, + inspectProcess: vi.fn(), + dispatchCompletion: vi.fn(), + dispatchAttention, + isLive: () => true + }) + + const turn = { prompt: 'fix the bug', agentType: 'codex' as const } + coordinator.observeHookStatus({ state: 'working', ...turn }) + coordinator.observeHookStatus({ state: 'waiting', ...turn, toolName: 'exec_command' }) + expect(vi.getTimerCount()).toBe(1) + + coordinator.dispose() + expect(vi.getTimerCount()).toBe(0) + + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) + expect(dispatchAttention).not.toHaveBeenCalled() + }) + + it('re-arms and fires a second distinct Codex pause after work resumed', () => { + const dispatchAttention = vi.fn() + const coordinator = createAgentCompletionCoordinator({ + paneKey: 'tab-1:leaf-1', + getPtyId: () => 'pty-1', + getSettings: () => null, + inspectProcess: vi.fn(), + dispatchCompletion: vi.fn(), + dispatchAttention, + isLive: () => true + }) + + const turn = { prompt: 'fix the bug', agentType: 'codex' as const } + coordinator.observeHookStatus({ state: 'working', ...turn }) + coordinator.observeHookStatus({ state: 'waiting', ...turn, toolName: 'exec_command', toolInput: 'ls' }) + // First pause auto-resolves before the window elapses. + coordinator.observeHookStatus({ state: 'working', ...turn, toolName: 'exec_command', toolInput: 'ls' }) + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) + expect(dispatchAttention).not.toHaveBeenCalled() + + // A later, genuinely-distinct pause must re-arm the debounce and fire. + coordinator.observeHookStatus({ state: 'waiting', ...turn, toolName: 'apply_patch', toolInput: 'diff' }) + expect(dispatchAttention).not.toHaveBeenCalled() + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) + expect(dispatchAttention).toHaveBeenCalledTimes(1) + }) + + it('cancels the debounced Codex attention when a working-spinner title resumes', () => { + // Why: a Codex resume can surface as a working title before the resume + // 'working' hook lands; that title must also cancel the pending attention + // so the self-resolving pause never fires a false banner (issue #8387). + const dispatchAttention = vi.fn() + const coordinator = createAgentCompletionCoordinator({ + paneKey: 'tab-1:leaf-1', + getPtyId: () => 'pty-1', + getSettings: () => null, + inspectProcess: vi.fn(), + dispatchCompletion: vi.fn(), + dispatchAttention, + isLive: () => true + }) + + const turn = { prompt: 'fix the bug', agentType: 'codex' as const } + coordinator.observeHookStatus({ state: 'working', ...turn }) + coordinator.observeHookStatus({ state: 'waiting', ...turn, toolName: 'exec_command' }) + expect(vi.getTimerCount()).toBe(1) + + coordinator.observeTitleWorking() + expect(vi.getTimerCount()).toBe(0) + + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) + expect(dispatchAttention).not.toHaveBeenCalled() + }) + + it('does not let a null-foreground inspection blip drop the debounced Codex attention', async () => { + // Why: guard for #8387 fail-open. In the pty-connection coordinator (real + // process polling), a transient null/shell foreground blip — or a remote + // inspection that cannot resolve the foreground — must not convert a genuine + // Codex pause into a process-exit completion while the attention debounce is + // still pending. Mirrors the pendingHookDoneTimer evidence-teardown guard. + let foreground: string | null = 'codex' + const dispatchAttention = vi.fn() + const dispatchCompletion = vi.fn() + const coordinator = createAgentCompletionCoordinator({ + paneKey: 'tab-1:leaf-1', + getPtyId: () => 'pty-1', + getSettings: () => null, + inspectProcess: vi.fn(async () => processResult(foreground)), + dispatchCompletion, + dispatchAttention, + isLive: () => true + }) + + coordinator.startProcessTracking() + // First cadence poll recognizes Codex as the foreground agent (active tier). + await vi.advanceTimersByTimeAsync(2_000) + await flushAsyncTicks() + + // Codex pauses for a permission decision: the OS attention is debounced. + const turn = { prompt: 'apply patch', agentType: 'codex' as const } + coordinator.observeHookStatus({ + state: 'waiting', + ...turn, + toolName: 'exec_command', + toolInput: 'rm -rf build' + }) + expect(dispatchAttention).not.toHaveBeenCalled() + + // Foreground reads null for the whole window; without the guard this would + // land a false process-exit completion racing/duplicating the pause banner. + foreground = null + await vi.advanceTimersByTimeAsync(CODEX_ATTENTION_QUIET_MS + 100) + await flushAsyncTicks() + + expect(dispatchCompletion).not.toHaveBeenCalled() + expect(dispatchAttention).toHaveBeenCalledTimes(1) + }) + it('keeps a generic title completion pending long enough for the first remote inspection', async () => { const inspection = createDeferred() const dispatchCompletion = vi.fn() diff --git a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts index fe3e8208ada..f71141e1045 100644 --- a/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts +++ b/src/renderer/src/components/terminal-pane/agent-completion-coordinator.ts @@ -61,6 +61,12 @@ const PENDING_TITLE_TTL_MS = Math.max(2_000, INSPECTION_TIMEOUT_MS + 500) const PENDING_TITLE_MAX_TTL_MS = Math.max(30_000, PENDING_TITLE_TTL_MS) const COMPLETION_REPLAY_GUARD_MS = 1_000 const HOOK_DONE_QUIET_MS = 1_500 +// Why: Codex fires its PermissionRequest hook at the human-input boundary before +// the approval decision, so under "Approve for me" the review agent approves and +// Codex resumes almost immediately. Debounce the OS attention notification behind +// this quiet window so a self-resolving pause never raises a false "approval +// required" banner (issue #8387). The visual status still updates immediately. +const CODEX_ATTENTION_QUIET_MS = 1_500 const POLL_TIER_INTERVAL_MS: Record = { active: ACTIVE_POLL_INTERVAL_MS, @@ -106,6 +112,7 @@ export function createAgentCompletionCoordinator( let pendingHookDoneTimer: ReturnType | null = null let pendingHookDoneTitle: string | null = null let pendingHookDonePayload: AgentCompletionStatusSnapshot | null = null + let pendingCodexAttentionTimer: ReturnType | null = null let pendingProcessExitAgent: RecognizedAgentProcess | null = null let pendingTitleSequence = 0 let pendingTitle: { @@ -155,6 +162,13 @@ export function createAgentCompletionCoordinator( pendingHookDonePayload = null } + function clearPendingCodexAttention(): void { + if (pendingCodexAttentionTimer !== null) { + clearTimeout(pendingCodexAttentionTimer) + pendingCodexAttentionTimer = null + } + } + function establishAgentEvidence(): void { agentIdentityEstablished = true hasAgentRunEvidence = true @@ -304,6 +318,9 @@ export function createAgentCompletionCoordinator( lastCompletedTurn = currentTurn lastCompletionSource = source workingStatusObserved = false + // Why: any committed completion (hook/title/process-exit) ends the turn, so a + // debounced Codex attention from an earlier pause must never fire after it. + clearPendingCodexAttention() if (optionsOverride.completionIdentity) { lastCompletionIdentityByPaneKey.set(options.paneKey, optionsOverride.completionIdentity) } @@ -324,6 +341,13 @@ export function createAgentCompletionCoordinator( } } + function dispatchAttentionNotification(payload: AgentCompletionStatusSnapshot): void { + options.dispatchAttention?.(payload.agentType ?? options.paneKey, { + source: 'hook', + agentStatus: payload + }) + } + function dispatchAttention(payload: AgentCompletionStatusSnapshot): void { if (!options.dispatchAttention || !options.isLive() || !hasAgentRunEvidence) { return @@ -333,11 +357,25 @@ export function createAgentCompletionCoordinator( return } lastAttentionToken = token + // Why: the visual "needs input" status must update immediately for every + // agent; only the OS attention notification is debounced (Codex, below). options.dispatchHookLifecycle?.(payload) - options.dispatchAttention(payload.agentType ?? options.paneKey, { - source: 'hook', - agentStatus: payload - }) + if (payload.agentType === 'codex') { + // Why: a Codex PermissionRequest that "Approve for me" auto-resolves fires a + // working/completion hook inside this window, which cancels the pending + // notification (see CODEX_ATTENTION_QUIET_MS). Scoped to Codex so every other + // agent's genuine pause still notifies immediately. + clearPendingCodexAttention() + pendingCodexAttentionTimer = setTimeout(() => { + pendingCodexAttentionTimer = null + if (!options.isLive() || !hasAgentRunEvidence) { + return + } + dispatchAttentionNotification(payload) + }, CODEX_ATTENTION_QUIET_MS) + return + } + dispatchAttentionNotification(payload) } function scheduleHookDoneCompletion(title: string, payload: AgentCompletionStatusSnapshot): void { @@ -472,9 +510,12 @@ export function createAgentCompletionCoordinator( handleRecognizedProcess(recognized) return true } - if (pendingHookDoneTimer !== null) { - // Why: a pending quiet-window 'done' is the authoritative completion; - // tearing down agent evidence here would make the timer drop it. + if (pendingHookDoneTimer !== null || pendingCodexAttentionTimer !== null) { + // Why: a pending quiet-window 'done' or debounced Codex attention is the + // authoritative signal for this turn; tearing down agent evidence here (a + // transient null/shell foreground blip before the agent process is + // recognized) would make the timer's hasAgentRunEvidence guard silently + // drop it. Keep the fail-open contract for #8387 as for the done timer. scheduleNextPoll() return false } @@ -696,6 +737,12 @@ export function createAgentCompletionCoordinator( ) { return false } + // Why: a genuine Codex resume can surface as a working-spinner title before + // (or instead of) the resume 'working' hook, so cancel the debounced + // attention here or the self-resolving pause still fires a false banner + // (#8387). Placed after the replay guard so only an authoritative resume — + // not a stale post-completion title replay — drops a still-pending banner. + clearPendingCodexAttention() workingStatusObserved = true requiresFreshWorking = false lastCompletionIdentityByPaneKey.delete(options.paneKey) @@ -789,6 +836,7 @@ export function createAgentCompletionCoordinator( // so the quiet-window timer never fires a false completion notification. if (isAttentionHookState(payload.state)) { clearPendingHookDone() + clearPendingCodexAttention() } return } @@ -797,6 +845,9 @@ export function createAgentCompletionCoordinator( } if (payload.state === 'working') { clearPendingHookDone() + // Why: resumed work (e.g. Codex after "Approve for me") cancels the debounced + // attention notification so the self-resolving pause never notifies. + clearPendingCodexAttention() workingStatusObserved = true requiresFreshWorking = false lastCompletionIdentity = null @@ -814,6 +865,9 @@ export function createAgentCompletionCoordinator( return } if (isCompletionHookState(payload.state)) { + // Why: the turn is ending, so a debounced attention from an earlier pause in + // this turn must not fire after the completion notification. + clearPendingCodexAttention() if (isRecognizedAgentType(payload.agentType)) { establishAgentEvidence() } @@ -885,6 +939,7 @@ export function createAgentCompletionCoordinator( function resetCompletionState(options: { requireFreshWorking?: boolean } = {}): void { clearPendingHookDone() + clearPendingCodexAttention() dropPendingTitle() agentIdentityEstablished = false hasAgentRunEvidence = false @@ -905,6 +960,7 @@ export function createAgentCompletionCoordinator( disposed = true clearPollTimer() clearPendingHookDone() + clearPendingCodexAttention() dropPendingTitle() // Why: the dedup identity is module-scoped so it survives a live-stream remount // (dispose-then-recreate with the same paneKey while isLive() stays true). Only diff --git a/src/renderer/src/hooks/agent-hook-completion-notifications.test.ts b/src/renderer/src/hooks/agent-hook-completion-notifications.test.ts index 75487c039b9..1801c78bace 100644 --- a/src/renderer/src/hooks/agent-hook-completion-notifications.test.ts +++ b/src/renderer/src/hooks/agent-hook-completion-notifications.test.ts @@ -56,6 +56,9 @@ type MockStoreState = { let mockStoreState: MockStoreState const HOOK_DONE_QUIET_MS = 1_500 +// Why: Codex attention notifications are debounced (issue #8387), so a genuine +// permission pause only notifies once this quiet window elapses without resuming. +const CODEX_ATTENTION_QUIET_MS = 1_500 vi.mock('@/store', () => ({ useAppStore: { @@ -106,6 +109,31 @@ function seedCodexPaneLaunchConfig( describe('agent hook completion notifications', () => { const paneKey = 'tab-1:11111111-1111-4111-8111-111111111111' + // Why: the Codex permission-pause tests share a working→pause→quiet-window + // sequence; centralizing it keeps the debounce advance (issue #8387) in one spot. + async function observeCodexPermissionPause(state: 'waiting' | 'blocked'): Promise { + const { observeAgentHookCompletionForNotification } = await import( + './agent-hook-completion-notifications' + ) + observeAgentHookCompletionForNotification({ + paneKey, + worktreeId: 'wt-1', + payload: hookStatus('working') + }) + observeAgentHookCompletionForNotification({ + paneKey, + worktreeId: 'wt-1', + payload: { + state, + prompt: 'implement notifications', + agentType: 'codex', + toolName: 'exec_command', + toolInput: 'git status' + } + }) + vi.advanceTimersByTime(CODEX_ATTENTION_QUIET_MS) + } + beforeEach(() => { vi.resetModules() vi.useFakeTimers() @@ -560,50 +588,14 @@ describe('agent hook completion notifications', () => { it('fails open for Codex auto-approved permission requests without launch proof', async () => { seedCodexPaneLaunchConfig(paneKey, YOLO_TUI_AGENT_ARGS.codex ?? '') - const { observeAgentHookCompletionForNotification } = - await import('./agent-hook-completion-notifications') - - observeAgentHookCompletionForNotification({ - paneKey, - worktreeId: 'wt-1', - payload: hookStatus('working') - }) - observeAgentHookCompletionForNotification({ - paneKey, - worktreeId: 'wt-1', - payload: { - state: 'waiting', - prompt: 'implement notifications', - agentType: 'codex', - toolName: 'exec_command', - toolInput: 'git status' - } - }) + await observeCodexPermissionPause('waiting') expect(dispatchTerminalNotification).toHaveBeenCalledTimes(1) }) it('still notifies for manual Codex permission requests', async () => { seedCodexPaneLaunchConfig(paneKey, '') - const { observeAgentHookCompletionForNotification } = - await import('./agent-hook-completion-notifications') - - observeAgentHookCompletionForNotification({ - paneKey, - worktreeId: 'wt-1', - payload: hookStatus('working') - }) - observeAgentHookCompletionForNotification({ - paneKey, - worktreeId: 'wt-1', - payload: { - state: 'waiting', - prompt: 'implement notifications', - agentType: 'codex', - toolName: 'exec_command', - toolInput: 'git status' - } - }) + await observeCodexPermissionPause('waiting') expect(dispatchTerminalNotification).toHaveBeenCalledTimes(1) expect(dispatchTerminalNotification).toHaveBeenCalledWith( @@ -618,25 +610,7 @@ describe('agent hook completion notifications', () => { it('fails open for Codex auto-approved blocked permission requests without launch proof', async () => { seedCodexPaneLaunchConfig(paneKey, YOLO_TUI_AGENT_ARGS.codex ?? '') - const { observeAgentHookCompletionForNotification } = - await import('./agent-hook-completion-notifications') - - observeAgentHookCompletionForNotification({ - paneKey, - worktreeId: 'wt-1', - payload: hookStatus('working') - }) - observeAgentHookCompletionForNotification({ - paneKey, - worktreeId: 'wt-1', - payload: { - state: 'blocked', - prompt: 'implement notifications', - agentType: 'codex', - toolName: 'exec_command', - toolInput: 'git status' - } - }) + await observeCodexPermissionPause('blocked') expect(dispatchTerminalNotification).toHaveBeenCalledTimes(1) }) diff --git a/src/renderer/src/lib/orchestration-skill-coverage.test.ts b/src/renderer/src/lib/orchestration-skill-coverage.test.ts index aea4a26a975..bcb68ff74b5 100644 --- a/src/renderer/src/lib/orchestration-skill-coverage.test.ts +++ b/src/renderer/src/lib/orchestration-skill-coverage.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest' import type { DiscoveredSkill } from '../../../shared/skills' +import type { TuiAgent } from '../../../shared/types' import { agentHasOrchestrationSkill, getOrchestrationSkillAgentStatuses @@ -97,7 +98,102 @@ describe('orchestration skill agent coverage', () => { ).toBe(true) }) - it('matches Windows skill paths', () => { + it('marks each provider-home agent from its own global skills location', () => { + const cases: { agent: TuiAgent; rootPath: string; directoryPath: string }[] = [ + { + agent: 'grok', + rootPath: '/Users/test/.grok/skills', + directoryPath: '/Users/test/.grok/skills/orchestration' + }, + { + agent: 'opencode', + rootPath: '/Users/test/.config/opencode/skills', + directoryPath: '/Users/test/.config/opencode/skills/orchestration' + }, + { + agent: 'pi', + rootPath: '/Users/test/.pi/agent/skills', + directoryPath: '/Users/test/.pi/agent/skills/orchestration' + }, + { + agent: 'gemini', + rootPath: '/Users/test/.gemini/skills', + directoryPath: '/Users/test/.gemini/skills/orchestration' + }, + { + agent: 'antigravity', + rootPath: '/Users/test/.gemini/antigravity/skills', + directoryPath: '/Users/test/.gemini/antigravity/skills/orchestration' + }, + { + agent: 'cursor', + rootPath: '/Users/test/.cursor/skills', + directoryPath: '/Users/test/.cursor/skills/orchestration' + } + ] + for (const { agent, rootPath, directoryPath } of cases) { + const skills = [ + skill({ providers: ['agent-skills'], sourceKind: 'home', rootPath, directoryPath }) + ] + expect(agentHasOrchestrationSkill(agent, skills)).toBe(true) + // Why: a provider-home install must not leak coverage to unrelated agents. + expect(agentHasOrchestrationSkill('claude', skills)).toBe(false) + } + }) + + it('marks a multi-segment provider-home agent from a Windows-style path', () => { + expect( + agentHasOrchestrationSkill('opencode', [ + skill({ + providers: ['agent-skills'], + sourceKind: 'home', + rootPath: 'C:\\Users\\test\\.config\\opencode\\skills', + directoryPath: 'C:\\Users\\test\\.config\\opencode\\skills\\orchestration' + }) + ]) + ).toBe(true) + }) + + it('keeps Gemini and Antigravity distinct despite sharing the ~/.gemini root', () => { + const geminiInstall = [ + skill({ + providers: ['agent-skills'], + sourceKind: 'home', + rootPath: '/Users/test/.gemini/skills', + directoryPath: '/Users/test/.gemini/skills/orchestration' + }) + ] + const antigravityInstall = [ + skill({ + providers: ['agent-skills'], + sourceKind: 'home', + rootPath: '/Users/test/.gemini/antigravity/skills', + directoryPath: '/Users/test/.gemini/antigravity/skills/orchestration' + }) + ] + + // Why: `.gemini/skills` and `.gemini/antigravity/skills` are siblings, so a + // segment matcher must not let one provider's install mark the other. + expect(agentHasOrchestrationSkill('gemini', geminiInstall)).toBe(true) + expect(agentHasOrchestrationSkill('antigravity', geminiInstall)).toBe(false) + expect(agentHasOrchestrationSkill('antigravity', antigravityInstall)).toBe(true) + expect(agentHasOrchestrationSkill('gemini', antigravityInstall)).toBe(false) + }) + + it('marks Claude Agent Teams from ~/.claude/skills like Claude Code', () => { + const skills = [ + skill({ + providers: ['claude'], + sourceKind: 'home', + rootPath: '/Users/test/.claude/skills', + directoryPath: '/Users/test/.claude/skills/orchestration' + }) + ] + + expect(agentHasOrchestrationSkill('claude-agent-teams', skills)).toBe(true) + }) + + it('marks Windows skill paths', () => { expect( agentHasOrchestrationSkill('codex', [ skill({ diff --git a/src/renderer/src/lib/orchestration-skill-coverage.ts b/src/renderer/src/lib/orchestration-skill-coverage.ts index 4cabdb095a6..28300112837 100644 --- a/src/renderer/src/lib/orchestration-skill-coverage.ts +++ b/src/renderer/src/lib/orchestration-skill-coverage.ts @@ -9,6 +9,12 @@ export type OrchestrationSkillLocationId = | 'codex-home' | 'codex-plugin-cache' | 'agents-home' + | 'grok-home' + | 'opencode-home' + | 'pi-home' + | 'gemini-home' + | 'antigravity-home' + | 'cursor-home' export type OrchestrationSkillAgentStatus = { agent: TuiAgent @@ -45,6 +51,43 @@ const ORCHESTRATION_SKILL_LOCATIONS: readonly OrchestrationSkillLocationDefiniti matchesSkill: (skill) => isGlobalOrchestrationSkill(skill) && pathContainsSegments(skill.rootPath, ['.agents', 'skills']) + }, + { + id: 'grok-home', + matchesSkill: (skill) => + isGlobalOrchestrationSkill(skill) && pathContainsSegments(skill.rootPath, ['.grok', 'skills']) + }, + { + id: 'opencode-home', + matchesSkill: (skill) => + isGlobalOrchestrationSkill(skill) && + pathContainsSegments(skill.rootPath, ['.config', 'opencode', 'skills']) + }, + { + id: 'pi-home', + matchesSkill: (skill) => + isGlobalOrchestrationSkill(skill) && + pathContainsSegments(skill.rootPath, ['.pi', 'agent', 'skills']) + }, + { + // Why: segment matching keeps `.gemini/skills` (Gemini CLI) distinct from the + // sibling `.gemini/antigravity/skills`, so the two never cross-mark each other. + id: 'gemini-home', + matchesSkill: (skill) => + isGlobalOrchestrationSkill(skill) && + pathContainsSegments(skill.rootPath, ['.gemini', 'skills']) + }, + { + id: 'antigravity-home', + matchesSkill: (skill) => + isGlobalOrchestrationSkill(skill) && + pathContainsSegments(skill.rootPath, ['.gemini', 'antigravity', 'skills']) + }, + { + id: 'cursor-home', + matchesSkill: (skill) => + isGlobalOrchestrationSkill(skill) && + pathContainsSegments(skill.rootPath, ['.cursor', 'skills']) } ] @@ -52,8 +95,16 @@ const ORCHESTRATION_SKILL_LOCATION_IDS_BY_AGENT: Partial< Record > = { claude: ['claude-home', 'agents-home'], + // Why: Agent Teams runs Claude Code, so it reads the same ~/.claude skills. + 'claude-agent-teams': ['claude-home', 'agents-home'], openclaude: ['claude-home', 'agents-home'], - codex: ['codex-home', 'codex-plugin-cache', 'agents-home'] + codex: ['codex-home', 'codex-plugin-cache', 'agents-home'], + grok: ['grok-home', 'agents-home'], + opencode: ['opencode-home', 'agents-home'], + pi: ['pi-home', 'agents-home'], + gemini: ['gemini-home', 'agents-home'], + antigravity: ['antigravity-home', 'agents-home'], + cursor: ['cursor-home', 'agents-home'] } function normalizeSkillName(value: string): string { diff --git a/src/renderer/src/lib/repo-display-labels.test.ts b/src/renderer/src/lib/repo-display-labels.test.ts index f0007ffc114..4935f2da1f8 100644 --- a/src/renderer/src/lib/repo-display-labels.test.ts +++ b/src/renderer/src/lib/repo-display-labels.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { getRepoDisplayLabelsByPath } from './repo-display-labels' +import { getRepoDisplayLabelKey, getRepoDisplayLabelsByPath } from './repo-display-labels' describe('getRepoDisplayLabelsByPath', () => { it('keeps non-colliding repository names basename-only', () => { @@ -8,8 +8,10 @@ describe('getRepoDisplayLabelsByPath', () => { { path: '/workspace/platform/worker', displayName: 'worker' } ]) - expect(labels.get('/workspace/platform/web')).toBe('web') - expect(labels.get('/workspace/platform/worker')).toBe('worker') + expect(labels.get(getRepoDisplayLabelKey({ path: '/workspace/platform/web' }))).toBe('web') + expect(labels.get(getRepoDisplayLabelKey({ path: '/workspace/platform/worker' }))).toBe( + 'worker' + ) }) it('adds the minimal real parent suffix only for colliding basenames', () => { @@ -19,9 +21,13 @@ describe('getRepoDisplayLabelsByPath', () => { { path: '/workspace/platform/billing/api', displayName: 'api' } ]) - expect(labels.get('/workspace/platform/web')).toBe('web') - expect(labels.get('/workspace/platform/payments/api')).toBe('payments/api') - expect(labels.get('/workspace/platform/billing/api')).toBe('billing/api') + expect(labels.get(getRepoDisplayLabelKey({ path: '/workspace/platform/web' }))).toBe('web') + expect(labels.get(getRepoDisplayLabelKey({ path: '/workspace/platform/payments/api' }))).toBe( + 'payments/api' + ) + expect(labels.get(getRepoDisplayLabelKey({ path: '/workspace/platform/billing/api' }))).toBe( + 'billing/api' + ) }) it('expands colliding labels in lockstep without skipping shared segments', () => { @@ -30,8 +36,39 @@ describe('getRepoDisplayLabelsByPath', () => { { path: '/workspace/team2/shared/api', displayName: 'api' } ]) - expect(labels.get('/workspace/team1/shared/api')).toBe('team1/shared/api') - expect(labels.get('/workspace/team2/shared/api')).toBe('team2/shared/api') + expect(labels.get(getRepoDisplayLabelKey({ path: '/workspace/team1/shared/api' }))).toBe( + 'team1/shared/api' + ) + expect(labels.get(getRepoDisplayLabelKey({ path: '/workspace/team2/shared/api' }))).toBe( + 'team2/shared/api' + ) + }) + + it('scopes labels by execution host so same-path repos on different hosts do not collide', () => { + // Real SSH folder-repo shape: connectionId set, executionHostId unset — so + // it must fall back to the connection host, not look identical to a local repo. + const localRepo = { path: '/Users/alice', displayName: 'alice' } + const sshRepo = { path: '/Users/alice', displayName: 'alice-prod', connectionId: 'prod-ssh' } + const labels = getRepoDisplayLabelsByPath([localRepo, sshRepo]) + + expect(labels.get(getRepoDisplayLabelKey(localRepo))).toBe('alice') + expect(labels.get(getRepoDisplayLabelKey(sshRepo))).toBe('alice-prod') + expect(labels.size).toBe(2) + }) + + it('keeps cross-host repos with identical path AND name as separate entries', () => { + // Hardening: when paths are byte-identical the same-name collision loop runs + // and re-sets each entry, so host scoping must survive that pass too — neither + // host may overwrite the other. Label text can still coincide; that residual is + // disambiguated by the host section/badge, not this map. + const localRepo = { path: '/Users/alice', displayName: 'home' } + const sshRepo = { path: '/Users/alice', displayName: 'home', connectionId: 'prod-ssh' } + const labels = getRepoDisplayLabelsByPath([localRepo, sshRepo]) + + expect(getRepoDisplayLabelKey(localRepo)).not.toBe(getRepoDisplayLabelKey(sshRepo)) + expect(labels.size).toBe(2) + expect(labels.get(getRepoDisplayLabelKey(localRepo))).toBeDefined() + expect(labels.get(getRepoDisplayLabelKey(sshRepo))).toBeDefined() }) it('normalizes Windows separators to slash display labels', () => { @@ -40,7 +77,11 @@ describe('getRepoDisplayLabelsByPath', () => { { path: 'C:\\workspace\\billing\\api', displayName: 'api' } ]) - expect(labels.get('C:\\workspace\\payments\\api')).toBe('payments/api') - expect(labels.get('C:\\workspace\\billing\\api')).toBe('billing/api') + expect(labels.get(getRepoDisplayLabelKey({ path: 'C:\\workspace\\payments\\api' }))).toBe( + 'payments/api' + ) + expect(labels.get(getRepoDisplayLabelKey({ path: 'C:\\workspace\\billing\\api' }))).toBe( + 'billing/api' + ) }) }) diff --git a/src/renderer/src/lib/repo-display-labels.ts b/src/renderer/src/lib/repo-display-labels.ts index 995d8d65b36..89b7beeaeb6 100644 --- a/src/renderer/src/lib/repo-display-labels.ts +++ b/src/renderer/src/lib/repo-display-labels.ts @@ -1,6 +1,21 @@ +import { getRepoExecutionHostId, type ExecutionHostId } from '../../../shared/execution-host' + type RepoDisplayLabelItem = { path: string displayName: string + connectionId?: string | null + executionHostId?: ExecutionHostId | null +} + +// Why: two repos can share the same absolute path across hosts (e.g. a local +// /Users/alice and an SSH host's /Users/alice). Keying labels by raw path alone +// lets one repo's label overwrite the other's, so scope the key by execution +// host. getRepoExecutionHostId returns 'local' for local repos and falls back to +// the connectionId (ssh:) for SSH folder-repos that leave executionHostId unset. +export function getRepoDisplayLabelKey( + item: Pick +): string { + return `${getRepoExecutionHostId(item)}::${item.path}` } function normalizePathSegments(path: string): string[] { @@ -29,7 +44,7 @@ export function getRepoDisplayLabelsByPath( for (const item of items) { const displayName = item.displayName || item.path - labels.set(item.path, displayName) + labels.set(getRepoDisplayLabelKey(item), displayName) const colliding = itemsByName.get(displayName) ?? [] colliding.push({ ...item, displayName }) itemsByName.set(displayName, colliding) @@ -49,7 +64,7 @@ export function getRepoDisplayLabelsByPath( nextLabels = collidingItems.map((item) => labelForDepth(item, depth)) } collidingItems.forEach((item, index) => { - labels.set(item.path, nextLabels[index] ?? item.displayName) + labels.set(getRepoDisplayLabelKey(item), nextLabels[index] ?? item.displayName) }) } diff --git a/src/renderer/src/store/slices/editor.test.ts b/src/renderer/src/store/slices/editor.test.ts index ac54883ebe3..638fc8cb674 100644 --- a/src/renderer/src/store/slices/editor.test.ts +++ b/src/renderer/src/store/slices/editor.test.ts @@ -456,7 +456,7 @@ describe('createEditorSlice openDiff', () => { expect(store.getState().openFiles).toEqual([ expect.objectContaining({ id: 'wt-1::diff::unstaged::file.ts', - runtimeEnvironmentId: undefined + runtimeEnvironmentId: null }), expect.objectContaining({ id: 'editor-diff:wt-1:env-1:unstaged:file.ts', @@ -498,6 +498,66 @@ describe('createEditorSlice openDiff', () => { ) }) + it('keeps a diff for an owner-less worktree off the focused global runtime', () => { + const store = createEditorStore() + // A remote runtime is globally focused, but wt-1's repo names no explicit + // owner. The diff must stamp null (not undefined): null forces a LOCAL read + // in settingsForRuntimeOwner, while undefined would inherit 'focused-env'. + store.setState({ + settings: { activeRuntimeEnvironmentId: 'focused-env' } as AppState['settings'] + }) + + store.getState().openDiff('wt-1', '/repo/file.ts', 'file.ts', 'typescript', false) + + expect(store.getState().openFiles[0]).toEqual( + expect.objectContaining({ + id: 'wt-1::diff::unstaged::file.ts', + runtimeEnvironmentId: null + }) + ) + }) + + it('routes an explicitly runtime-owned worktree diff to its owner over the focused runtime', () => { + const store = createEditorStore() + store.setState({ + settings: { activeRuntimeEnvironmentId: 'focused-env' } as AppState['settings'], + repos: [ + { id: 'repo-1', executionHostId: 'runtime:owner-env' } + ] as unknown as AppState['repos'], + worktreesByRepo: { + 'repo-1': [{ id: 'repo-1::/srv/wt', repoId: 'repo-1', hostId: 'runtime:owner-env' }] + } as unknown as AppState['worktreesByRepo'] + }) + + store.getState().openDiff('repo-1::/srv/wt', '/srv/wt/file.ts', 'file.ts', 'typescript', false) + + expect(store.getState().openFiles[0]).toEqual( + expect.objectContaining({ runtimeEnvironmentId: 'owner-env' }) + ) + }) + + it('keeps an SSH-owned worktree diff off the focused runtime so it routes via its connection', () => { + const store = createEditorStore() + // An SSH worktree is owned by its connection, not the focused runtime. Its + // diff stamps null (not the focused env), so the read targets local IPC and + // flows over connectionId rather than the focused runtime's RPC. + store.setState({ + settings: { activeRuntimeEnvironmentId: 'focused-env' } as AppState['settings'], + repos: [{ id: 'repo-ssh', connectionId: 'conn-1' }] as unknown as AppState['repos'], + worktreesByRepo: { + 'repo-ssh': [{ id: 'repo-ssh::/srv/wt', repoId: 'repo-ssh', hostId: 'ssh:conn-1' }] + } as unknown as AppState['worktreesByRepo'] + }) + + store + .getState() + .openDiff('repo-ssh::/srv/wt', '/srv/wt/file.ts', 'file.ts', 'typescript', false) + + expect(store.getState().openFiles[0]).toEqual( + expect.objectContaining({ runtimeEnvironmentId: null }) + ) + }) + it('repairs an existing diff tab entry to the correct mode and staged state', () => { const store = createEditorStore() diff --git a/src/renderer/src/store/slices/editor.ts b/src/renderer/src/store/slices/editor.ts index 7e825ec6f77..07bbab69f1c 100644 --- a/src/renderer/src/store/slices/editor.ts +++ b/src/renderer/src/store/slices/editor.ts @@ -63,7 +63,7 @@ import { import { settingsForRuntimeOwner } from '@/runtime/runtime-rpc-client' import { notifyHostOfMirroredEditorClose } from '@/runtime/close-mirrored-editor-tab' import { findWorktreeById, getRepoIdFromWorktreeId } from './worktree-helpers' -import { getRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner' +import { getExplicitRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner' import { addAdditionalValidWorkspaceKeys, type WorkspaceSessionHydrationOptions @@ -335,9 +335,11 @@ function resolveDiffRuntimeEnvironmentId( if (explicitRuntimeEnvironmentId !== undefined) { return explicitRuntimeEnvironmentId } - // Why: Source Control callers often know only the worktree. Runtime-host - // diffs still need their owner stamped so content loads through runtime RPC. - return getRuntimeEnvironmentIdForWorktree(state, worktreeId) ?? undefined + // Why: route diffs by the worktree's EXPLICIT owner (#6957); owner-less/local + // resolves to null so the read is forced LOCAL. undefined would instead + // inherit the focused runtime in settingsForRuntimeOwner and read the diff + // from the wrong host — the exact bug in #8484. + return getExplicitRuntimeEnvironmentIdForWorktree(state, worktreeId) ?? null } export type PendingEditorReveal = {