From 968e951abc6d7f71869a28807d2f4fb1bd3f7cd2 Mon Sep 17 00:00:00 2001 From: Brennan Benson Date: Fri, 10 Jul 2026 20:22:12 -0700 Subject: [PATCH] Fix Fable review findings: guard WSL hook branch, wire settings search, catalog keyword keys, sparse-env askpass, one-shot agent classification (#7652) --- src/main/git/runner.ts | 5 +++- src/main/hooks.test.ts | 10 ++++++- src/main/hooks.ts | 6 +++- src/main/ipc/repos-remote.test.ts | 15 ++++------ .../ipc/terminal-git-credential-guard.test.ts | 29 +++++++++++++++++++ src/main/ipc/terminal-git-credential-guard.ts | 18 ++++++++++-- .../components/settings/terminal-search.ts | 4 ++- src/renderer/src/i18n/locales/en.json | 11 ++++++- src/renderer/src/i18n/locales/es.json | 11 ++++++- src/renderer/src/i18n/locales/ja.json | 11 ++++++- src/renderer/src/i18n/locales/ko.json | 11 ++++++- src/renderer/src/i18n/locales/zh.json | 11 ++++++- src/shared/agent-process-recognition.ts | 17 ++++++----- src/shared/types.ts | 5 ++-- 14 files changed, 134 insertions(+), 30 deletions(-) diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index 67430c60572..bd541da3a93 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -614,7 +614,10 @@ export function promptGuardGitEnv( // would never see the guard — and WSL setups commonly use the Windows // credential manager as git's helper, which pops the same OAuth window // (issue #7652). Askpass vars are deliberately not forwarded: a Windows - // askpass path is meaningless inside the distro. + // askpass path is meaningless inside the distro. GIT_CONFIG_* must be + // forwarded all-or-nothing (including caller-preexisting entries): a + // GIT_CONFIG_COUNT that crosses without every KEY_n/VALUE_n below it makes + // git error out on "bogus count". addWslEnvKeys(next, [ 'GIT_TERMINAL_PROMPT', 'GCM_INTERACTIVE', diff --git a/src/main/hooks.test.ts b/src/main/hooks.test.ts index bbcccc24211..a92900c63a0 100644 --- a/src/main/hooks.test.ts +++ b/src/main/hooks.test.ts @@ -956,7 +956,15 @@ describe('runHook', () => { expect(execFileMock).toHaveBeenCalledWith( 'wsl.exe', ['-d', 'Ubuntu', '--', 'bash', '-c', "cd '/home/jin/feature' && echo hello"], - expect.any(Object), + // #7652 regression: the unattended WSL hook branch must carry the + // credential guard, and WSLENV is what carries it into the distro. + expect.objectContaining({ + env: expect.objectContaining({ + GIT_TERMINAL_PROMPT: '0', + GCM_INTERACTIVE: 'never', + WSLENV: expect.stringContaining('GIT_TERMINAL_PROMPT') + }) + }), expect.any(Function) ) expect(execMock).not.toHaveBeenCalled() diff --git a/src/main/hooks.ts b/src/main/hooks.ts index 6dfb3db2fc6..c7d56b94651 100644 --- a/src/main/hooks.ts +++ b/src/main/hooks.ts @@ -636,7 +636,11 @@ export function runHook( { timeout: HOOK_TIMEOUT, encoding: 'utf-8', - env: { ...process.env, ...wslEnv } + // Why: same unattended-git guard as the non-WSL branch below + // (issue #7652) — WSL repos are the likeliest to hit the GCM + // popup, and the guard's WSLENV registration is what carries it + // across the wsl.exe boundary into the distro. + env: promptGuardShellEnv({ ...process.env, ...wslEnv }) }, (error, stdout, stderr) => { finish(error ?? null, stdout, stderr) diff --git a/src/main/ipc/repos-remote.test.ts b/src/main/ipc/repos-remote.test.ts index 7f649ff755f..2a0336d8c12 100644 --- a/src/main/ipc/repos-remote.test.ts +++ b/src/main/ipc/repos-remote.test.ts @@ -9,6 +9,7 @@ import { existsSync } from 'node:fs' import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' +import type * as GitRunner from '../git/runner' import type * as RepoModule from '../git/repo' import { DEFAULT_REPO_BADGE_COLOR } from '../../shared/constants' import { getGitRepoRoot, isGitRepo } from '../git/repo' @@ -102,16 +103,12 @@ vi.mock('../git/repo', async () => { } }) -vi.mock('../git/runner', () => ({ +vi.mock('../git/runner', async () => ({ + // Why: keep the real nonInteractiveGitEnv so the clone regression test + // (#7652) asserts the actual guard's markers, not a mock echoing itself. + ...(await vi.importActual('../git/runner')), gitExecFileAsync: vi.fn(), - gitSpawn: gitSpawnMock, - // Mirrors the real guard's credential-prompt markers so the clone regression - // test (#7652) can assert Git Credential Manager's OAuth popup is disabled. - nonInteractiveGitEnv: (env: NodeJS.ProcessEnv = process.env) => ({ - ...env, - GIT_TERMINAL_PROMPT: '0', - GCM_INTERACTIVE: 'never' - }) + gitSpawn: gitSpawnMock })) vi.mock('../git/worktree', () => ({ diff --git a/src/main/ipc/terminal-git-credential-guard.test.ts b/src/main/ipc/terminal-git-credential-guard.test.ts index 704ff9d2a21..f8711434b28 100644 --- a/src/main/ipc/terminal-git-credential-guard.test.ts +++ b/src/main/ipc/terminal-git-credential-guard.test.ts @@ -50,6 +50,35 @@ describe('applyTerminalGitCredentialPromptGuard', () => { expect(wslenvKeys).not.toContain('SSH_ASKPASS') }) + it('guards a headless one-shot agent launch — it can answer a prompt even less than a TUI', () => { + const env: Record = { PATH: '/usr/bin' } + applyTerminalGitCredentialPromptGuard(env, { + launchCommand: 'claude -p "fix the tests"', + suppressUserTerminalPrompt: false, + platform: 'darwin' + }) + expect(isGuarded(env)).toBe(true) + }) + + it('never materializes an empty askpass into a sparse daemon wire env, but keeps a caller-set one', () => { + const sparse: Record = { PATH: '/usr/bin' } + applyTerminalGitCredentialPromptGuard(sparse, { + launchCommand: 'claude', + suppressUserTerminalPrompt: true, + platform: 'win32' + }) + expect(sparse.GIT_ASKPASS).toBeUndefined() + expect(sparse.SSH_ASKPASS).toBeUndefined() + + const withAskpass: Record = { PATH: '/usr/bin', GIT_ASKPASS: 'C:\\feeder.exe' } + applyTerminalGitCredentialPromptGuard(withAskpass, { + launchCommand: 'claude', + suppressUserTerminalPrompt: true, + platform: 'win32' + }) + expect(withAskpass.GIT_ASKPASS).toBe('C:\\feeder.exe') + }) + it('never rewrites the terminal locale — the git-runner locale pins must not leak into a shell', () => { const env: Record = { PATH: '/usr/bin', LC_ALL: 'ja_JP.UTF-8' } applyTerminalGitCredentialPromptGuard(env, { diff --git a/src/main/ipc/terminal-git-credential-guard.ts b/src/main/ipc/terminal-git-credential-guard.ts index 9160a6bac86..7f1edc66067 100644 --- a/src/main/ipc/terminal-git-credential-guard.ts +++ b/src/main/ipc/terminal-git-credential-guard.ts @@ -27,7 +27,11 @@ export function applyTerminalGitCredentialPromptGuard( platform?: NodeJS.Platform } ): void { - const isAgentTerminal = Boolean(recognizeAgentProcessFromCommandLine(opts.launchCommand)) + // Why: include headless one-shots (`claude -p …`) — those can answer a + // credential prompt even less than a TUI agent can. + const isAgentTerminal = Boolean( + recognizeAgentProcessFromCommandLine(opts.launchCommand, { includeHeadlessOneShot: true }) + ) const platform = opts.platform ?? process.platform if (!isAgentTerminal && (!opts.suppressUserTerminalPrompt || platform !== 'win32')) { return @@ -35,8 +39,16 @@ export function applyTerminalGitCredentialPromptGuard( // Why: the shell variant — a terminal env is the user's whole environment, // so the git-runner locale pins (issue #7808) must not leak into it. for (const [key, value] of Object.entries(promptGuardShellEnv(env, platform))) { - if (typeof value === 'string') { - env[key] = value + if (typeof value !== 'string') { + continue } + // Why: the guard materializes GIT_ASKPASS/SSH_ASKPASS='' when absent. + // Daemon spawns pass a sparse wire env here and merge process.env later — + // a materialized '' would clobber a daemon-inherited askpass that the + // preserve-when-set design (see nonInteractiveGitEnv docs) means to keep. + if ((key === 'GIT_ASKPASS' || key === 'SSH_ASKPASS') && !(key in env)) { + continue + } + env[key] = value } } diff --git a/src/renderer/src/components/settings/terminal-search.ts b/src/renderer/src/components/settings/terminal-search.ts index 783f7085d2a..1f6c3c7e6a0 100644 --- a/src/renderer/src/components/settings/terminal-search.ts +++ b/src/renderer/src/components/settings/terminal-search.ts @@ -22,6 +22,7 @@ import { getTerminalTypographySearchEntries } from './terminal-typography-search' import { + getTerminalGitCredentialPopupSearchEntry, getTerminalRightClickToPasteSearchEntry, getTerminalWindowsPowershellImplementationSearchEntry, getTerminalWindowsShellSearchEntry @@ -112,7 +113,8 @@ export function getTerminalPaneSearchEntries(platform: { ...(isWindowsTerminalHost ? [ ...getTerminalWindowsShellSearchEntry(), - ...getTerminalWindowsPowershellImplementationSearchEntry() + ...getTerminalWindowsPowershellImplementationSearchEntry(), + ...getTerminalGitCredentialPopupSearchEntry() ] : []), ...(platform.isWindows ? getTerminalRightClickToPasteSearchEntry() : []), diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 2fb5f7016eb..f4f12c6b292 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -8515,7 +8515,16 @@ "713c4a2f92": "Choose the default shell for new terminal panes on Windows.", "13715f9d23": "Default Shell", "8630676830": "Block Git Credential Popups", - "9d8b09bc09": "Stop git in terminals from opening Git Credential Manager sign-in windows." + "9d8b09bc09": "Stop git in terminals from opening Git Credential Manager sign-in windows.", + "ba9f11ecc3": "git", + "7c7c74ef60": "credential", + "8eff361700": "popup", + "7c82e855b0": "oauth", + "bf215181b5": "github", + "b19a25c6a0": "sign in", + "ef5acdb939": "gcm", + "4ae35dbb42": "prompt", + "27e4a4878d": "credential manager" } } }, diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index ed1c28fefb5..982f57cab2e 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -8478,7 +8478,16 @@ "713c4a2f92": "Elija el shell predeterminado para los nuevos paneles de terminal en Windows.", "13715f9d23": "Shell predeterminado", "8630676830": "Block Git Credential Popups", - "9d8b09bc09": "Stop git in terminals from opening Git Credential Manager sign-in windows." + "9d8b09bc09": "Stop git in terminals from opening Git Credential Manager sign-in windows.", + "ba9f11ecc3": "git", + "7c7c74ef60": "credential", + "8eff361700": "popup", + "7c82e855b0": "oauth", + "bf215181b5": "github", + "b19a25c6a0": "sign in", + "ef5acdb939": "gcm", + "4ae35dbb42": "prompt", + "27e4a4878d": "credential manager" } } }, diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 7e26207dad9..a1f798ff39e 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -8500,7 +8500,16 @@ "713c4a2f92": "Windows 上の新規 terminal ペインのデフォルト シェルを選択します。", "13715f9d23": "デフォルトのシェル", "8630676830": "Block Git Credential Popups", - "9d8b09bc09": "Stop git in terminals from opening Git Credential Manager sign-in windows." + "9d8b09bc09": "Stop git in terminals from opening Git Credential Manager sign-in windows.", + "ba9f11ecc3": "git", + "7c7c74ef60": "credential", + "8eff361700": "popup", + "7c82e855b0": "oauth", + "bf215181b5": "github", + "b19a25c6a0": "sign in", + "ef5acdb939": "gcm", + "4ae35dbb42": "prompt", + "27e4a4878d": "credential manager" } } }, diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 989e79d913e..4bc5826c3f4 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -8463,7 +8463,16 @@ "713c4a2f92": "Windows의 새 terminal 패널에 대한 기본 셸을 선택합니다.", "13715f9d23": "기본 쉘", "8630676830": "Block Git Credential Popups", - "9d8b09bc09": "Stop git in terminals from opening Git Credential Manager sign-in windows." + "9d8b09bc09": "Stop git in terminals from opening Git Credential Manager sign-in windows.", + "ba9f11ecc3": "git", + "7c7c74ef60": "credential", + "8eff361700": "popup", + "7c82e855b0": "oauth", + "bf215181b5": "github", + "b19a25c6a0": "sign in", + "ef5acdb939": "gcm", + "4ae35dbb42": "prompt", + "27e4a4878d": "credential manager" } } }, diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 0bd3ca7359f..2955385f20e 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -8463,7 +8463,16 @@ "713c4a2f92": "为 Windows 上的新终端窗格选择默认 Shell。", "13715f9d23": "默认 Shell", "8630676830": "Block Git Credential Popups", - "9d8b09bc09": "Stop git in terminals from opening Git Credential Manager sign-in windows." + "9d8b09bc09": "Stop git in terminals from opening Git Credential Manager sign-in windows.", + "ba9f11ecc3": "git", + "7c7c74ef60": "credential", + "8eff361700": "popup", + "7c82e855b0": "oauth", + "bf215181b5": "github", + "b19a25c6a0": "sign in", + "ef5acdb939": "gcm", + "4ae35dbb42": "prompt", + "27e4a4878d": "credential manager" } } }, diff --git a/src/shared/agent-process-recognition.ts b/src/shared/agent-process-recognition.ts index cbf54eb39e9..0b0a5461656 100644 --- a/src/shared/agent-process-recognition.ts +++ b/src/shared/agent-process-recognition.ts @@ -286,17 +286,20 @@ export function recognizeAgentProcess( return { agent, processName: normalized } } export function recognizeAgentProcessFromCommandLine( - commandLine: string | null | undefined + commandLine: string | null | undefined, + // Why: TUI consumers (status hooks, shell shadows) filter out headless + // one-shots (`claude -p …`); non-interactivity guards include them — a + // one-shot agent can't answer a prompt either. + options?: { includeHeadlessOneShot?: boolean } ): RecognizedAgentProcess | null { if (!commandLine) { return null } + const keep = options?.includeHeadlessOneShot === true const tokens = tokenizeCommandLine(commandLine) const firstNormalized = normalizeProcessName(tokens[0]) - const directRecognition = filterHeadlessOneShotAgentCommand( - recognizeAgentProcess(tokens[0]), - tokens - ) + const direct = recognizeAgentProcess(tokens[0]) + const directRecognition = keep ? direct : filterHeadlessOneShotAgentCommand(direct, tokens) if (directRecognition) { return directRecognition } @@ -304,10 +307,10 @@ export function recognizeAgentProcessFromCommandLine( if (!entrypoint) { return null } - const entrypointRecognition = isPythonProcessName(firstNormalized) + const viaEntrypoint = isPythonProcessName(firstNormalized) ? recognizePythonEntrypoint(tokens, entrypoint) : (recognizeAgentProcess(entrypoint) ?? recognizeNodeScriptEntrypoint(entrypoint)) - return filterHeadlessOneShotAgentCommand(entrypointRecognition, tokens) + return keep ? viaEntrypoint : filterHeadlessOneShotAgentCommand(viaEntrypoint, tokens) } export function isAgentForegroundWrapperProcess(processName: string | null | undefined): boolean { const normalized = normalizeProcessName(processName) diff --git a/src/shared/types.ts b/src/shared/types.ts index 082f32054a3..4dd25fc7ffe 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -2563,8 +2563,9 @@ export type GlobalSettings = { * network-restricted environments (issue #7652). Only applies on Windows * hosts: the popup is a Windows credential-manager behavior, and elsewhere * the guard would only remove working tty prompts. Agent terminals and - * Orca-run setup scripts are always guarded on every platform regardless of - * this setting. Turn off to let your terminal prompt via the OS helper. */ + * Orca-run setup scripts are always guarded regardless of this setting, on + * whichever host spawns them (an SSH host applies its own guard). Turn off + * to let your terminal prompt via the OS helper. */ terminalSuppressGitCredentialPrompt?: boolean terminalWordSeparator?: string terminalCursorOpacity?: number