From b634032fa4943fc17f26a14c0c76c8b98dfe31ae Mon Sep 17 00:00:00 2001 From: Tranquil-Flow Date: Wed, 5 Aug 2026 12:44:07 +0200 Subject: [PATCH 01/37] fix(desktop): gate WSL bridge for remote backends Seed backend-mode state before creating the first window and update it after runtime resolution. Keep remote reconnects gated until a local backend is confirmed. Cover Windows child-process suppression and bridge state transitions. --- apps/desktop/electron/main.ts | 14 ++- .../electron/wsl-path-bridge-gate.test.ts | 75 ++++++++++++++++ apps/desktop/electron/wsl-path-bridge.test.ts | 88 ++++++++++++++++++- apps/desktop/electron/wsl-path-bridge.ts | 47 +++++++++- 4 files changed, 217 insertions(+), 7 deletions(-) create mode 100644 apps/desktop/electron/wsl-path-bridge-gate.test.ts diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index a1bb265ab4..b7546f3339 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -353,7 +353,7 @@ import { installWindowsSystemCaTrust } from './windows-system-ca' import { readWindowsUserEnvVar } from './windows-user-env' import { isPackagedInstallPath as isPackagedInstallPathUnderRoots } from './workspace-cwd' import { readWslWindowsClipboardImage } from './wsl-clipboard-image' -import { resolvePickerDefaultPath } from './wsl-path-bridge' +import { resolvePickerDefaultPath, setWslBridgeActive } from './wsl-path-bridge' const USER_DATA_OVERRIDE = process.env.HERMES_DESKTOP_USER_DATA_DIR @@ -10669,9 +10669,18 @@ async function startHermes() { }) if (setup.kind === 'remote') { + // Paths from the remote backend belong to a host the Windows desktop + // cannot open via wsl.exe — disable WSL path bridging so native dialogs + // and file panels don't spawn wsl.exe (or the interactive install prompt + // on WSL-less machines) for unresolvable paths. (#66433) + setWslBridgeActive(false) + return setup.connection } + // Local WSL backend — paths are bridgeable. + setWslBridgeActive(true) + const backend = setup.backend // Route old runtimes (no `serve`) through the legacy `dashboard --no-open`. backend.args = getBackendArgsForRuntime(backend) @@ -15030,6 +15039,9 @@ app.whenReady().then(() => { registerPowerResumeListeners() keepAwake.set(readPersistedKeepAwake()) f12Blocked = readPersistedDisableF12() + // Seed this before the first window exists: a picker can open before + // startHermes() finishes resolving the configured backend. + setWslBridgeActive(!primaryBackendIsRemote()) // Quick Entry's global chord — registered on ready so a cold launch restores // it without the renderer visiting Settings. A failed registration is logged // here and surfaced in Settings via the IPC state (never silent). diff --git a/apps/desktop/electron/wsl-path-bridge-gate.test.ts b/apps/desktop/electron/wsl-path-bridge-gate.test.ts new file mode 100644 index 0000000000..1b864f922e --- /dev/null +++ b/apps/desktop/electron/wsl-path-bridge-gate.test.ts @@ -0,0 +1,75 @@ +/** + * Windows-platform regression for the WSL path-bridge gate (#66433). + * + * The behavioural tests in wsl-path-bridge.test.ts prove the no-op contract + * (paths pass through unchanged when the bridge is inactive). This file goes + * one rung further: with `process.platform` stubbed to `win32` and + * `child_process.execFileSync` mocked, it proves the actual `wsl.exe` spawn is + * suppressed — not just that the return value looks right. + * + * Each test re-imports the module fresh (vi.resetModules) so IS_WINDOWS is + * re-evaluated against the stubbed platform. + */ +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest' + +const execFileSyncMock = vi.fn(() => 'Ubuntu\n') + +vi.mock('node:child_process', () => ({ execFileSync: execFileSyncMock })) + +describe('WSL bridge gate on Windows (#66433)', () => { + const realPlatform = process.platform + + beforeEach(() => { + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }) + vi.resetModules() + execFileSyncMock.mockClear() + }) + + afterEach(() => { + Object.defineProperty(process, 'platform', { value: realPlatform, configurable: true }) + }) + + test('wsl.exe IS probed for a POSIX path when the bridge is active (control)', async () => { + const { resolveLocalReadPath } = await import('./wsl-path-bridge') + resolveLocalReadPath('/home/ubuntu/project') + expect(execFileSyncMock).toHaveBeenCalled() + // Sanity: it really was wsl.exe, not some other binary. + expect(execFileSyncMock).toHaveBeenNthCalledWith( + 1, + 'wsl.exe', + expect.arrayContaining(['-l', '-q']), + expect.anything() + ) + }) + + test('wsl.exe is NEVER probed when the bridge is inactive — even for POSIX paths', async () => { + const { resolveLocalReadPath, setWslBridgeActive } = await import('./wsl-path-bridge') + setWslBridgeActive(false) + // A POSIX path that WOULD trigger bridging (and the wsl.exe probe) when + // active — but with the bridge off, resolveDefaultWslDistro is never + // reached because resolveLocalReadPath returns before it. + const result = resolveLocalReadPath('/home/ubuntu/project') + expect(execFileSyncMock).not.toHaveBeenCalled() + expect(result).toBe('/home/ubuntu/project') + }) + + test('the picker default-path also skips the wsl.exe probe when inactive', async () => { + const { resolvePickerDefaultPath, setWslBridgeActive } = await import('./wsl-path-bridge') + setWslBridgeActive(false) + const result = resolvePickerDefaultPath('/home/ubuntu') + expect(execFileSyncMock).not.toHaveBeenCalled() + expect(result).toBe('/home/ubuntu') + }) + + test('re-enabling the bridge restores wsl.exe probing', async () => { + const { resolveLocalReadPath, setWslBridgeActive } = await import('./wsl-path-bridge') + setWslBridgeActive(false) + resolveLocalReadPath('/home/ubuntu/project') + expect(execFileSyncMock).not.toHaveBeenCalled() + + setWslBridgeActive(true) + execFileSyncMock.mockClear() + resolveLocalReadPath('/home/ubuntu/project') + expect(execFileSyncMock).toHaveBeenCalled() + }) +}) diff --git a/apps/desktop/electron/wsl-path-bridge.test.ts b/apps/desktop/electron/wsl-path-bridge.test.ts index 3dcf18e7b3..52073af3c3 100644 --- a/apps/desktop/electron/wsl-path-bridge.test.ts +++ b/apps/desktop/electron/wsl-path-bridge.test.ts @@ -1,8 +1,25 @@ import assert from 'node:assert/strict' -import { test } from 'vitest' +import { afterEach, test } from 'vitest' -import { parseDefaultDistro, resolvePickerDefaultPath, wslPosixToWindowsAccessible } from './wsl-path-bridge' +import { + isWslBridgeActive, + parseDefaultDistro, + resolveLocalReadPath, + resolvePickerDefaultPath, + setWslBridgeActive, + wslPosixToWindowsAccessible +} from './wsl-path-bridge' + +// ── helpers ────────────────────────────────────────────────────────── + +/** Reset the bridge to its default active state after every test so no test + * leaks global state into the next one. */ +afterEach(() => { + setWslBridgeActive(true) +}) + +// ── distro parsing (unchanged) ─────────────────────────────────────── test('parseDefaultDistro reads the first distro from clean utf-8 output', () => { assert.equal(parseDefaultDistro('Ubuntu\nDebian\n'), 'Ubuntu') @@ -20,6 +37,8 @@ test('parseDefaultDistro strips the default-marker and blank lines', () => { assert.equal(parseDefaultDistro(' \n\n'), null) }) +// ── wslPosixToWindowsAccessible ────────────────────────────────────── + test('wslPosixToWindowsAccessible maps a drvfs mount to its Windows drive', () => { assert.equal(wslPosixToWindowsAccessible('/mnt/c/Users/alex', 'Ubuntu'), 'C:\\Users\\alex') assert.equal(wslPosixToWindowsAccessible('/mnt/d', 'Ubuntu'), 'D:\\') @@ -34,8 +53,73 @@ test('wslPosixToWindowsAccessible leaves non-absolute / already-Windows paths al assert.equal(wslPosixToWindowsAccessible('relative/dir', 'Ubuntu'), 'relative/dir') }) +// ── resolvePickerDefaultPath (bridge active) ───────────────────────── + test('resolvePickerDefaultPath bridges a WSL cwd but passes Windows paths and empties through', () => { assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu'), '\\\\wsl.localhost\\Ubuntu\\home\\alex') assert.equal(resolvePickerDefaultPath('C:\\proj', 'Ubuntu'), 'C:\\proj') assert.equal(resolvePickerDefaultPath(undefined, 'Ubuntu'), undefined) }) + +// ── bridge active / inactive ───────────────────────────────────────── + +test('bridge defaults to active', () => { + assert.equal(isWslBridgeActive(), true) +}) + +test('setWslBridgeActive(false) → resolvePickerDefaultPath passes raw path through without bridging', () => { + setWslBridgeActive(false) + // Even a clear WSL POSIX path must pass through unchanged when the bridge + // is inactive — no distro probe, no wsl.exe, no install prompt. + assert.equal(resolvePickerDefaultPath('/home/alex'), '/home/alex') + assert.equal(resolvePickerDefaultPath('/mnt/c/Users/alex'), '/mnt/c/Users/alex') + // Windows paths and empties are unaffected either way. + assert.equal(resolvePickerDefaultPath('C:\\proj'), 'C:\\proj') + assert.equal(resolvePickerDefaultPath(undefined), undefined) +}) + +test('setWslBridgeActive(false) → resolveLocalReadPath passes raw path through without bridging', () => { + setWslBridgeActive(false) + // resolveLocalReadPath is used by fs-read-dir to make WSL paths readable + // on the Windows host. When the bridge is inactive (remote gateway), the + // raw POSIX path must be returned as-is — no UNC rewriting, no distro + // resolution. The downstream fs call will fail gracefully on non-WSL + // hosts, which is the desired behaviour. + assert.equal(resolveLocalReadPath('/home/alex/proj'), '/home/alex/proj') + assert.equal(resolveLocalReadPath('/mnt/c/Users/alex'), '/mnt/c/Users/alex') + // Non-POSIX paths are never bridged regardless of state. + assert.equal(resolveLocalReadPath('C:\\Users\\alex'), 'C:\\Users\\alex') + assert.equal(resolveLocalReadPath(''), '') +}) + +test('setWslBridgeActive(true) restores picker bridging', () => { + setWslBridgeActive(false) + assert.equal(resolvePickerDefaultPath('/home/alex'), '/home/alex') + + setWslBridgeActive(true) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu'), '\\\\wsl.localhost\\Ubuntu\\home\\alex') +}) + +test('toggling the bridge is idempotent and does not corrupt cached state', () => { + // Toggle twice each way. + setWslBridgeActive(false) + assert.equal(isWslBridgeActive(), false) + setWslBridgeActive(false) + assert.equal(isWslBridgeActive(), false) + + setWslBridgeActive(true) + assert.equal(isWslBridgeActive(), true) + setWslBridgeActive(true) + assert.equal(isWslBridgeActive(), true) + + // Bridging still works after the toggles. + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu'), '\\\\wsl.localhost\\Ubuntu\\home\\alex') +}) + +// ── state isolation: every test sees a clean active bridge ──────────── + +test('state isolation: bridge is active after a previous test toggled it off', () => { + // This test relies on afterEach resetting the bridge. + // If isolation is broken, isWslBridgeActive() would be false here. + assert.equal(isWslBridgeActive(), true) +}) diff --git a/apps/desktop/electron/wsl-path-bridge.ts b/apps/desktop/electron/wsl-path-bridge.ts index 60d3c0cf18..c83a217d17 100644 --- a/apps/desktop/electron/wsl-path-bridge.ts +++ b/apps/desktop/electron/wsl-path-bridge.ts @@ -16,6 +16,28 @@ const WSL_MOUNT_RE = /^\/mnt\/([a-z])(?:\/(.*))?$/i let cachedDistro: null | string = null let cachedUncBase: null | string = null +/** + * Whether WSL path bridging is active. The bridge only makes sense when the + * desktop runs on Windows AND the gateway is a *local* backend (e.g. running + * inside WSL on the same machine). When the gateway is a remote host, the + * POSIX paths it reports belong to a machine the Windows host cannot open via + * `wsl.exe` — bridging them only spawns `wsl.exe` (and on WSL-less machines, + * the interactive "Install WSL" console prompt) for paths that can never be + * resolved locally. main.ts toggles this off once it resolves a remote + * backend. Defaults to active so a local Windows+WSL boot is unaffected. + */ +let wslBridgeActive = true + +/** Enable/disable WSL path bridging at runtime (called by main.ts). */ +export function setWslBridgeActive(active: boolean): void { + wslBridgeActive = active +} + +/** Test seam: is the bridge currently active? */ +export function isWslBridgeActive(): boolean { + return wslBridgeActive +} + /** * Pick the default distro from `wsl.exe -l -q` output. * @@ -116,22 +138,39 @@ export function wslPosixToWindowsAccessible(posixPath: string, distro: string = /** Native folder dialog `defaultPath`: open a WSL cwd in the Windows picker. */ export function resolvePickerDefaultPath( defaultPath: string | undefined, - distro: string = resolveDefaultWslDistro() + distro?: string ): string | undefined { if (!defaultPath) { return undefined } + // Remote-gateway POSIX paths can't be opened via wsl.exe — no-op the bridge + // so the native dialog gets the raw path (it falls back gracefully) instead + // of triggering a wsl.exe spawn / install prompt. (#66433) + if (!wslBridgeActive) { + return defaultPath + } + const value = String(defaultPath).trim() - return value.startsWith('/') && !WIN_DRIVE_RE.test(value) ? wslPosixToWindowsAccessible(value, distro) : defaultPath + return value.startsWith('/') && !WIN_DRIVE_RE.test(value) + ? wslPosixToWindowsAccessible(value, distro ?? resolveDefaultWslDistro()) + : defaultPath } /** fs read path: on Windows, make a WSL cwd readable via its UNC / drive form. */ -export function resolveLocalReadPath(dirPath: string, distro: string = resolveDefaultWslDistro()): string { +export function resolveLocalReadPath(dirPath: string, distro?: string): string { const value = String(dirPath || '').trim() + // In remote-gateway mode the POSIX paths belong to a host the Windows + // desktop cannot open locally — skip the WSL bridge entirely (no distro + // probe, no wsl.exe) so the file panel never spawns the install prompt on + // WSL-less machines. (#66433) + if (!wslBridgeActive) { + return value + } + return IS_WINDOWS && value.startsWith('/') && !WIN_DRIVE_RE.test(value) - ? wslPosixToWindowsAccessible(value, distro) + ? wslPosixToWindowsAccessible(value, distro ?? resolveDefaultWslDistro()) : value } From deec0432765680c704658e36697a49ce488e68a1 Mon Sep 17 00:00:00 2001 From: Tranquil-Flow Date: Thu, 6 Aug 2026 21:55:05 +0200 Subject: [PATCH 02/37] fix(desktop): scope WSL bridge state by profile --- apps/desktop/electron/main.ts | 33 ++- .../electron/wsl-path-bridge-profile.test.ts | 242 ++++++++++++++++++ apps/desktop/electron/wsl-path-bridge.ts | 53 ++-- apps/desktop/src/global.d.ts | 2 + apps/desktop/src/lib/desktop-fs.test.ts | 8 +- apps/desktop/src/lib/desktop-fs.ts | 6 +- 6 files changed, 311 insertions(+), 33 deletions(-) create mode 100644 apps/desktop/electron/wsl-path-bridge-profile.test.ts diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index b7546f3339..adf7338ef5 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -353,7 +353,7 @@ import { installWindowsSystemCaTrust } from './windows-system-ca' import { readWindowsUserEnvVar } from './windows-user-env' import { isPackagedInstallPath as isPackagedInstallPathUnderRoots } from './workspace-cwd' import { readWslWindowsClipboardImage } from './wsl-clipboard-image' -import { resolvePickerDefaultPath, setWslBridgeActive } from './wsl-path-bridge' +import { resolvePickerDefaultPath, setActiveGatewayProfile, setWslBridgeProfileState } from './wsl-path-bridge' const USER_DATA_OVERRIDE = process.env.HERMES_DESKTOP_USER_DATA_DIR @@ -9898,6 +9898,7 @@ async function ensureBackend(profile) { if (route.backend === 'primary') { const connection = await startHermes() + setWslBridgeProfileState(key, connection.mode !== 'remote') // A shared backend still owes the caller its profile scope, so renderer-side // WebSocket, filesystem, and cache routing target the selected profile. @@ -9921,8 +9922,10 @@ async function ensureBackend(profile) { if (existing) { existing.lastActiveAt = Date.now() + const connection = await existing.connectionPromise + setWslBridgeProfileState(key, connection.mode !== 'remote') - return existing.connectionPromise + return connection } evictLruPoolBackends(POOL_MAX_BACKENDS - 1) @@ -9955,7 +9958,10 @@ async function ensureBackend(profile) { backendPool.set(key, entry) startPoolIdleReaper() - return entry.connectionPromise + const connection = await entry.connectionPromise + setWslBridgeProfileState(key, connection.mode !== 'remote') + + return connection } // ── Registry-scoped backends (multi-connection, PR 2 of the campaign) ────── @@ -10579,6 +10585,11 @@ async function startHermes() { } const connectionAttempt = backendConnectionState.startAttempt() + const primaryProfile = primaryProfileKey() + + // Legacy path callers without an explicit profile belong to the primary + // window backend. Profile-scoped callers still pass their key directly. + setActiveGatewayProfile(primaryProfile) // Classify this boot BEFORE the throwing resolve/mint runs: a remote failure // must NOT latch (it's transient — see shouldLatchBackendStartFailure), while @@ -10660,7 +10671,7 @@ async function startHermes() { // both for an already-saved remote and after first-run remote Apply. attemptedRemote = primaryBackendIsRemote() - return resolveRemoteBackend(primaryProfileKey()) + return resolveRemoteBackend(primaryProfile) }, waitForDecision: waitForFirstRunSetupChoice, // Mutual exclusion with an in-app update (#50238). Remote connections @@ -10673,13 +10684,13 @@ async function startHermes() { // cannot open via wsl.exe — disable WSL path bridging so native dialogs // and file panels don't spawn wsl.exe (or the interactive install prompt // on WSL-less machines) for unresolvable paths. (#66433) - setWslBridgeActive(false) + setWslBridgeProfileState(primaryProfile, false) return setup.connection } // Local WSL backend — paths are bridgeable. - setWslBridgeActive(true) + setWslBridgeProfileState(primaryProfile, true) const backend = setup.backend // Route old runtimes (no `serve`) through the legacy `dashboard --no-open`. @@ -13949,7 +13960,10 @@ ipcMain.handle('hermes:selectPaths', async (_event, options: any = {}) => { try { // On a Windows host with a WSL backend the cwd may be a POSIX/WSL path; // bridge it to a UNC/drive form the native dialog can actually open. - const bridged = IS_WINDOWS ? resolvePickerDefaultPath(String(options.defaultPath)) : String(options.defaultPath) + const bridged = IS_WINDOWS + ? resolvePickerDefaultPath(String(options.defaultPath), undefined, options?.profile) + : String(options.defaultPath) + resolvedDefaultPath = bridged ? path.resolve(bridged) : undefined } catch { resolvedDefaultPath = undefined @@ -15041,7 +15055,10 @@ app.whenReady().then(() => { f12Blocked = readPersistedDisableF12() // Seed this before the first window exists: a picker can open before // startHermes() finishes resolving the configured backend. - setWslBridgeActive(!primaryBackendIsRemote()) + const primaryProfile = primaryProfileKey() + + setActiveGatewayProfile(primaryProfile) + setWslBridgeProfileState(primaryProfile, !primaryBackendIsRemote()) // Quick Entry's global chord — registered on ready so a cold launch restores // it without the renderer visiting Settings. A failed registration is logged // here and surfaced in Settings via the IPC state (never silent). diff --git a/apps/desktop/electron/wsl-path-bridge-profile.test.ts b/apps/desktop/electron/wsl-path-bridge-profile.test.ts new file mode 100644 index 0000000000..8f83e0fa00 --- /dev/null +++ b/apps/desktop/electron/wsl-path-bridge-profile.test.ts @@ -0,0 +1,242 @@ +/** + * Profile-scoped eligibility for the WSL path bridge (#66447). + * + * The single-profile tests in wsl-path-bridge.test.ts and the Windows-platform + * gate tests in wsl-path-bridge-gate.test.ts cover the *what* (paths pass + * through unchanged when bridging is disabled) but not the *which profile*. The + * desktop is multi-profile: the renderer can swap the live gateway onto any + * profile (primary or pool) without reloading the window — so the bridge + * eligibility MUST be keyed off the **currently active profile's** backend + * configuration, not a process-global boolean. This file proves the + * per-profile contract: + * + * 1. local primary profile → bridge ON (preserved) + * 2. remote primary profile → bridge OFF (preserved) + * 3. local primary + remote non-primary → bridge OFF when the non-primary + * is active; bridge ON when the primary is active again — **no bleed**. + * 4. remote primary + local non-primary → bridge ON when the non-primary + * is active; bridge OFF when the primary is active again — **no bleed**. + * 5. profile-scoped calls accept a profile argument; absent it falls back + * to the live gateway profile that the renderer announced. + * 6. afterEach resets state so tests don't bleed into each other. + * + * These tests are pure behavior: they assert the eligibility function's output + * and the public selectors' output against observable calls. They do NOT read + * the implementation source — only the public surface exported from + * `./wsl-path-bridge`. + */ +import assert from 'node:assert/strict' + +import { afterEach, describe, test } from 'vitest' + +import { + isWslBridgeActive, + resolveLocalReadPath, + resolvePickerDefaultPath, + setActiveGatewayProfile, + setWslBridgeActive, + setWslBridgeProfileState, + wslPosixToWindowsAccessible +} from './wsl-path-bridge' + +// ── helpers ────────────────────────────────────────────────────────── + +const PROFILE_PRIMARY = 'default' +const PROFILE_LOCAL = 'team-local' +const PROFILE_REMOTE = 'team-remote' + +/** Reset every profile key the bridge knows about to a clean state. */ +afterEach(() => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_LOCAL, true) + setWslBridgeProfileState(PROFILE_REMOTE, false) + setActiveGatewayProfile(PROFILE_PRIMARY) + setWslBridgeActive(true) +}) + +// ── single-profile contract (preserved behaviour) ──────────────────── + +describe('WSL bridge profile eligibility — single-profile contract preserved', () => { + test('primary local → bridge ON (preserved)', () => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setActiveGatewayProfile(PROFILE_PRIMARY) + assert.equal(isWslBridgeActive(), true) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + }) + + test('primary remote → bridge OFF (preserved)', () => { + setWslBridgeProfileState(PROFILE_PRIMARY, false) + setActiveGatewayProfile(PROFILE_PRIMARY) + assert.equal(isWslBridgeActive(), false) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), '/home/alex') + assert.equal(resolveLocalReadPath('/home/alex/proj', undefined, PROFILE_PRIMARY), '/home/alex/proj') + }) +}) + +// ── multi-profile regression (the gap) ──────────────────────────────── + +describe('WSL bridge profile eligibility — multi-profile (no cross-profile bleed)', () => { + test('local primary + remote non-primary → non-primary OFF, primary ON', () => { + // Primary is a local backend (WSL on this Windows host). A second profile + // points at a remote host whose POSIX paths the Windows host CANNOT open + // via wsl.exe — bridging must be OFF for it. + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_REMOTE, false) + + // Non-primary remote is foregrounded — bridge must be OFF for it. + setActiveGatewayProfile(PROFILE_REMOTE) + assert.equal(isWslBridgeActive(), false) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_REMOTE), '/home/alex') + assert.equal(resolveLocalReadPath('/home/alex/proj', undefined, PROFILE_REMOTE), '/home/alex/proj') + + // Swap back to local primary — bridge must be ON again, no bleed. + setActiveGatewayProfile(PROFILE_PRIMARY) + assert.equal(isWslBridgeActive(), true) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + }) + + test('remote primary + local non-primary → non-primary ON, primary OFF', () => { + // Primary is remote (no local WSL paths). A second profile is local — + // bridging should be ON for it because its paths CAN be opened locally. + setWslBridgeProfileState(PROFILE_PRIMARY, false) + setWslBridgeProfileState(PROFILE_LOCAL, true) + + // Local non-primary foregrounded — bridge ON for it. + setActiveGatewayProfile(PROFILE_LOCAL) + assert.equal(isWslBridgeActive(), true) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_LOCAL), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + + // Swap back to remote primary — bridge OFF for it, no bleed. + setActiveGatewayProfile(PROFILE_PRIMARY) + assert.equal(isWslBridgeActive(), false) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), '/home/alex') + }) + + test('three profiles: each profile behaves independently', () => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_LOCAL, true) + setWslBridgeProfileState(PROFILE_REMOTE, false) + + // Same path, different profiles, different outcomes. + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_LOCAL), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_REMOTE), '/home/alex') + }) +}) + +// ── profile-argument contract ──────────────────────────────────────── + +describe('WSL bridge profile eligibility — selector argument contract', () => { + test("selector with explicit profile → that profile's bridge state", () => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_REMOTE, false) + setActiveGatewayProfile(PROFILE_PRIMARY) + + // Explicit profile wins over the live gateway. + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_REMOTE), '/home/alex') + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + }) + + test("selector with no profile → live gateway profile's bridge state", () => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_REMOTE, false) + setActiveGatewayProfile(PROFILE_REMOTE) + + // No profile argument → fallback to active gateway profile (remote → OFF). + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu'), '/home/alex') + assert.equal(resolveLocalReadPath('/home/alex/proj'), '/home/alex/proj') + + setActiveGatewayProfile(PROFILE_PRIMARY) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu'), '\\\\wsl.localhost\\Ubuntu\\home\\alex') + }) + + test('selector with unknown profile → bridge ON (defaults to active for new profiles)', () => { + // A profile that has never been seeded should default to the safe + // "bridge ON" behaviour so a brand-new local profile isn't accidentally + // disabled. The renderer seeds the state at boot; an unknown key here is + // either a renderer race or a profile created mid-session. + setWslBridgeProfileState(PROFILE_PRIMARY, false) + setActiveGatewayProfile(PROFILE_PRIMARY) + + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', 'unknown-profile'), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + }) +}) + +// ── legacy back-compat: setWslBridgeActive targets the active profile ─ + +describe('WSL bridge profile eligibility — legacy toggle targets active profile', () => { + test('setWslBridgeActive(false) flips the active profile, not a process-global', () => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_REMOTE, true) + setActiveGatewayProfile(PROFILE_REMOTE) + + setWslBridgeActive(false) + + // Active profile (REMOTE) flipped to OFF; primary untouched. + assert.equal(isWslBridgeActive(), false) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + assert.equal(resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_REMOTE), '/home/alex') + }) + + test('setWslBridgeActive(true) restores the active profile only', () => { + setWslBridgeProfileState(PROFILE_PRIMARY, true) + setWslBridgeProfileState(PROFILE_REMOTE, false) + setActiveGatewayProfile(PROFILE_REMOTE) + + setWslBridgeActive(true) + + // Active (REMOTE) restored; primary unchanged. + assert.equal(isWslBridgeActive(), true) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_REMOTE), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + assert.equal( + resolvePickerDefaultPath('/home/alex', 'Ubuntu', PROFILE_PRIMARY), + '\\\\wsl.localhost\\Ubuntu\\home\\alex' + ) + }) +}) + +// ── wslPosixToWindowsAccessible stays pure / unchanged ─────────────── + +describe('WSL bridge profile eligibility — POSIX translation stays pure', () => { + test('wslPosixToWindowsAccessible ignores bridge state (pure translation)', () => { + setWslBridgeProfileState(PROFILE_PRIMARY, false) + setActiveGatewayProfile(PROFILE_PRIMARY) + + // The translator does NOT consult the bridge state — it's pure POSIX → UNC. + // Tests elsewhere assert that the bridge gate short-circuits BEFORE this + // function is reached. A regression here would mean coupling leaked into + // a helper that should stay side-effect-free. + assert.equal( + wslPosixToWindowsAccessible('/home/alex/proj', 'Ubuntu'), + '\\\\wsl.localhost\\Ubuntu\\home\\alex\\proj' + ) + assert.equal(wslPosixToWindowsAccessible('/mnt/c/Users/alex', 'Ubuntu'), 'C:\\Users\\alex') + }) +}) diff --git a/apps/desktop/electron/wsl-path-bridge.ts b/apps/desktop/electron/wsl-path-bridge.ts index c83a217d17..74b991643c 100644 --- a/apps/desktop/electron/wsl-path-bridge.ts +++ b/apps/desktop/electron/wsl-path-bridge.ts @@ -17,25 +17,39 @@ let cachedDistro: null | string = null let cachedUncBase: null | string = null /** - * Whether WSL path bridging is active. The bridge only makes sense when the - * desktop runs on Windows AND the gateway is a *local* backend (e.g. running - * inside WSL on the same machine). When the gateway is a remote host, the - * POSIX paths it reports belong to a machine the Windows host cannot open via - * `wsl.exe` — bridging them only spawns `wsl.exe` (and on WSL-less machines, - * the interactive "Install WSL" console prompt) for paths that can never be - * resolved locally. main.ts toggles this off once it resolves a remote - * backend. Defaults to active so a local Windows+WSL boot is unaffected. + * WSL path eligibility belongs to the backend profile that produced the path. + * A single desktop process can keep a local primary backend and a remote pool + * backend alive simultaneously, so a process-global boolean can bleed between + * them. Unknown profiles retain the historical local default until Electron + * resolves and records their actual backend mode. */ -let wslBridgeActive = true +const DEFAULT_WSL_BRIDGE_PROFILE = 'default' +const wslBridgeProfiles = new Map() +let activeWslBridgeProfile = DEFAULT_WSL_BRIDGE_PROFILE -/** Enable/disable WSL path bridging at runtime (called by main.ts). */ -export function setWslBridgeActive(active: boolean): void { - wslBridgeActive = active +function normalizeWslBridgeProfile(profile?: null | string): string { + return String(profile || '').trim() || DEFAULT_WSL_BRIDGE_PROFILE } -/** Test seam: is the bridge currently active? */ -export function isWslBridgeActive(): boolean { - return wslBridgeActive +/** Select the profile used by legacy callers that cannot pass one explicitly. */ +export function setActiveGatewayProfile(profile?: null | string): void { + activeWslBridgeProfile = normalizeWslBridgeProfile(profile) +} + +/** Record whether paths returned by one profile belong to this host's WSL. */ +export function setWslBridgeProfileState(profile: null | string, active: boolean): void { + wslBridgeProfiles.set(normalizeWslBridgeProfile(profile), Boolean(active)) +} + +/** Backward-compatible toggle: update only the current fallback profile. */ +export function setWslBridgeActive(active: boolean): void { + setWslBridgeProfileState(activeWslBridgeProfile, active) +} + +export function isWslBridgeActive(profile?: null | string): boolean { + const key = profile == null ? activeWslBridgeProfile : normalizeWslBridgeProfile(profile) + + return wslBridgeProfiles.get(key) ?? true } /** @@ -138,7 +152,8 @@ export function wslPosixToWindowsAccessible(posixPath: string, distro: string = /** Native folder dialog `defaultPath`: open a WSL cwd in the Windows picker. */ export function resolvePickerDefaultPath( defaultPath: string | undefined, - distro?: string + distro?: string, + profile?: null | string ): string | undefined { if (!defaultPath) { return undefined @@ -147,7 +162,7 @@ export function resolvePickerDefaultPath( // Remote-gateway POSIX paths can't be opened via wsl.exe — no-op the bridge // so the native dialog gets the raw path (it falls back gracefully) instead // of triggering a wsl.exe spawn / install prompt. (#66433) - if (!wslBridgeActive) { + if (!isWslBridgeActive(profile)) { return defaultPath } @@ -159,14 +174,14 @@ export function resolvePickerDefaultPath( } /** fs read path: on Windows, make a WSL cwd readable via its UNC / drive form. */ -export function resolveLocalReadPath(dirPath: string, distro?: string): string { +export function resolveLocalReadPath(dirPath: string, distro?: string, profile?: null | string): string { const value = String(dirPath || '').trim() // In remote-gateway mode the POSIX paths belong to a host the Windows // desktop cannot open locally — skip the WSL bridge entirely (no distro // probe, no wsl.exe) so the file panel never spawns the install prompt on // WSL-less machines. (#66433) - if (!wslBridgeActive) { + if (!isWslBridgeActive(profile)) { return value } diff --git a/apps/desktop/src/global.d.ts b/apps/desktop/src/global.d.ts index bf85c94f35..98db0b39b3 100644 --- a/apps/desktop/src/global.d.ts +++ b/apps/desktop/src/global.d.ts @@ -1311,6 +1311,8 @@ export interface HermesSelectPathsOptions { defaultPath?: string directories?: boolean multiple?: boolean + /** Backend profile that produced defaultPath; Electron uses it for WSL gating. */ + profile?: string filters?: Array<{ name: string; extensions: string[] }> } diff --git a/apps/desktop/src/lib/desktop-fs.test.ts b/apps/desktop/src/lib/desktop-fs.test.ts index 2a3d0c54e6..274e5b0440 100644 --- a/apps/desktop/src/lib/desktop-fs.test.ts +++ b/apps/desktop/src/lib/desktop-fs.test.ts @@ -76,7 +76,7 @@ describe('desktop filesystem facade', () => { }) it('uses local Electron filesystem methods in local mode', async () => { - $connection.set({ mode: 'local' } as never) + $connection.set({ mode: 'local', profile: 'team-local' } as never) await expect(readDesktopDir('/work')).resolves.toEqual({ entries: [{ name: 'local', path: '/local', isDirectory: true }] @@ -90,7 +90,7 @@ describe('desktop filesystem facade', () => { expect(readFileText).toHaveBeenCalledWith('/work/file.txt') expect(readFileDataUrl).toHaveBeenCalledWith('/work/file.txt') expect(gitRoot).toHaveBeenCalledWith('/work') - expect(selectPaths).toHaveBeenCalledWith({ directories: true }) + expect(selectPaths).toHaveBeenCalledWith({ directories: true, profile: 'team-local' }) expect(api).not.toHaveBeenCalled() }) @@ -216,12 +216,12 @@ describe('desktop filesystem facade', () => { it('uses the local Electron picker for remote file selection', async () => { const remoteSelect = vi.fn(async () => ['/remote/project']) - $connection.set({ mode: 'remote' } as never) + $connection.set({ mode: 'remote', profile: 'team-remote' } as never) setDesktopFsRemotePicker({ selectPaths: remoteSelect }) await expect(selectDesktopPaths({ directories: false, multiple: false })).resolves.toEqual(['/local']) - expect(selectPaths).toHaveBeenCalledWith({ directories: false, multiple: false }) + expect(selectPaths).toHaveBeenCalledWith({ directories: false, multiple: false, profile: 'team-remote' }) expect(remoteSelect).not.toHaveBeenCalled() }) diff --git a/apps/desktop/src/lib/desktop-fs.ts b/apps/desktop/src/lib/desktop-fs.ts index d2ebc3ac10..6123197c68 100644 --- a/apps/desktop/src/lib/desktop-fs.ts +++ b/apps/desktop/src/lib/desktop-fs.ts @@ -201,13 +201,15 @@ export async function desktopFileDiff(repoRoot: string, filePath: string): Promi export async function selectDesktopPaths(options?: HermesSelectPathsOptions): Promise { const desktop = bridge() + const profile = desktopFsProfile() + const localOptions = profile ? { ...options, profile } : options if (!isDesktopFsRemoteMode()) { - return desktop.selectPaths(options) + return desktop.selectPaths(localOptions) } if (!options?.directories) { - return desktop.selectPaths(options) + return desktop.selectPaths(localOptions) } return remotePicker ? remotePicker.selectPaths({ ...options, multiple: false }) : [] From 0a8cdec697ee5830a1df23c5e1f247fa1f2efefd Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 21 Aug 2026 00:39:33 -0700 Subject: [PATCH 03/37] fix(desktop): silence wsl.exe stderr banner + detached explorer relaunch rung Follow-ups to the salvaged WSL-bridge gating (#66447): - wsl-path-bridge.ts: discard wsl.exe stderr so the 'WSL is not installed' banner can never leak into an attached console on WSL-less machines (#80184). - scripts/desktop-update/windows.ps1: add an explorer.exe-mediated detached relaunch rung between the WMI attempt and the tethered Start-Process fallback. When Win32_Process.Create fails (observed ReturnValue 8), the Desktop no longer re-attaches to the hand-off console, so its stdout stops flooding the window and the console can close. --- apps/desktop/electron/wsl-path-bridge.ts | 5 +++ scripts/desktop-update/windows.ps1 | 53 ++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/apps/desktop/electron/wsl-path-bridge.ts b/apps/desktop/electron/wsl-path-bridge.ts index 74b991643c..feff32a2f7 100644 --- a/apps/desktop/electron/wsl-path-bridge.ts +++ b/apps/desktop/electron/wsl-path-bridge.ts @@ -86,6 +86,11 @@ export function resolveDefaultWslDistro(): string { const out = execFileSync('wsl.exe', ['-l', '-q'], { encoding: 'utf8', env: { ...process.env, WSL_UTF8: '1' }, + // On WSL-less machines wsl.exe prints "The Windows Subsystem for Linux + // is not installed..." to stderr; stderr is inherited by default, so + // that banner leaks into whatever console the app is attached to + // (visible e.g. during the update hand-off). Discard it. (#80184) + stdio: ['ignore', 'pipe', 'ignore'], timeout: 2000, windowsHide: true }) diff --git a/scripts/desktop-update/windows.ps1 b/scripts/desktop-update/windows.ps1 index f0bfd52839..5787519495 100644 --- a/scripts/desktop-update/windows.ps1 +++ b/scripts/desktop-update/windows.ps1 @@ -571,6 +571,59 @@ function Start-DesktopRelaunch { } catch { Write-HandoffLog "WARNING: WMI relaunch failed: $($_.Exception.Message); falling back" } + if (-not $spawned) { + # Middle rung: explorer.exe-mediated launch. On some machines + # Win32_Process.Create fails outright (observed ReturnValue 8, + # "unknown failure"), and the tethered fallback below re-attaches the + # Desktop to this console — its stdout then floods the console and the + # window can't close while the app lives. Explorer re-parents the + # target exactly like a normal shell launch, giving the same + # no-console detachment WMI would have. Explorer returns no pid, so + # verify by watching for a fresh Hermes process. + try { + $exeName = [System.IO.Path]::GetFileNameWithoutExtension($RelaunchExe) + $before = @(Get-Process -Name $exeName -ErrorAction SilentlyContinue | ForEach-Object { $_.Id }) + Start-Process -FilePath 'explorer.exe' -ArgumentList ('"{0}"' -f $RelaunchExe) | Out-Null + $explorerDeadline = (Get-Date).AddSeconds(15) + while ((Get-Date) -lt $explorerDeadline) { + $fresh = @(Get-Process -Name $exeName -ErrorAction SilentlyContinue | Where-Object { $before -notcontains $_.Id }) + if ($fresh.Count -gt 0) { + Write-HandoffLog "desktop relaunched detached via explorer (pid $($fresh[0].Id))" + $spawned = $true + # Same foreground hand-off as the WMI rung: the new process + # starts unfocused and only the current foreground owner + # (us) can delegate that right. + try { + if ($script:Win32) { + [HermesHandoff.Win32]::AllowSetForegroundWindow([int]$fresh[0].Id) | Out-Null + $focusDeadline = (Get-Date).AddSeconds(20) + while ((Get-Date) -lt $focusDeadline) { + $hwnd = [System.IntPtr]::Zero + try { $hwnd = (Get-Process -Id $fresh[0].Id -ErrorAction Stop).MainWindowHandle } catch { break } + if ($hwnd -ne [System.IntPtr]::Zero) { + [HermesHandoff.Win32]::ShowWindow($hwnd, 9) | Out-Null # SW_RESTORE + [HermesHandoff.Win32]::SetForegroundWindow($hwnd) | Out-Null + Write-HandoffLog "focused relaunched desktop window" + break + } + Start-Sleep -Milliseconds 400 + } + } + } catch { + Write-HandoffLog "WARNING: could not focus relaunched desktop: $($_.Exception.Message)" + } + break + } + Start-Sleep -Milliseconds 400 + if ($script:Ui) { [System.Windows.Forms.Application]::DoEvents() } + } + if (-not $spawned) { + Write-HandoffLog "WARNING: explorer relaunch did not produce a $exeName process; falling back" + } + } catch { + Write-HandoffLog "WARNING: explorer relaunch failed: $($_.Exception.Message); falling back" + } + } if (-not $spawned) { try { # Fallback keeps the old behavior (console tie-in and all) -- From fc9cbc872d8050c22f1192b16bc5ff4aed471e10 Mon Sep 17 00:00:00 2001 From: Adolanium <94890352+Adolanium@users.noreply.github.com> Date: Fri, 21 Aug 2026 06:12:55 +0300 Subject: [PATCH 04/37] fix(cron): do not load MEMORY.md into scheduled jobs Cron already sets skip_memory=True and denylists the memory toolset. The default cron toolset still names memory, so init treated that as a request and built MemoryStore. MEMORY.md then landed in the job prompt. Treat a denylisted toolset as not requested, and strip memory from the cron enabled list. Flush agents that actually want the memory tool are unchanged (#65429). --- agent/agent_init.py | 18 ++++++---- cron/scheduler.py | 28 ++++++++++++--- tests/agent/test_skip_memory_store_65429.py | 38 ++++++++++++++++++++- tests/cron/test_agent_scheduling_gate.py | 2 +- tests/cron/test_scheduler.py | 35 ++++++++++++++----- 5 files changed, 99 insertions(+), 22 deletions(-) diff --git a/agent/agent_init.py b/agent/agent_init.py index f982d038d1..ca5996636b 100644 --- a/agent/agent_init.py +++ b/agent/agent_init.py @@ -1815,13 +1815,17 @@ def init_agent( agent._memory_nudge_interval = 10 agent._turns_since_memory = 0 agent._iters_since_skill = 0 - # A flush/background agent may pass skip_memory=True to avoid spinning up an - # external memory *provider*, but if the caller also explicitly enables the - # "memory" toolset it still needs the built-in file-backed store — otherwise - # the memory tool dispatches with store=None and every call fails (#65429). - # So the built-in store is created unless memory is globally disabled, while - # the external-provider block below stays gated on skip_memory. - _memory_toolset_requested = "memory" in (agent.enabled_toolsets or []) + # skip_memory=True skips the external memory *provider*. Flush/background + # agents can still pass enabled_toolsets=["memory"] so the built-in file + # store exists and the memory tool does not fail with store=None (#65429). + # A toolset on disabled_toolsets is not a request. Cron always denylists + # memory, but the default cron toolset still names it, so an enabled-only + # check would load MEMORY.md into an auto-approve job. + _enabled_toolsets = agent.enabled_toolsets or [] + _disabled_toolsets = agent.disabled_toolsets or [] + _memory_toolset_requested = ( + "memory" in _enabled_toolsets and "memory" not in _disabled_toolsets + ) if not skip_memory or _memory_toolset_requested: try: from tools.memory_tool import ( diff --git a/cron/scheduler.py b/cron/scheduler.py index 53704dbcf4..59ce8fafa8 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -361,8 +361,10 @@ def _resolve_cron_disabled_toolsets(cfg: dict) -> list[str]: Three toolsets are always disabled in cron context regardless of config: - ``messaging`` — interactive, needs a live gateway session - ``clarify`` — interactive, blocks waiting for user input - - ``memory`` — cron agents are constructed with ``skip_memory=True``, so - exposing this tool only gives the model an unbacked tool that fails + - ``memory`` — cron agents run with ``skip_memory=True``. The tool is + hidden, and ``memory`` is stripped from enabled_toolsets so the + built-in store is not created either (MEMORY.md would otherwise + land in the cron system prompt). ``cronjob`` is policy-denied by default (loop prevention, not a security boundary) and config-gated: setting ``cron.allow_agent_scheduling: true`` @@ -436,6 +438,10 @@ def _resolve_cron_enabled_toolsets(job: dict, cfg: dict) -> list[str] | None: 3. ``None`` on any lookup failure — AIAgent loads the full default set (legacy behavior before this change, preserved as the safety net). + ``memory`` is always stripped. Cron denylists that toolset and passes + ``skip_memory=True``. Leaving it in enabled_toolsets still constructs + the built-in MemoryStore and injects MEMORY.md into the job prompt. + _DEFAULT_OFF_TOOLSETS ({moa, homeassistant, rl}) are removed by ``_get_platform_tools`` for unconfigured platforms, so fresh installs get cron WITHOUT ``moa`` by default (issue reported by Norbert — @@ -443,10 +449,12 @@ def _resolve_cron_enabled_toolsets(job: dict, cfg: dict) -> list[str] | None: """ per_job = job.get("enabled_toolsets") if per_job: - return _merge_mcp_into_per_job_toolsets(list(per_job), cfg or {}) + return _strip_cron_memory_toolset( + _merge_mcp_into_per_job_toolsets(list(per_job), cfg or {}) + ) try: from hermes_cli.tools_config import _get_platform_tools # lazy: avoid heavy import at cron module load - return sorted(_get_platform_tools(cfg or {}, "cron")) + return _strip_cron_memory_toolset(sorted(_get_platform_tools(cfg or {}, "cron"))) except Exception as exc: logger.warning( "Cron toolset resolution failed, falling back to full default toolset: %s", @@ -455,6 +463,17 @@ def _resolve_cron_enabled_toolsets(job: dict, cfg: dict) -> list[str] | None: return None +def _strip_cron_memory_toolset(enabled: list[str] | None) -> list[str] | None: + """Drop ``memory`` from a cron enabled-toolset list. + + ``None`` means "full default set" and is left alone. skip_memory=True plus + the memory denylist still keep the store off on that fallback path. + """ + if enabled is None: + return None + return [name for name in enabled if name != "memory"] + + def _resolve_job_reasoning_config(job: dict, cfg: dict, model: str) -> dict | None: """Resolve the effective reasoning config for a cron run. @@ -496,6 +515,7 @@ def _resolve_job_reasoning_config(job: dict, cfg: dict, model: str) -> dict | No ) return resolve_reasoning_config(cfg if isinstance(cfg, dict) else {}, str(model)) + # Valid delivery platforms — used to validate user-supplied platform names # in cron delivery targets, preventing env var enumeration via crafted names. _KNOWN_DELIVERY_PLATFORMS = frozenset({ diff --git a/tests/agent/test_skip_memory_store_65429.py b/tests/agent/test_skip_memory_store_65429.py index f6966cd423..c84dbab0da 100644 --- a/tests/agent/test_skip_memory_store_65429.py +++ b/tests/agent/test_skip_memory_store_65429.py @@ -24,7 +24,9 @@ class _FakeOpenAI: pass -def _make_agent(monkeypatch, enabled_toolsets=None, skip_memory=True): +def _make_agent( + monkeypatch, enabled_toolsets=None, disabled_toolsets=None, skip_memory=True +): monkeypatch.setattr("run_agent.get_tool_definitions", lambda **kw: []) monkeypatch.setattr("run_agent.check_toolset_requirements", lambda: {}) monkeypatch.setattr("run_agent.OpenAI", _FakeOpenAI) @@ -38,6 +40,7 @@ def _make_agent(monkeypatch, enabled_toolsets=None, skip_memory=True): skip_context_files=True, skip_memory=skip_memory, enabled_toolsets=enabled_toolsets, + disabled_toolsets=disabled_toolsets, ) @@ -96,3 +99,36 @@ def test_skip_memory_memory_tool_handler_works_and_provider_skipped( memory_md = tmp_path / "hm" / "memories" / "MEMORY.md" assert memory_md.exists() assert "User prefers concise answers." in memory_md.read_text() + + +def test_skip_memory_disabled_toolset_does_not_load_store(monkeypatch, tmp_path): + """Cron shape: skip_memory=True, memory named in enabled AND disabled. + + #65429 must not load MEMORY.md just because the default cron toolset + still lists memory while the denylist hides the tool. + """ + home = tmp_path / "hm" + monkeypatch.setenv("HERMES_HOME", str(home)) + mem_dir = home / "memories" + mem_dir.mkdir(parents=True) + secret = "cron-should-never-see-this-memory" + (mem_dir / "MEMORY.md").write_text(secret + "\n") + (mem_dir / "USER.md").write_text("cron-should-never-see-this-profile\n") + + agent = _make_agent( + monkeypatch, + enabled_toolsets=["memory", "file"], + disabled_toolsets=["memory"], + skip_memory=True, + ) + assert agent._memory_store is None + assert agent._memory_manager is None + assert agent._memory_enabled is False + assert agent._user_profile_enabled is False + + from agent.system_prompt import build_system_prompt_parts + + parts = build_system_prompt_parts(agent) + blob = " ".join(str(v) for v in parts.values()) + assert secret not in blob + assert "cron-should-never-see-this-profile" not in blob diff --git a/tests/cron/test_agent_scheduling_gate.py b/tests/cron/test_agent_scheduling_gate.py index 6e80dc6823..abbe6a9fb4 100644 --- a/tests/cron/test_agent_scheduling_gate.py +++ b/tests/cron/test_agent_scheduling_gate.py @@ -21,7 +21,7 @@ from cron.scheduler import _resolve_cron_disabled_toolsets # The toolsets that must be denied in cron context no matter what the # agent-scheduling gate says: messaging/clarify are interactive-only, -# memory is unbacked in cron runs (skip_memory=True). +# memory stays off in cron runs (skip_memory=True, toolset denylisted). ALWAYS_DISABLED = ["messaging", "clarify", "memory"] diff --git a/tests/cron/test_scheduler.py b/tests/cron/test_scheduler.py index 00db344c5e..4bfe7b27c0 100644 --- a/tests/cron/test_scheduler.py +++ b/tests/cron/test_scheduler.py @@ -118,6 +118,24 @@ class TestPerJobToolsetMcpMerge: assert m_platform.call_args[0][1] == "cron" assert set(result) == set(sentinel) + def test_resolver_strips_memory_from_per_job_list(self): + result = _resolve_cron_enabled_toolsets( + {"enabled_toolsets": ["memory", "file"]}, + {"mcp_servers": {}}, + ) + assert "memory" not in result + assert "file" in result + + def test_resolver_strips_memory_from_platform_fallback(self): + job = {"enabled_toolsets": None} + with patch( + "hermes_cli.tools_config._get_platform_tools", + return_value={"web", "memory", "file"}, + ): + result = _resolve_cron_enabled_toolsets(job, {}) + assert result == ["file", "web"] + assert "memory" not in result + class TestResolveOrigin: def test_full_origin(self): @@ -613,10 +631,9 @@ class TestRunJobSessionPersistence: def test_run_job_memory_toolset_disabled_in_cron(self, tmp_path): """memory toolset must be disabled in cron sessions — issue #38129. - Cron agents are constructed with skip_memory=True, so the memory - backend is not initialised. Exposing the memory tool only gives the - model an unbacked tool that fails at runtime with - "Memory is not available." Hiding it from the schema prevents that. + Cron agents are constructed with skip_memory=True. The memory tool + stays off the schema, and memory is stripped from enabled_toolsets + so MEMORY.md is not loaded into the job prompt. """ job = { "id": "memory-hide-job", @@ -634,10 +651,9 @@ class TestRunJobSessionPersistence: def test_run_job_disables_memory_even_when_per_job_enables_it(self, tmp_path): """Cron runs pass skip_memory=True, so memory must not be exposed. - A cron job can request the memory tool through enabled_toolsets, but - there is no MemoryStore injected for cron agents. Keep memory in the - disabled set so AIAgent filters the unbacked tool out before the model - can call it and receive "Memory is not available" failures. + A cron job can name the memory toolset in enabled_toolsets. The + resolver drops it, and the denylist still lists it, so the model + never gets the tool and init never builds MemoryStore. """ job = { "id": "memory-toolset-job", @@ -650,7 +666,8 @@ class TestRunJobSessionPersistence: kwargs = mock_agent_cls.call_args.kwargs assert kwargs["skip_memory"] is True - assert kwargs["enabled_toolsets"] == ["memory", "file"] + assert kwargs["enabled_toolsets"] == ["file"] + assert "memory" not in (kwargs["enabled_toolsets"] or []) assert "memory" in kwargs["disabled_toolsets"] def test_tick_skips_due_jobs_while_dispatch_is_paused(self, tmp_path): From 54227416ce1ad0de8b53c85dd994e1b4d0941074 Mon Sep 17 00:00:00 2001 From: vinsew Date: Fri, 21 Aug 2026 13:20:38 +0800 Subject: [PATCH 05/37] fix(opencode): send Ox Alpha reasoning effort through Zen OpenCode documents x-preview-f-free as accepting low, high, and max reasoning effort on its Zen Chat Completions endpoint. Hermes previously resolved the user's per-model override to max but the plain Zen provider profile discarded it, so successful calls silently ran at the server default. Introduce an OpenCodeZenProfile scoped only to x-preview-f-free. It forwards the normalized top-level reasoning_effort, maps xhigh to max, preserves server defaults when unset or disabled, and leaves every other Zen model untouched. Add profile and full transport tests that prove max reaches the outgoing request and that non-target models are unaffected. Also correct the nanoid security-pin comment to match the already-locked 3.3.18 release. --- .../model-providers/opencode-zen/__init__.py | 28 +++++++++- .../test_opencode_go_profile.py | 56 +++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/plugins/model-providers/opencode-zen/__init__.py b/plugins/model-providers/opencode-zen/__init__.py index 456be0aa42..e830ed3762 100644 --- a/plugins/model-providers/opencode-zen/__init__.py +++ b/plugins/model-providers/opencode-zen/__init__.py @@ -158,7 +158,33 @@ class OpenCodeGoProfile(ProviderProfile): return extra_body, top_level -opencode_zen = ProviderProfile( +class OpenCodeZenProfile(ProviderProfile): + """OpenCode Zen - model-specific reasoning controls.""" + + def build_api_kwargs_extras( + self, *, reasoning_config: dict | None = None, model: str | None = None, **context + ) -> tuple[dict[str, Any], dict[str, Any]]: + if _flat_model_name(model) != "x-preview-f-free": + return {}, {} + if not isinstance(reasoning_config, dict): + return {}, {} + if reasoning_config.get("enabled") is False: + return {}, {} + + effort = (reasoning_config.get("effort") or "").strip().lower() + if not effort or effort == "none": + return {}, {} + + from agent.reasoning_effort import clamp_effort + + supported_efforts = ("low", "high", "max") + clamped = clamp_effort(effort, supported_efforts, {"xhigh": "max"}) + if clamped not in supported_efforts: + return {}, {} + return {}, {"reasoning_effort": clamped} + + +opencode_zen = OpenCodeZenProfile( name="opencode-zen", aliases=("opencode", "opencode_zen", "zen"), env_vars=("OPENCODE_ZEN_API_KEY",), diff --git a/tests/plugins/model_providers/test_opencode_go_profile.py b/tests/plugins/model_providers/test_opencode_go_profile.py index 5b24821cf9..8524ba1978 100644 --- a/tests/plugins/model_providers/test_opencode_go_profile.py +++ b/tests/plugins/model_providers/test_opencode_go_profile.py @@ -16,6 +16,62 @@ def opencode_go_profile(): return profile +@pytest.fixture +def opencode_zen_profile(): + """Resolve the registered OpenCode Zen provider profile.""" + import model_tools # noqa: F401 + import providers + + profile = providers.get_provider_profile("opencode-zen") + assert profile is not None, "opencode-zen provider profile must be registered" + return profile + + +class TestOpenCodeZenOxReasoning: + """Ox Alpha Free uses OpenCode Zen's native reasoning_effort control.""" + + def test_max_effort_is_emitted(self, opencode_zen_profile): + extra_body, top_level = opencode_zen_profile.build_api_kwargs_extras( + reasoning_config={"enabled": True, "effort": "max"}, + model="x-preview-f-free", + ) + assert extra_body == {} + assert top_level == {"reasoning_effort": "max"} + + @pytest.mark.parametrize("reasoning_config", [None, {"enabled": False}]) + def test_unset_or_disabled_preserves_server_default( + self, opencode_zen_profile, reasoning_config + ): + extra_body, top_level = opencode_zen_profile.build_api_kwargs_extras( + reasoning_config=reasoning_config, + model="x-preview-f-free", + ) + assert extra_body == {} + assert top_level == {} + + def test_other_zen_models_are_untouched(self, opencode_zen_profile): + extra_body, top_level = opencode_zen_profile.build_api_kwargs_extras( + reasoning_config={"enabled": True, "effort": "max"}, + model="gemini-3-flash", + ) + assert extra_body == {} + assert top_level == {} + + def test_max_reaches_chat_completions_request(self, opencode_zen_profile): + from agent.transports.chat_completions import ChatCompletionsTransport + + kwargs = ChatCompletionsTransport().build_kwargs( + model="x-preview-f-free", + messages=[{"role": "user", "content": "ping"}], + tools=None, + provider_profile=opencode_zen_profile, + reasoning_config={"enabled": True, "effort": "max"}, + base_url="https://opencode.ai/zen/v1", + ) + assert "extra_body" not in kwargs + assert kwargs["reasoning_effort"] == "max" + + class TestOpenCodeGoKimiReasoning: """Kimi K2 models use Moonshot's thinking + reasoning_effort shape on OpenCode Go.""" From d4d04098a5f9afb526f0995c1eb08648d92095a7 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 21 Aug 2026 00:34:19 -0700 Subject: [PATCH 06/37] =?UTF-8?q?fix:=20Ox=20Alpha=20reasoning=20effort=20?= =?UTF-8?q?reaches=20the=20wire=20clamped=20=E2=80=94=20shared=20across=20?= =?UTF-8?q?zen=20and=20free=20providers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Widens the salvaged #91323 fix (@vinsew): the effort vocabulary moves to agent.reasoning_effort (OX_ALPHA_EFFORTS/OVERRIDES, the declared-policy home every other model vocabulary lives in), and the translation is shared between the opencode-zen profile and the keyless opencode-free profile — Ox Alpha is reachable through both, and the free profile previously dropped effort entirely. Live-verified: medium clamps to low (raw medium 400s: 'This model always engages in thinking... use low, high, or max'), xhigh rounds to max, and full agent turns with effort=medium complete on BOTH providers. --- agent/reasoning_effort.py | 7 +++ .../model-providers/opencode-free/__init__.py | 30 ++++++++++- .../model-providers/opencode-zen/__init__.py | 51 ++++++++++++------- .../test_opencode_go_profile.py | 34 +++++++++++++ 4 files changed, 103 insertions(+), 19 deletions(-) diff --git a/agent/reasoning_effort.py b/agent/reasoning_effort.py index 48b44a25ee..e29c0273e5 100644 --- a/agent/reasoning_effort.py +++ b/agent/reasoning_effort.py @@ -95,6 +95,13 @@ KIMI_K3_EFFORTS: tuple[str, ...] = ("low", "high", "max") #: Moonshot/Kimi K2-era models: low/medium/high. KIMI_K2_EFFORTS: tuple[str, ...] = ("low", "medium", "high") +#: OpenCode "Ox Alpha" stealth model (x-preview-f-free): thinking is always +#: on and the wire accepts exactly low/high/max — medium/none/xhigh 400 with +#: "This model always engages in thinking and cannot be disabled; please use +#: low, high, or max" (verified live 2026-08-21). xhigh rounds up to max. +OX_ALPHA_EFFORTS: tuple[str, ...] = ("low", "high", "max") +OX_ALPHA_OVERRIDES: dict[str, str] = {"xhigh": "max"} + #: Tencent TokenHub: low/medium/high. TOKENHUB_EFFORTS: tuple[str, ...] = ("low", "medium", "high") diff --git a/plugins/model-providers/opencode-free/__init__.py b/plugins/model-providers/opencode-free/__init__.py index 8d8c1a67ef..8ef88dcdf1 100644 --- a/plugins/model-providers/opencode-free/__init__.py +++ b/plugins/model-providers/opencode-free/__init__.py @@ -9,6 +9,8 @@ hermes_cli.models.opencode_zen_free_runtime). No OpenCode account needed. Select via ``hermes model`` or ``/model free``. """ +from typing import Any + from hermes_cli import __version__ as _HERMES_VERSION from providers import register_provider from providers.base import ProviderProfile @@ -23,7 +25,33 @@ _KEYLESS_HEADERS = { "User-Agent": f"HermesAgent/{_HERMES_VERSION}", } -opencode_free = ProviderProfile( + +class OpenCodeFreeProfile(ProviderProfile): + """OpenCode Free — keyless, with Ox Alpha reasoning controls. + + Ox Alpha (x-preview-f-free) is reachable through this provider as well + as opencode-zen; both share the same wire contract (reasoning_effort + accepts exactly low/high/max — anything else 400s). The translation + lives in the zen plugin; resolve it through the registered zen profile's + module so the two providers can never drift. + """ + + def build_api_kwargs_extras( + self, *, reasoning_config: dict | None = None, model: str | None = None, **context + ) -> tuple[dict[str, Any], dict[str, Any]]: + try: + import sys + + from providers import get_provider_profile + + zen_profile = get_provider_profile("opencode-zen") + zen_module = sys.modules[type(zen_profile).__module__] + return zen_module._build_ox_alpha_reasoning_extras(reasoning_config, model) + except Exception: + return {}, {} + + +opencode_free = OpenCodeFreeProfile( name="opencode-free", aliases=("free", "opencode_free"), env_vars=(), # keyless — nothing to configure diff --git a/plugins/model-providers/opencode-zen/__init__.py b/plugins/model-providers/opencode-zen/__init__.py index e830ed3762..2d55cae33b 100644 --- a/plugins/model-providers/opencode-zen/__init__.py +++ b/plugins/model-providers/opencode-zen/__init__.py @@ -158,30 +158,45 @@ class OpenCodeGoProfile(ProviderProfile): return extra_body, top_level +def _build_ox_alpha_reasoning_extras( + reasoning_config: dict | None, model: str | None +) -> tuple[dict[str, Any], dict[str, Any]]: + """Shared Ox Alpha (x-preview-f-free) reasoning_effort translation. + + Used by both the opencode-zen profile and the opencode-free keyless + profile — the model is reachable through either provider and the wire + contract is identical (low/high/max only; anything else 400s). + """ + if _flat_model_name(model) != "x-preview-f-free": + return {}, {} + if not isinstance(reasoning_config, dict): + return {}, {} + if reasoning_config.get("enabled") is False: + return {}, {} + + effort = (reasoning_config.get("effort") or "").strip().lower() + if not effort or effort == "none": + return {}, {} + + from agent.reasoning_effort import ( + OX_ALPHA_EFFORTS, + OX_ALPHA_OVERRIDES, + clamp_effort, + ) + + clamped = clamp_effort(effort, OX_ALPHA_EFFORTS, OX_ALPHA_OVERRIDES) + if clamped not in OX_ALPHA_EFFORTS: + return {}, {} + return {}, {"reasoning_effort": clamped} + + class OpenCodeZenProfile(ProviderProfile): """OpenCode Zen - model-specific reasoning controls.""" def build_api_kwargs_extras( self, *, reasoning_config: dict | None = None, model: str | None = None, **context ) -> tuple[dict[str, Any], dict[str, Any]]: - if _flat_model_name(model) != "x-preview-f-free": - return {}, {} - if not isinstance(reasoning_config, dict): - return {}, {} - if reasoning_config.get("enabled") is False: - return {}, {} - - effort = (reasoning_config.get("effort") or "").strip().lower() - if not effort or effort == "none": - return {}, {} - - from agent.reasoning_effort import clamp_effort - - supported_efforts = ("low", "high", "max") - clamped = clamp_effort(effort, supported_efforts, {"xhigh": "max"}) - if clamped not in supported_efforts: - return {}, {} - return {}, {"reasoning_effort": clamped} + return _build_ox_alpha_reasoning_extras(reasoning_config, model) opencode_zen = OpenCodeZenProfile( diff --git a/tests/plugins/model_providers/test_opencode_go_profile.py b/tests/plugins/model_providers/test_opencode_go_profile.py index 8524ba1978..54fae9b15f 100644 --- a/tests/plugins/model_providers/test_opencode_go_profile.py +++ b/tests/plugins/model_providers/test_opencode_go_profile.py @@ -71,6 +71,40 @@ class TestOpenCodeZenOxReasoning: assert "extra_body" not in kwargs assert kwargs["reasoning_effort"] == "max" + def test_unsupported_efforts_clamp_to_wire_vocabulary(self, opencode_zen_profile): + """medium/xhigh are not on Ox Alpha's wire (400 raw); they must clamp + to the nearest supported level, never pass through.""" + for requested, expected in (("medium", "low"), ("xhigh", "max")): + _, top_level = opencode_zen_profile.build_api_kwargs_extras( + reasoning_config={"enabled": True, "effort": requested}, + model="x-preview-f-free", + ) + assert top_level == {"reasoning_effort": expected}, requested + + def test_opencode_free_profile_shares_the_translation(self): + """Ox Alpha is reachable via the keyless opencode-free provider too; + its profile must emit the identical clamped reasoning_effort.""" + import model_tools # noqa: F401 + import providers + from providers.base import ProviderProfile + + profile = providers.get_provider_profile("opencode-free") + assert profile is not None + assert ( + type(profile).build_api_kwargs_extras + is not ProviderProfile.build_api_kwargs_extras + ), "opencode-free must override build_api_kwargs_extras (aux gate)" + _, top_level = profile.build_api_kwargs_extras( + reasoning_config={"enabled": True, "effort": "medium"}, + model="x-preview-f-free", + ) + assert top_level == {"reasoning_effort": "low"} + _, other = profile.build_api_kwargs_extras( + reasoning_config={"enabled": True, "effort": "max"}, + model="big-pickle", + ) + assert other == {} + class TestOpenCodeGoKimiReasoning: """Kimi K2 models use Moonshot's thinking + reasoning_effort shape on OpenCode Go.""" From 7d0f06f75d0c91dde7f798e2b96c22e78ccc67ef Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 21 Aug 2026 00:34:28 -0700 Subject: [PATCH 07/37] chore: map contributor email for vinsew --- contributors/emails/yiyangchaishu@gmail.com | 1 + 1 file changed, 1 insertion(+) create mode 100644 contributors/emails/yiyangchaishu@gmail.com diff --git a/contributors/emails/yiyangchaishu@gmail.com b/contributors/emails/yiyangchaishu@gmail.com new file mode 100644 index 0000000000..87f7b960c6 --- /dev/null +++ b/contributors/emails/yiyangchaishu@gmail.com @@ -0,0 +1 @@ +vinsew From 443d4387b514655a8b090825079f0966d11e2511 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 21 Aug 2026 00:43:17 -0700 Subject: [PATCH 08/37] perf(desktop): idle renderers stop burning CPU on infinite CSS animations (#53902, #73082) The arc-border running indicator animated background-position (repaint every frame: ~3,600 main-thread style recalcs/min per arc, ~4,100ms/min of renderer task time measured over 60s of true idle) and progress-slide animated left (forced layout every frame). Both now travel via transform on the compositor: same visuals, 3,617 -> 79 style recalcs/min and 4,108 -> 153 ms/min task time in the same harness (-96%). --- apps/desktop/src/styles.css | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/apps/desktop/src/styles.css b/apps/desktop/src/styles.css index a65d9168cb..041c9a69c0 100644 --- a/apps/desktop/src/styles.css +++ b/apps/desktop/src/styles.css @@ -982,11 +982,19 @@ } @keyframes arc-border { + /* Compositor-only travel. The ::before layer is 300% × 300% of the host + (mirroring the old `background-size: 300%`), so translating it from + -10% → -50% of its own size reproduces the old + `background-position: 15% → 75%` exactly (offset = -2·W·p), while + `transform` animates on the compositor with zero per-frame style + recalc/paint. The old background-position version cost ~3,600 + main-thread style recalcs per minute per arc, pinning idle renderers + (#53902, #73082). */ 0% { - background-position: 15% 15%; + transform: translate(-10%, -10%); } 100% { - background-position: 75% 75%; + transform: translate(-50%, -50%); } } @@ -1067,8 +1075,14 @@ .arc-border::before { content: ''; position: absolute; - inset: 0; - border-radius: inherit; + top: 0; + left: 0; + /* The gradient layer IS 300% of the ring (the old version kept a host-sized + ::before and slid a 300%-sized background through it). Sizing the layer + itself lets the travel be a `transform` — composited, no main-thread + style/paint work — while the host's mask + overflow clip it to the ring. */ + width: 300%; + height: 300%; background: linear-gradient( var(--arc-angle), transparent 0%, @@ -1085,7 +1099,7 @@ var(--arc-c0) 95%, var(--arc-c1) 100% ); - background-size: 300% 300%; + will-change: transform; animation: arc-border var(--arc-duration) linear infinite; } @@ -2489,15 +2503,21 @@ button[data-slot='aui_msg-reactions'] svg { flow). Color/positioning come from the primitive; this owns the animation. */ .progress-slide { width: 40%; + /* left is pinned; travel is compositor-only (see arc-border note). */ + left: 0; + will-change: transform; animation: progress-slide 1.15s ease-in-out infinite; } @keyframes progress-slide { + /* Old version animated `left: -42% → 100%`, forcing main-thread layout + every frame for the whole hatch flow. The block is 40% of its track, so + the same travel in units of its own width is -105% → 250%. */ 0% { - left: -42%; + transform: translateX(-105%); } 100% { - left: 100%; + transform: translateX(250%); } } From 37da0d4d50fe9b34841f596d285861d34b831e9a Mon Sep 17 00:00:00 2001 From: qixuancao Date: Wed, 12 Aug 2026 18:10:05 +0800 Subject: [PATCH 09/37] fix(agent): synchronize background review cancellation --- agent/agent_init.py | 12 +- agent/background_review.py | 238 +++++++++-- agent/conversation_loop.py | 25 -- run_agent.py | 71 +++- tests/run_agent/test_background_review.py | 484 +++++++++++++++++++--- 5 files changed, 700 insertions(+), 130 deletions(-) diff --git a/agent/agent_init.py b/agent/agent_init.py index ca5996636b..88985abbdc 100644 --- a/agent/agent_init.py +++ b/agent/agent_init.py @@ -909,14 +909,12 @@ def init_agent( agent._active_children = [] # Running child AIAgents (for interrupt propagation) agent._active_children_lock = threading.Lock() - # Background memory/skill review state (agent/background_review.py). Holds - # the forked review AIAgent while its run_conversation() is in flight, so - # the NEXT live turn can proactively interrupt a still-running review - # instead of letting the two race concurrently against the same - # session_id/credentials (observed as doubled prompt-token counts and a - # Ctrl+C-proof lockup when a live turn started before a review fired at - # the end of the prior turn had finished). + # Background memory/skill review state (agent/background_review.py). + # ``_background_review_run`` is installed before the worker starts and + # fences its first provider-capable phase; the direct agent pointer keeps + # normal interrupt propagation available once the fork is constructed. agent._background_review_agent = None + agent._background_review_run = None agent._background_review_lock = threading.Lock() # Store OpenRouter provider preferences diff --git a/agent/background_review.py b/agent/background_review.py index ae12a059d8..d643b95e12 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -23,6 +23,7 @@ import json import logging import os from pathlib import Path +import threading from typing import Any, Dict, List, Optional from agent.thread_scoped_output import thread_scoped_silence @@ -30,6 +31,156 @@ from agent.thread_scoped_output import thread_scoped_silence logger = logging.getLogger(__name__) +_BACKGROUND_REVIEW_CANCEL_TIMEOUT_SECONDS = 2.0 + + +class _BackgroundReviewRun: + """Per-review cancellation and request-completion handshake.""" + + def __init__(self) -> None: + self.cancel_requested = threading.Event() + self.request_done = threading.Event() + self._lock = threading.Lock() + self._review_agent = None + self._request_started = False + self._request_finished = False + self._cancel_dispatched = False + + def begin_request(self, review_agent: Any) -> bool: + """Atomically admit the first provider-capable review phase.""" + with self._lock: + self._review_agent = review_agent + if self.cancel_requested.is_set() or self._request_finished: + return False + self._request_started = True + return True + + def cancel(self) -> Any: + """Fence startup and return the running fork, if one was admitted.""" + with self._lock: + self.cancel_requested.set() + if ( + self._request_started + and not self._request_finished + and not self._cancel_dispatched + ): + self._cancel_dispatched = True + return self._review_agent + return None + + def mark_request_finished(self) -> bool: + """Latch request completion once; the caller publishes the event.""" + with self._lock: + if self._request_finished: + return False + self._request_finished = True + self._request_started = False + self._review_agent = None + return True + + +def prepare_background_review_run(agent: Any) -> Optional[_BackgroundReviewRun]: + """Install a unique run token on the parent before ``Thread.start()``.""" + lock = getattr(agent, "_background_review_lock", None) + if lock is None: + try: + lock = threading.Lock() + agent._background_review_lock = lock + except (AttributeError, TypeError): + return None + + run = _BackgroundReviewRun() + try: + with lock: + current = getattr(agent, "_background_review_run", None) + if current is not None and not current.request_done.is_set(): + return None + agent._background_review_run = run + except (AttributeError, TypeError): + return None + return run + + +def finish_background_review_run( + agent: Any, + run: Optional[_BackgroundReviewRun], +) -> None: + """Publish one run's request exit without clearing a successor (ABA-safe).""" + if run is None or not run.mark_request_finished(): + return + + lock = getattr(agent, "_background_review_lock", None) + if lock is not None: + with lock: + if getattr(agent, "_background_review_run", None) is run: + agent._background_review_run = None + elif getattr(agent, "_background_review_run", None) is run: + agent._background_review_run = None + run.request_done.set() + + +def _interrupt_background_review(review_agent: Any) -> None: + """Request abort off-thread so a broken abort hook cannot stall foreground.""" + + def _interrupt() -> None: + try: + review_agent.interrupt("superseded by a new live turn") + except Exception: + logger.debug( + "Failed to cancel in-flight background review for a new turn", + exc_info=True, + ) + + try: + threading.Thread( + target=_interrupt, + daemon=True, + name="bg-review-cancel", + ).start() + except Exception: + logger.debug( + "Failed to start background-review cancellation thread", + exc_info=True, + ) + + +def cancel_background_review_for_live_turn(agent: Any) -> bool: + """Cancel the current review and await its request-phase acknowledgement. + + Returns ``False`` when acknowledgement is unavailable or misses the bounded + deadline. Callers must then stop before same-session turn-context work. + """ + lock = getattr(agent, "_background_review_lock", None) + if lock is not None: + with lock: + run = getattr(agent, "_background_review_run", None) + legacy_agent = getattr(agent, "_background_review_agent", None) + else: + run = getattr(agent, "_background_review_run", None) + legacy_agent = getattr(agent, "_background_review_agent", None) + + if run is None: + if legacy_agent is None: + return True + _interrupt_background_review(legacy_agent) + return False + + review_agent = run.cancel() + if review_agent is not None: + _interrupt_background_review(review_agent) + + acknowledged = run.request_done.wait( + timeout=_BACKGROUND_REVIEW_CANCEL_TIMEOUT_SECONDS + ) + if not acknowledged: + logger.error( + "Background review did not acknowledge cancellation within %.1fs; " + "refusing to start overlapping live turn", + _BACKGROUND_REVIEW_CANCEL_TIMEOUT_SECONDS, + ) + return acknowledged + + # --------------------------------------------------------------------------- # Background-review aux-model selector + routed digest. # @@ -862,13 +1013,23 @@ def _run_review_in_thread( messages_snapshot: List[Dict], prompt: str, task_cfg: Optional[Dict[str, Any]] = None, + review_run: Optional[_BackgroundReviewRun] = None, ) -> None: """Worker function executed in the background-review daemon thread. Spawns a forked ``AIAgent`` inheriting the parent's runtime, runs the review prompt, and surfaces a compact action summary back to the user via ``agent._safe_print`` and ``agent.background_review_callback``. + + ``review_run`` is the per-review cancellation token from + :func:`prepare_background_review_run`. If a live turn bumps the + cancel generation before this review reaches its first provider call, + the review aborts without entering ``run_conversation()`` (#84423). """ + if review_run is not None and review_run.cancel_requested.is_set(): + finish_background_review_run(agent, review_run) + return + # Local import to avoid a hard circular dep at module load. from run_agent import AIAgent from tools.terminal_tool import set_approval_callback as _set_approval_callback @@ -918,6 +1079,10 @@ def _run_review_in_thread( except (ValueError, AttributeError): pass + def _finish_request_phase(agent_ref) -> None: + _unregister_review_agent(agent_ref) + finish_background_review_run(agent, review_run) + try: # Silence stdout/stderr for THIS worker thread only. A process-global # ``contextlib.redirect_stdout(devnull)`` here would also blank @@ -1117,12 +1282,10 @@ def _run_review_in_thread( # Register this fork on the PARENT's _active_children (the same # list interrupt() fans out to for subagent delegation) and # _background_review_agent (a direct pointer the next live turn - # uses to proactively cancel a still-running review). Without - # this, a review still streaming when the next turn starts races - # the live turn against the same session_id/credentials — producing - # doubled prompt-token accounting and a Ctrl+C-proof lockup. - # Best-effort: agents built without agent_init.py (test stubs) - # degrade to "no cross-cancellation" rather than aborting the review. + # uses to interrupt an admitted request). The per-review run token + # separately fences startup and acknowledges request-phase exit. + # The legacy pointer/list remain best-effort for direct test stubs; + # a prepared run token is the live-turn cancellation authority. if hasattr(agent, "_background_review_agent"): _br_lock = getattr(agent, "_background_review_lock", None) if _br_lock is not None: @@ -1173,22 +1336,26 @@ def _run_review_in_thread( pass try: - # Routed to a different model -> replay a digest (cache is cold - # on that model anyway, so minimise cold-written tokens). Same - # model -> replay the full snapshot (warm cache reads). - _review_history = ( - _digest_history(messages_snapshot) if _routed - else messages_snapshot - ) - review_agent.run_conversation( - user_message=( - prompt - + "\n\nYou can only call memory and skill " - "management tools. Other tools will be denied " - "at runtime — do not attempt them." - ), - conversation_history=_review_history, + request_admitted = ( + review_run is None or review_run.begin_request(review_agent) ) + if request_admitted: + # Routed to a different model -> replay a digest (cache is cold + # on that model anyway, so minimise cold-written tokens). Same + # model -> replay the full snapshot (warm cache reads). + _review_history = ( + _digest_history(messages_snapshot) if _routed + else messages_snapshot + ) + review_agent.run_conversation( + user_message=( + prompt + + "\n\nYou can only call memory and skill " + "management tools. Other tools will be denied " + "at runtime — do not attempt them." + ), + conversation_history=_review_history, + ) finally: clear_thread_tool_whitelist() # Attribute the review fork's usage to the PARENT session. @@ -1199,12 +1366,9 @@ def _run_review_in_thread( if review_agent is not None: review_usage.update(_snapshot_review_usage(review_agent)) _record_review_usage_to_parent(agent, review_usage) - # Unregister as soon as run_conversation() itself has - # returned — that's the only phase making outbound API - # calls, i.e. the only phase that can race the parent's - # next live turn. Runs on both the success and exception - # path (this whole block is inside the try/finally above). - _unregister_review_agent(review_agent) + # Publish completion as soon as the provider-capable phase has + # returned or startup cancellation has fenced it out. + _finish_request_phase(review_agent) # Snapshot review actions before teardown. close() is allowed to # clean per-session state, but the user-visible self-improvement @@ -1284,13 +1448,10 @@ def _run_review_in_thread( # thread-scoped silence here so teardown output (Honcho flush, Hindsight # sync, background thread joins) stays quiet even on the exception path, # without blanking other threads' streams. - # Also a safety-net unregister: covers exceptions raised during setup - # (between registration and the run_conversation try/finally above) - # that the primary _unregister_review_agent call site never reaches. - # _unregister_review_agent is idempotent (checks `is`/`in` membership), - # so calling it again here after the primary call site already ran is - # a harmless no-op. - _unregister_review_agent(review_agent) + # Also a safety-net completion: covers exceptions raised during setup + # before the request-phase finally. Both tracking cleanup and the + # per-run completion publication are identity-scoped and idempotent. + _finish_request_phase(review_agent) if review_agent is not None: try: with thread_scoped_silence(): @@ -1319,6 +1480,7 @@ def spawn_background_review_thread( review_skills: bool = False, focus: Optional[str] = None, task_cfg: Optional[Dict[str, Any]] = None, + review_run: Optional[_BackgroundReviewRun] = None, ): """Build the review thread target and prompt for a background review. @@ -1359,7 +1521,13 @@ def spawn_background_review_thread( ) def _target() -> None: - _run_review_in_thread(agent, messages_snapshot, prompt, task_cfg) + _run_review_in_thread( + agent, + messages_snapshot, + prompt, + task_cfg=task_cfg, + review_run=review_run, + ) return _target, prompt diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 951b874701..3d7f07ef39 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -1820,31 +1820,6 @@ def run_conversation( agent._last_compression_attempt_recorded = False agent._last_compression_attempt_in_place = None - # If a background memory/skill review spawned at the end of a PRIOR turn - # (agent/background_review.py) is still running its own run_conversation() - # when THIS turn starts, cancel it now rather than letting both make - # outbound API calls concurrently against the same session_id/credentials. - # That concurrency can produce doubled prompt-token accounting on this - # turn's own calls and, because the review fork is a fully separate - # AIAgent with no route back to THIS agent's interrupt() by default, a - # lockup that survives a normal /stop and needs a hard Ctrl+C. - # ``review_agent.interrupt()`` is fire-and-forget here — it just flags - # cancellation and aborts the review's in-flight socket; it does not - # block waiting for the review's daemon thread to exit, so it can't add - # latency to this turn. Only ever set on the real owning agent (the - # review fork's own copy of this attribute stays None — reviews don't - # spawn nested reviews), so this is a no-op on every other run_conversation - # caller (subagents, the review fork itself, etc). - _pending_review = getattr(agent, "_background_review_agent", None) - if _pending_review is not None: - try: - _pending_review.interrupt("superseded by a new live turn") - except Exception: - logger.debug( - "Failed to cancel in-flight background review for a new turn", - exc_info=True, - ) - # Adopt any ~/.hermes/.env credential/base-url edits made since the last # turn — a Settings save updates .env but not this worker's client, which # was built at agent init (#67821). No-op when .env is unchanged. diff --git a/run_agent.py b/run_agent.py index f5c4de1274..107c0f33ab 100644 --- a/run_agent.py +++ b/run_agent.py @@ -1896,22 +1896,37 @@ class AIAgent: enabled, task_cfg = load_background_review_settings() if not enabled: return - from agent.background_review import spawn_background_review_thread + from agent.background_review import ( + finish_background_review_run, + prepare_background_review_run, + spawn_background_review_thread, + ) from tools.thread_context import propagate_context_to_thread - target, _prompt = spawn_background_review_thread( - self, - messages_snapshot, - review_memory=review_memory, - review_skills=review_skills, - focus=focus, - task_cfg=task_cfg, - ) - # Carry the active profile into the review thread so MEMORY.md / skill - # review writes land in the right profile (#54937). - t = threading.Thread( - target=propagate_context_to_thread(target), daemon=True, name="bg-review" - ) - t.start() + + review_run = prepare_background_review_run(self) + if review_run is None: + return + try: + target, _prompt = spawn_background_review_thread( + self, + messages_snapshot, + review_memory=review_memory, + review_skills=review_skills, + focus=focus, + task_cfg=task_cfg, + review_run=review_run, + ) + # Carry the active profile into the review thread so MEMORY.md / + # skill review writes land in the right profile (#54937). + t = threading.Thread( + target=propagate_context_to_thread(target), + daemon=True, + name="bg-review", + ) + t.start() + except Exception: + finish_background_review_run(self, review_run) + raise def _build_memory_write_metadata( self, @@ -8478,6 +8493,32 @@ class AIAgent: moa_config: Optional[dict[str, Any]] = None, ) -> Dict[str, Any]: """Forwarder — see ``agent.conversation_loop.run_conversation``.""" + # A review deliberately shares this agent's session_id for prompt-cache + # parity. Fence review startup or interrupt an admitted request, then + # await that request's exit before opening any live-turn Relay or task + # instrumentation for the same session. + from agent.background_review import cancel_background_review_for_live_turn + + if not cancel_background_review_for_live_turn(self): + failure = ( + "Live turn not started: the prior background review did not " + "acknowledge cancellation before the bounded safety deadline. " + "Retry after the review has stopped." + ) + existing_messages = conversation_history + if existing_messages is None: + existing_messages = getattr(self, "_session_messages", None) + return { + "final_response": failure, + "messages": list(existing_messages or []), + "completed": False, + "api_calls": 0, + "error": failure, + "failed": True, + "retryable": True, + "background_review_cancellation_timeout": True, + } + from agent.aux_accounting import ( reset_accounting_context, set_accounting_context, diff --git a/tests/run_agent/test_background_review.py b/tests/run_agent/test_background_review.py index 84f11a5c28..19aa95f395 100644 --- a/tests/run_agent/test_background_review.py +++ b/tests/run_agent/test_background_review.py @@ -2,10 +2,67 @@ from __future__ import annotations +import threading + import run_agent as run_agent_module from run_agent import AIAgent +_REAL_THREAD = threading.Thread + + +class _TurnBoundaryReached(Exception): + """Stop a live turn exactly when it reaches turn-context construction.""" + + +class CapturingThread: + targets = [] + + def __init__(self, *, target, daemon=None, name=None): + self._target = target + self.targets.append(target) + + def start(self): + pass + + +class ObservedEvent: + """A real Event that also exposes when a waiter starts waiting.""" + + def __init__(self): + self._event = threading.Event() + self.wait_started = threading.Event() + self.set_calls = 0 + + def set(self): + self.set_calls += 1 + self._event.set() + + def wait(self, timeout=None): + self.wait_started.set() + return self._event.wait(timeout) + + def is_set(self): + return self._event.is_set() + + +class FakeReviewAgent: + def __init__(self, **kwargs): + self._session_messages = [] + + def run_conversation(self, **kwargs): + pass + + def interrupt(self, message=None): + pass + + def shutdown_memory_provider(self): + pass + + def close(self): + pass + + def _bare_agent() -> AIAgent: agent = object.__new__(AIAgent) agent.model = "fake-model" @@ -31,6 +88,7 @@ def _bare_agent() -> AIAgent: agent._safe_print = lambda *_args, **_kwargs: None import threading as _threading agent._background_review_agent = None + agent._background_review_run = None agent._background_review_lock = _threading.Lock() agent._active_children = [] agent._active_children_lock = _threading.Lock() @@ -45,6 +103,89 @@ class ImmediateThread: self._target() +def _install_live_turn_boundary(monkeypatch, on_boundary=None): + import agent.conversation_loop as conversation_loop_module + + def stop_at_boundary(*args, **kwargs): + if on_boundary is not None: + on_boundary() + raise _TurnBoundaryReached + + monkeypatch.setattr( + conversation_loop_module, + "build_turn_context", + stop_at_boundary, + ) + + +def _run_wrapped_live_turn_to_boundary(agent, result): + try: + result["return"] = AIAgent.run_conversation( + agent, + "next turn", + task_id="live-task", + ) + except _TurnBoundaryReached: + result["boundary_reached"] = True + except BaseException as exc: # surfaced in the test thread for a useful failure + result["error"] = exc + + +def _install_relay_recorder(monkeypatch, review_run=None): + from agent import relay_runtime + from hermes_cli.observability import relay_shared_metrics + + calls = [] + + def review_acknowledged(): + return bool(review_run and review_run.request_done.is_set()) + + class RelayTurn: + relay_enabled = True + + class RecordingCoordinator: + def acquire_conversation(self, **kwargs): + calls.append(("acquire", review_acknowledged())) + return object() + + def begin_turn(self, lease, **kwargs): + calls.append(("begin", review_acknowledged())) + return RelayTurn() + + def finish_logical_calls(self, turn, **kwargs): + pass + + def end_turn(self, turn, **kwargs): + pass + + def release_conversation(self, lease): + pass + + monkeypatch.setattr( + relay_runtime, + "SESSION_COORDINATOR", + RecordingCoordinator(), + ) + monkeypatch.setattr( + relay_runtime, + "current_profile_key", + lambda: "/test-profile", + ) + monkeypatch.setattr( + relay_shared_metrics, + "start_task_run", + lambda **kwargs: calls.append( + ("start_task_run", review_acknowledged()) + ), + ) + monkeypatch.setattr( + relay_shared_metrics, + "finish_task_run", + lambda **kwargs: None, + ) + return calls + + def test_background_review_shuts_down_memory_provider_before_close(monkeypatch): events = [] @@ -277,34 +418,19 @@ def test_background_review_explicit_focus_runs_even_in_subagent(monkeypatch): assert len(forks) == 1, "explicit focus review must run even in a subagent" -def test_background_review_registers_on_active_children_for_interrupt(monkeypatch): - """The review fork must be added to the parent's ``_active_children`` so - ``AIAgent.interrupt()`` (which fans out to that list) can reach it, and - to ``_background_review_agent`` so the NEXT live turn can proactively - cancel a still-running review. Regression for the doubled-token- - accounting / Ctrl+C-proof lockup that a review racing a new live turn - against the same session_id/credentials can cause. - """ +def test_background_review_registers_before_start_runs_and_cleans_up(monkeypatch): + """The parent must own a unique review run before the worker can start.""" seen = {} - class FakeReviewAgent: - def __init__(self, **kwargs): - self._session_messages = [] - + class RecordingReviewAgent(FakeReviewAgent): def run_conversation(self, **kwargs): - # While run_conversation is "in flight", both tracking slots on - # the parent must already point at this fork. + seen["run"] = agent._background_review_run seen["active_children_during_run"] = list(agent._active_children) seen["background_review_agent_during_run"] = agent._background_review_agent - def shutdown_memory_provider(self): - pass - - def close(self): - pass - - monkeypatch.setattr(run_agent_module, "AIAgent", FakeReviewAgent) - monkeypatch.setattr(run_agent_module.threading, "Thread", ImmediateThread) + monkeypatch.setattr(run_agent_module, "AIAgent", RecordingReviewAgent) + CapturingThread.targets = [] + monkeypatch.setattr(run_agent_module.threading, "Thread", CapturingThread) agent = _bare_agent() @@ -314,53 +440,315 @@ def test_background_review_registers_on_active_children_for_interrupt(monkeypatc review_memory=True, ) + run = agent._background_review_run + assert run is not None + assert len(CapturingThread.targets) == 1 + assert not run.request_done.is_set() + + observed_done = ObservedEvent() + run.request_done = observed_done + CapturingThread.targets[0]() + fork = seen["background_review_agent_during_run"] assert fork is not None + assert seen["run"] is run assert seen["active_children_during_run"] == [fork] - - # After the review completes, both tracking slots must be cleared — - # otherwise a later interrupt() would try to cancel an already-closed - # agent, or the next turn would wait on a review that no longer exists. + assert observed_done.is_set() + assert observed_done.set_calls == 1 + assert agent._background_review_run is None assert agent._background_review_agent is None assert agent._active_children == [] -def test_new_live_turn_cancels_still_running_background_review(monkeypatch): - """conversation_loop.run_conversation() must proactively interrupt a - background review still in flight from a prior turn, rather than let the - two race concurrently against the same session_id/credentials. This is - the other half of the fix: registration alone only enables interrupt() - propagation, it doesn't by itself stop the race — something has to - actually call interrupt() at the start of the next turn. - """ - import agent.conversation_loop as conversation_loop_module +def test_live_turn_waits_for_review_exit_before_relay_and_turn_context(monkeypatch): + """The outer production wrapper waits before same-session instrumentation.""" + review_entered = threading.Event() + review_returned = threading.Event() + allow_review_return = threading.Event() + interrupted = threading.Event() + boundary_reached = threading.Event() + seen = {} - calls = [] - - class FakeReviewAgent: + class BlockingReviewAgent(FakeReviewAgent): def interrupt(self, message=None): - calls.append(message) + seen["interrupt_message"] = message + interrupted.set() + + def run_conversation(self, **kwargs): + review_entered.set() + assert allow_review_return.wait(2.0) + review_returned.set() + + monkeypatch.setattr(run_agent_module, "AIAgent", BlockingReviewAgent) + CapturingThread.targets = [] + monkeypatch.setattr(run_agent_module.threading, "Thread", CapturingThread) agent = _bare_agent() - agent._background_review_agent = FakeReviewAgent() + AIAgent._spawn_background_review( + agent, + messages_snapshot=[{"role": "user", "content": "hello"}], + review_memory=True, + ) + run = agent._background_review_run + assert run is not None + observed_done = ObservedEvent() + run.request_done = observed_done - # Invoke just the cancellation snippet in isolation via the same - # attribute contract run_conversation() reads, to avoid dragging in the - # rest of the turn machinery (network calls, tool setup, etc.) that - # isn't relevant to this regression. - _pending_review = getattr(agent, "_background_review_agent", None) - assert _pending_review is not None - _pending_review.interrupt("superseded by a new live turn") + monkeypatch.setattr(run_agent_module.threading, "Thread", _REAL_THREAD) + worker = _REAL_THREAD(target=CapturingThread.targets[0], daemon=True) + worker.start() + assert review_entered.wait(2.0) - assert calls == ["superseded by a new live turn"] + def on_boundary(): + seen["review_returned_at_boundary"] = review_returned.is_set() + boundary_reached.set() + + _install_live_turn_boundary(monkeypatch, on_boundary) + relay_calls = _install_relay_recorder(monkeypatch, run) + live_result = {} + live = _REAL_THREAD( + target=_run_wrapped_live_turn_to_boundary, + args=(agent, live_result), + daemon=True, + ) + live.start() + + assert interrupted.wait(2.0) + wait_started = observed_done.wait_started.wait(2.0) + relay_calls_before_ack = list(relay_calls) + allow_review_return.set() + worker.join(timeout=2.0) + live.join(timeout=2.0) + + assert not worker.is_alive() + assert not live.is_alive() + assert wait_started + assert relay_calls_before_ack == [] + assert relay_calls == [ + ("acquire", True), + ("begin", True), + ("start_task_run", True), + ] + assert boundary_reached.is_set() + assert seen["interrupt_message"] == "superseded by a new live turn" + assert seen["review_returned_at_boundary"] is True + assert live_result == {"boundary_reached": True} +def test_live_turn_cancels_review_during_startup_before_provider(monkeypatch): + """A review cancelled before its worker runs must never call its provider.""" + provider_calls = [] + boundary_reached = threading.Event() + + class RecordingReviewAgent(FakeReviewAgent): + def run_conversation(self, **kwargs): + provider_calls.append(kwargs) + + monkeypatch.setattr(run_agent_module, "AIAgent", RecordingReviewAgent) + CapturingThread.targets = [] + monkeypatch.setattr(run_agent_module.threading, "Thread", CapturingThread) + + agent = _bare_agent() + AIAgent._spawn_background_review( + agent, + messages_snapshot=[{"role": "user", "content": "hello"}], + review_memory=True, + ) + run = agent._background_review_run + assert run is not None + + _install_live_turn_boundary(monkeypatch, boundary_reached.set) + relay_calls = _install_relay_recorder(monkeypatch, run) + live_result = {} + live = _REAL_THREAD( + target=_run_wrapped_live_turn_to_boundary, + args=(agent, live_result), + daemon=True, + ) + live.start() + assert run.cancel_requested.wait(2.0) + + worker = _REAL_THREAD(target=CapturingThread.targets[0], daemon=True) + worker.start() + worker.join(timeout=2.0) + live.join(timeout=2.0) + + assert not worker.is_alive() + assert not live.is_alive() + assert boundary_reached.is_set() + assert provider_calls == [] + assert run.request_done.is_set() + assert relay_calls == [ + ("acquire", True), + ("begin", True), + ("start_task_run", True), + ] + assert live_result == {"boundary_reached": True} +def test_live_turn_stops_safely_when_review_acknowledgement_times_out(monkeypatch): + """A broken review abort path must not create an unbounded foreground wait.""" + import time + + import agent.background_review as background_review_module + + review_entered = threading.Event() + interrupt_entered = threading.Event() + interrupt_returned = threading.Event() + allow_interrupt_return = threading.Event() + allow_review_return = threading.Event() + + class WedgedReviewAgent(FakeReviewAgent): + def run_conversation(self, **kwargs): + review_entered.set() + allow_review_return.wait(5.0) + + def interrupt(self, message=None): + interrupt_entered.set() + allow_interrupt_return.wait(5.0) + interrupt_returned.set() + + monkeypatch.setattr(run_agent_module, "AIAgent", WedgedReviewAgent) + CapturingThread.targets = [] + monkeypatch.setattr(run_agent_module.threading, "Thread", CapturingThread) + monkeypatch.setattr( + background_review_module, + "_BACKGROUND_REVIEW_CANCEL_TIMEOUT_SECONDS", + 0.01, + raising=False, + ) + + agent = _bare_agent() + AIAgent._spawn_background_review( + agent, + messages_snapshot=[{"role": "user", "content": "hello"}], + review_memory=True, + ) + run = agent._background_review_run + assert run is not None + monkeypatch.setattr(run_agent_module.threading, "Thread", _REAL_THREAD) + worker = _REAL_THREAD(target=CapturingThread.targets[0], daemon=True) + worker.start() + assert review_entered.wait(2.0) + + boundary_calls = [] + _install_live_turn_boundary( + monkeypatch, lambda: boundary_calls.append(True) + ) + relay_calls = _install_relay_recorder(monkeypatch, run) + + started = time.monotonic() + result = AIAgent.run_conversation( + agent, + "next turn", + task_id="live-task", + ) + elapsed = time.monotonic() - started + + allow_interrupt_return.set() + allow_review_return.set() + worker.join(timeout=2.0) + + assert elapsed < 2.0 + assert interrupt_entered.is_set() + assert interrupt_returned.wait(2.0) + assert not worker.is_alive() + assert relay_calls == [] + assert boundary_calls == [] + assert result is not None + assert result["completed"] is False + assert result["failed"] is True + assert result["background_review_cancellation_timeout"] is True + assert "not started" in result["final_response"].lower() +def test_live_turn_fails_closed_for_untracked_legacy_review_stub(monkeypatch): + """Directly constructed agents without a handshake must not overlap turns.""" + interrupts = [] + interrupt_called = threading.Event() + + class LegacyReviewAgent: + def interrupt(self, message=None): + interrupts.append(message) + interrupt_called.set() + + agent = _bare_agent() + del agent._background_review_run + agent._background_review_agent = LegacyReviewAgent() + boundary_calls = [] + _install_live_turn_boundary( + monkeypatch, lambda: boundary_calls.append(True) + ) + relay_calls = _install_relay_recorder(monkeypatch) + + result = AIAgent.run_conversation( + agent, + "next turn", + task_id="live-task", + ) + + assert interrupt_called.wait(2.0) + assert interrupts == ["superseded by a new live turn"] + assert relay_calls == [] + assert boundary_calls == [] + assert result["background_review_cancellation_timeout"] is True + assert result["failed"] is True +def test_stale_review_cleanup_cannot_clear_or_signal_newer_review(monkeypatch): + """A retired worker's late cleanup must be scoped to its own run identity.""" + first_cleanup_entered = threading.Event() + allow_first_cleanup = threading.Event() + instance_count = 0 + + class BlockingCleanupReviewAgent(FakeReviewAgent): + def __init__(self, **kwargs): + nonlocal instance_count + super().__init__(**kwargs) + self.index = instance_count + instance_count += 1 + + def shutdown_memory_provider(self): + if self.index == 0: + first_cleanup_entered.set() + assert allow_first_cleanup.wait(2.0) + + monkeypatch.setattr(run_agent_module, "AIAgent", BlockingCleanupReviewAgent) + CapturingThread.targets = [] + monkeypatch.setattr(run_agent_module.threading, "Thread", CapturingThread) + + agent = _bare_agent() + AIAgent._spawn_background_review( + agent, + messages_snapshot=[{"role": "user", "content": "first"}], + review_memory=True, + ) + first_run = agent._background_review_run + first_worker = _REAL_THREAD(target=CapturingThread.targets[0], daemon=True) + first_worker.start() + assert first_cleanup_entered.wait(2.0) + assert first_run.request_done.is_set() + + AIAgent._spawn_background_review( + agent, + messages_snapshot=[{"role": "user", "content": "second"}], + review_memory=True, + ) + second_run = agent._background_review_run + second_target = CapturingThread.targets[1] + assert second_run is not first_run + assert not second_run.request_done.is_set() + + allow_first_cleanup.set() + first_worker.join(timeout=2.0) + + assert not first_worker.is_alive() + assert agent._background_review_run is second_run + assert not second_run.request_done.is_set() + + second_target() + assert second_run.request_done.is_set() + assert agent._background_review_run is None # --------------------------------------------------------------------------- # memory_notifications mode: off | on | verbose From 1b92a9496206d06274e1837afe0ea82fee8d6374 Mon Sep 17 00:00:00 2001 From: qixuancao Date: Wed, 12 Aug 2026 18:50:19 +0800 Subject: [PATCH 10/37] refactor(agent): simplify background review run state --- agent/background_review.py | 11 ++--------- tests/run_agent/test_background_review.py | 1 - 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/agent/background_review.py b/agent/background_review.py index d643b95e12..51e20e92da 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -42,28 +42,22 @@ class _BackgroundReviewRun: self.request_done = threading.Event() self._lock = threading.Lock() self._review_agent = None - self._request_started = False self._request_finished = False self._cancel_dispatched = False def begin_request(self, review_agent: Any) -> bool: """Atomically admit the first provider-capable review phase.""" with self._lock: - self._review_agent = review_agent if self.cancel_requested.is_set() or self._request_finished: return False - self._request_started = True + self._review_agent = review_agent return True def cancel(self) -> Any: """Fence startup and return the running fork, if one was admitted.""" with self._lock: self.cancel_requested.set() - if ( - self._request_started - and not self._request_finished - and not self._cancel_dispatched - ): + if self._review_agent is not None and not self._cancel_dispatched: self._cancel_dispatched = True return self._review_agent return None @@ -74,7 +68,6 @@ class _BackgroundReviewRun: if self._request_finished: return False self._request_finished = True - self._request_started = False self._review_agent = None return True diff --git a/tests/run_agent/test_background_review.py b/tests/run_agent/test_background_review.py index 19aa95f395..b1ef719005 100644 --- a/tests/run_agent/test_background_review.py +++ b/tests/run_agent/test_background_review.py @@ -19,7 +19,6 @@ class CapturingThread: targets = [] def __init__(self, *, target, daemon=None, name=None): - self._target = target self.targets.append(target) def start(self): From b883756b79374c8e8be770b383e51a421bcd8665 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Fri, 21 Aug 2026 13:58:43 +0530 Subject: [PATCH 11/37] fix: foreground priority for background review cancel timeout Change fail-closed behavior to proceed-with-warning when a background review does not acknowledge cancellation within the bounded deadline. The review is non-critical self-improvement work and must never block a user-facing turn (#84423). Keep the off-thread interrupt to ensure a broken abort path cannot stall the bounded wait. --- agent/background_review.py | 25 +++++---- run_agent.py | 23 ++------ tests/run_agent/test_background_review.py | 64 +++++++++++++++-------- 3 files changed, 60 insertions(+), 52 deletions(-) diff --git a/agent/background_review.py b/agent/background_review.py index 51e20e92da..42979f3bd0 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -113,7 +113,13 @@ def finish_background_review_run( def _interrupt_background_review(review_agent: Any) -> None: - """Request abort off-thread so a broken abort hook cannot stall foreground.""" + """Request abort off-thread so a broken abort hook cannot stall foreground. + + The bounded wait on ``request_done`` in + :func:`cancel_background_review_for_live_turn` is only effective if + ``interrupt()`` returns quickly. Off-loading to a daemon thread ensures + a slow or wedged abort path cannot block the foreground turn (#84423). + """ def _interrupt() -> None: try: @@ -137,11 +143,13 @@ def _interrupt_background_review(review_agent: Any) -> None: ) -def cancel_background_review_for_live_turn(agent: Any) -> bool: +def cancel_background_review_for_live_turn(agent: Any) -> None: """Cancel the current review and await its request-phase acknowledgement. - Returns ``False`` when acknowledgement is unavailable or misses the bounded - deadline. Callers must then stop before same-session turn-context work. + Foreground priority is preserved: if the review does not acknowledge within + the bounded deadline, a warning is logged and the live turn proceeds + anyway. The review is non-critical self-improvement work and must never + block a user-facing turn (#84423). """ lock = getattr(agent, "_background_review_lock", None) if lock is not None: @@ -154,9 +162,9 @@ def cancel_background_review_for_live_turn(agent: Any) -> bool: if run is None: if legacy_agent is None: - return True + return _interrupt_background_review(legacy_agent) - return False + return review_agent = run.cancel() if review_agent is not None: @@ -166,12 +174,11 @@ def cancel_background_review_for_live_turn(agent: Any) -> bool: timeout=_BACKGROUND_REVIEW_CANCEL_TIMEOUT_SECONDS ) if not acknowledged: - logger.error( + logger.warning( "Background review did not acknowledge cancellation within %.1fs; " - "refusing to start overlapping live turn", + "proceeding with foreground live turn", _BACKGROUND_REVIEW_CANCEL_TIMEOUT_SECONDS, ) - return acknowledged # --------------------------------------------------------------------------- diff --git a/run_agent.py b/run_agent.py index 107c0f33ab..6237738141 100644 --- a/run_agent.py +++ b/run_agent.py @@ -8496,28 +8496,11 @@ class AIAgent: # A review deliberately shares this agent's session_id for prompt-cache # parity. Fence review startup or interrupt an admitted request, then # await that request's exit before opening any live-turn Relay or task - # instrumentation for the same session. + # instrumentation for the same session. Foreground priority is retained + # if the review does not acknowledge within the bounded deadline (#84423). from agent.background_review import cancel_background_review_for_live_turn - if not cancel_background_review_for_live_turn(self): - failure = ( - "Live turn not started: the prior background review did not " - "acknowledge cancellation before the bounded safety deadline. " - "Retry after the review has stopped." - ) - existing_messages = conversation_history - if existing_messages is None: - existing_messages = getattr(self, "_session_messages", None) - return { - "final_response": failure, - "messages": list(existing_messages or []), - "completed": False, - "api_calls": 0, - "error": failure, - "failed": True, - "retryable": True, - "background_review_cancellation_timeout": True, - } + cancel_background_review_for_live_turn(self) from agent.aux_accounting import ( reset_accounting_context, diff --git a/tests/run_agent/test_background_review.py b/tests/run_agent/test_background_review.py index b1ef719005..930dfb7428 100644 --- a/tests/run_agent/test_background_review.py +++ b/tests/run_agent/test_background_review.py @@ -585,8 +585,10 @@ def test_live_turn_cancels_review_during_startup_before_provider(monkeypatch): assert live_result == {"boundary_reached": True} -def test_live_turn_stops_safely_when_review_acknowledgement_times_out(monkeypatch): - """A broken review abort path must not create an unbounded foreground wait.""" +def test_live_turn_proceeds_when_review_acknowledgement_times_out(monkeypatch): + """A broken review abort path must not block the foreground indefinitely. + The live turn proceeds after the bounded wait, retaining foreground priority. + """ import time import agent.background_review as background_review_module @@ -637,11 +639,15 @@ def test_live_turn_stops_safely_when_review_acknowledgement_times_out(monkeypatc relay_calls = _install_relay_recorder(monkeypatch, run) started = time.monotonic() - result = AIAgent.run_conversation( - agent, - "next turn", - task_id="live-task", + live_result = {} + live = _REAL_THREAD( + target=_run_wrapped_live_turn_to_boundary, + args=(agent, live_result), + daemon=True, ) + live.start() + live.join(timeout=5.0) + elapsed = time.monotonic() - started allow_interrupt_return.set() @@ -652,17 +658,21 @@ def test_live_turn_stops_safely_when_review_acknowledgement_times_out(monkeypatc assert interrupt_entered.is_set() assert interrupt_returned.wait(2.0) assert not worker.is_alive() - assert relay_calls == [] - assert boundary_calls == [] - assert result is not None - assert result["completed"] is False - assert result["failed"] is True - assert result["background_review_cancellation_timeout"] is True - assert "not started" in result["final_response"].lower() + assert not live.is_alive() + # Foreground retains priority: Relay/turn-context proceed even though + # the review did not acknowledge within the bounded deadline. + assert boundary_calls == [True] + assert live_result == {"boundary_reached": True} + assert relay_calls == [ + ("acquire", False), + ("begin", False), + ("start_task_run", False), + ] + assert agent.session_id == "test-session" -def test_live_turn_fails_closed_for_untracked_legacy_review_stub(monkeypatch): - """Directly constructed agents without a handshake must not overlap turns.""" +def test_live_turn_interrupts_legacy_review_but_keeps_foreground_priority(monkeypatch): + """Legacy stubs are interrupted without turning review into a user blocker.""" interrupts = [] interrupt_called = threading.Event() @@ -680,18 +690,26 @@ def test_live_turn_fails_closed_for_untracked_legacy_review_stub(monkeypatch): ) relay_calls = _install_relay_recorder(monkeypatch) - result = AIAgent.run_conversation( - agent, - "next turn", - task_id="live-task", + live_result = {} + live = _REAL_THREAD( + target=_run_wrapped_live_turn_to_boundary, + args=(agent, live_result), + daemon=True, ) + live.start() + live.join(timeout=5.0) assert interrupt_called.wait(2.0) assert interrupts == ["superseded by a new live turn"] - assert relay_calls == [] - assert boundary_calls == [] - assert result["background_review_cancellation_timeout"] is True - assert result["failed"] is True + assert not live.is_alive() + assert boundary_calls == [True] + assert live_result == {"boundary_reached": True} + assert relay_calls == [ + ("acquire", False), + ("begin", False), + ("start_task_run", False), + ] + assert agent.session_id == "test-session" def test_stale_review_cleanup_cannot_clear_or_signal_newer_review(monkeypatch): From 8bdf0e89e23841ee385c70b2630c5af6756f3efd Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:08:29 +0530 Subject: [PATCH 12/37] chore: add qixuancao to AUTHOR_MAP --- contributors/emails/qixuancao36@gmail.com | 1 + 1 file changed, 1 insertion(+) create mode 100644 contributors/emails/qixuancao36@gmail.com diff --git a/contributors/emails/qixuancao36@gmail.com b/contributors/emails/qixuancao36@gmail.com new file mode 100644 index 0000000000..661be63dbb --- /dev/null +++ b/contributors/emails/qixuancao36@gmail.com @@ -0,0 +1 @@ +qixuancao From 80aef061fe064d8bb1dfcf89813c9c9e1e4cbdba Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:09:07 +0530 Subject: [PATCH 13/37] refactor: use request_hard_interrupt for review cancellation Replaces direct review_agent.interrupt() call with the existing agent.interrupt_compat.request_hard_interrupt() helper, which handles both the new hard_interrupt ABI and the legacy interrupt fallback. --- agent/background_review.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/agent/background_review.py b/agent/background_review.py index 42979f3bd0..f48a3d5200 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -123,7 +123,9 @@ def _interrupt_background_review(review_agent: Any) -> None: def _interrupt() -> None: try: - review_agent.interrupt("superseded by a new live turn") + from agent.interrupt_compat import request_hard_interrupt + + request_hard_interrupt(review_agent, "superseded by a new live turn") except Exception: logger.debug( "Failed to cancel in-flight background review for a new turn", From 40b4a3bfe1504c3e9b078fe25ff8503e24bb6a5d Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 21 Aug 2026 03:30:16 -0700 Subject: [PATCH 14/37] =?UTF-8?q?fix(update):=20every=20begun=20update=20r?= =?UTF-8?q?eceipt=20is=20now=20persisted=20=E2=80=94=20command-boundary=20?= =?UTF-8?q?finalization?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #91283: begin_update_receipt() fires early in _cmd_update_impl, but finalization only existed on the success/ZIP/CalledProcessError paths. Early sys.exit paths (Windows concurrent-instance preflight, venv-holder refusal, head-pinned no-op, fetch failure) terminated with the receipt started but never written — losing exactly the refused/ failed runs the receipt matters most for. - update_receipt.py: finalize_pending_update_receipt(exit_code, stop_reason) — boundary safety net; maps exit 2 → 'refused', other non-zero → 'failed'; records exit_code + stop_reason. Exactly-once by construction (singleton popped in finalize_update_receipt), so runs the inner paths already finalized are untouched. - main.py cmd_update: SystemExit/BaseException/else arms around _cmd_update_impl persist any still-open receipt with the real exit code, then re-raise unchanged. Future early exits are covered without per-site finalize patches. - 5 regression tests incl. end-to-end through the real cmd_update wrapper (exit 2 preserved, outcome 'refused', stop reason recorded, singleton cleared, exactly one receipt file). --- hermes_cli/main.py | 32 +++++++ hermes_cli/update_receipt.py | 47 ++++++++++- tests/hermes_cli/test_update_receipt.py | 106 ++++++++++++++++++++++++ 3 files changed, 183 insertions(+), 2 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 0c7c51608f..7aa49f83d4 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -10130,6 +10130,38 @@ def cmd_update(args): try: _self()._cmd_update_impl(args, gateway_mode=gateway_mode) + except SystemExit as _update_exit: + # Receipt boundary (#91283 review): the impl has many early + # sys.exit paths (concurrent-instance preflight, venv-holder + # refusal, head-pinned no-op, fetch failure) that never reach an + # inner finalize. Persist any still-open receipt with the real + # exit code, then let the exit proceed unchanged. No-op when an + # inner path already finalized (exactly-once by construction). + try: + from hermes_cli.update_receipt import finalize_pending_update_receipt + + _code = _update_exit.code if isinstance(_update_exit.code, int) else 1 + finalize_pending_update_receipt(_code, f"sys.exit({_code})") + except Exception: + pass + raise + except BaseException as _update_exc: + try: + from hermes_cli.update_receipt import finalize_pending_update_receipt + + finalize_pending_update_receipt( + 1, f"{type(_update_exc).__name__}: {_update_exc}" + ) + except Exception: + pass + raise + else: + try: + from hermes_cli.update_receipt import finalize_pending_update_receipt + + finalize_pending_update_receipt(0, "completed at command boundary") + except Exception: + pass finally: _update_lock.release() _finalize_update_output(_update_io_state) diff --git a/hermes_cli/update_receipt.py b/hermes_cli/update_receipt.py index 63769c4a07..db90a2f319 100644 --- a/hermes_cli/update_receipt.py +++ b/hermes_cli/update_receipt.py @@ -166,10 +166,15 @@ def record_gateway_restart(**kwargs: Any) -> None: logger.debug("Could not record gateway restart result: %s", exc) -def finalize_update_receipt(outcome: str, fleet: list | None = None) -> Optional[Path]: +def finalize_update_receipt( + outcome: str, fleet: list | None = None, stop_reason: str = "" +) -> Optional[Path]: """Finalize + persist the receipt. Returns the written path or None. - ``outcome`` is one of ``success`` / ``partial`` / ``failed``. + ``outcome`` is one of ``success`` / ``partial`` / ``failed`` / + ``refused``. Exactly-once by construction: the module singleton is + popped first, so a second call (e.g. the command-boundary safety net + after an inner path already finalized) is a no-op returning None. """ global _current receipt = _current @@ -178,6 +183,8 @@ def finalize_update_receipt(outcome: str, fleet: list | None = None) -> Optional return None try: receipt.finalize(outcome) + if stop_reason: + receipt.data["stop_reason"] = stop_reason if fleet is not None: receipt.data["fleet"] = fleet directory = _receipt_dir() @@ -202,6 +209,42 @@ def finalize_update_receipt(outcome: str, fleet: list | None = None) -> Optional return None +def finalize_pending_update_receipt( + exit_code: Optional[int] = None, stop_reason: str = "" +) -> Optional[Path]: + """Command-boundary safety net: persist a still-open receipt, if any. + + ``hermes update`` has many early-termination paths (Windows + concurrent-instance preflight, venv-holder refusal, head-pinned no-op, + fetch failure — all ``sys.exit``) that predate the inner finalize + call sites. Any receipt still open when the update COMMAND unwinds is + finalized here so every post-begin run leaves a record — the + refused/failed runs are exactly the ones a receipt matters most for + (review on #91283). No-op when no receipt is open (the inner paths + already finalized — exactly-once via the popped singleton) or when + recording was never started. Never raises. + + Outcome mapping: exit 0/None → ``success`` (a path that completed + without an explicit inner finalize), exit 2 → ``refused`` (the + updater's preflight-refusal convention), anything else → ``failed``. + """ + if _current is None: + return None + if exit_code in (0, None): + outcome = "success" + elif exit_code == 2: + outcome = "refused" + else: + outcome = "failed" + try: + receipt = _current + if receipt is not None and exit_code is not None: + receipt.data["exit_code"] = int(exit_code) + except Exception: + pass + return finalize_update_receipt(outcome, stop_reason=stop_reason) + + def _prune_old_receipts(directory: Path) -> None: try: receipts = sorted( diff --git a/tests/hermes_cli/test_update_receipt.py b/tests/hermes_cli/test_update_receipt.py index 6f2d3f9968..e120bb9e06 100644 --- a/tests/hermes_cli/test_update_receipt.py +++ b/tests/hermes_cli/test_update_receipt.py @@ -11,6 +11,7 @@ Covers: import json import os +import sys from pathlib import Path import pytest @@ -121,6 +122,111 @@ class TestReceiptLifecycle: assert ur.read_latest_receipt() is None +class TestCommandBoundaryFinalization: + """Receipt lifetime is owned by the update-command boundary (#91283 review). + + Early sys.exit paths (concurrent-instance preflight exit-2, venv-holder + refusal, fetch failure) predate the inner finalize sites; the boundary + safety net must persist the receipt exactly once with the stop reason, + while inner-finalized runs are untouched. + """ + + def test_pending_receipt_persisted_on_exit_2_refusal(self, receipt_home): + ur.begin_update_receipt() + ur.record_step("windows_preflight", False, "another hermes.exe running") + path = ur.finalize_pending_update_receipt(2, "sys.exit(2)") + assert path is not None and path.is_file() + payload = json.loads(path.read_text(encoding="utf-8")) + assert payload["outcome"] == "refused" + assert payload["exit_code"] == 2 + assert payload["stop_reason"] == "sys.exit(2)" + assert payload["finished_at"] is not None + assert ur._current is None + + def test_pending_receipt_persisted_on_exit_1_failure(self, receipt_home): + ur.begin_update_receipt() + path = ur.finalize_pending_update_receipt(1, "sys.exit(1)") + payload = json.loads(path.read_text(encoding="utf-8")) + assert payload["outcome"] == "failed" + assert payload["exit_code"] == 1 + + def test_noop_when_inner_path_already_finalized(self, receipt_home): + """Exactly-once: boundary call after an inner finalize writes nothing.""" + ur.begin_update_receipt() + first = ur.finalize_update_receipt("success") + assert first is not None + second = ur.finalize_pending_update_receipt(0, "boundary") + assert second is None + directory = receipt_home / "logs" / "update_receipts" + assert len(list(directory.glob("update_*.json"))) == 1 + + def test_noop_when_never_begun(self, receipt_home): + assert ur.finalize_pending_update_receipt(2, "sys.exit(2)") is None + assert ur.read_latest_receipt() is None + + def test_cmd_update_boundary_finalizes_on_early_exit( + self, receipt_home, monkeypatch + ): + """End-to-end through the real cmd_update wrapper: an impl that begins + a receipt then sys.exit(2)s (the concurrent-instance shape) must leave + a finalized 'refused' receipt, preserve the exit code, and clear the + singleton.""" + from types import SimpleNamespace + + from hermes_cli import main as hermes_main + + def _fake_impl(args, gateway_mode): + ur.begin_update_receipt() + ur.record_step("windows_preflight", False, "hermes.exe holds venv") + sys.exit(2) + + monkeypatch.setattr(hermes_main, "_cmd_update_impl", _fake_impl) + monkeypatch.setattr( + hermes_main, "detect_install_method", lambda *a, **k: "git", raising=False + ) + monkeypatch.setattr( + hermes_main, + "_install_hangup_protection", + lambda gateway_mode: None, + raising=False, + ) + monkeypatch.setattr( + hermes_main, "_finalize_update_output", lambda state: None, raising=False + ) + + class _FakeLock: + holder = None + + def acquire(self): + return True + + def release(self): + pass + + import hermes_cli.update_lock as update_lock_mod + + monkeypatch.setattr(update_lock_mod, "UpdateLock", _FakeLock) + + args = SimpleNamespace( + check=False, gateway=False, branch=None, yes=False, + force=False, force_venv=False, + ) + with pytest.raises(SystemExit) as exc_info: + hermes_main.cmd_update(args) + + assert exc_info.value.code == 2 # exit code preserved + latest = ur.read_latest_receipt() + assert latest is not None + assert latest["outcome"] == "refused" + assert latest["exit_code"] == 2 + assert latest["stop_reason"] == "sys.exit(2)" + assert latest["steps"][0]["name"] == "windows_preflight" + assert ur._current is None + # exactly-once: exactly one receipt file + directory = receipt_home / "logs" / "update_receipts" + assert len(list(directory.glob("update_*.json"))) == 1 + + class TestFleetClassification: def _fleet_with(self, monkeypatch, tmp_path, record, expected_sha="a" * 40): """Run collect_fleet_versions against one fake default profile.""" From ef04d846e9505388d8eab61b7c6608acb652bf59 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Fri, 21 Aug 2026 03:38:02 -0700 Subject: [PATCH 15/37] feat(cron): cron agents now run with memory enabled like every other agent Cron jobs were constructed with skip_memory=True and a hard 'memory' toolset denial, so MEMORY.md/USER.md never loaded and the memory tool was stripped even from per-job enabled_toolsets. That was inconsistent with kanban/delegate/gateway agents (which all get memory) and forced users into hacky bypasses. - cron/scheduler.py: skip_memory=False on the cron AIAgent; drop 'memory' from _resolve_cron_disabled_toolsets; remove _strip_cron_memory_toolset and its call sites - agent/agent_init.py: update stale comment referencing the cron denylist - tests: flip pinning tests to the new contract (memory enabled, per-job memory toolset kept, user-level denylist still wins) - docs: cron-internals + automate-with-cron no longer claim cron has no persistent memory --- agent/agent_init.py | 7 +-- cron/scheduler.py | 37 +++++----------- tests/cron/test_agent_scheduling_gate.py | 31 +++++++++---- tests/cron/test_scheduler.py | 43 ++++++++----------- .../docs/developer-guide/cron-internals.md | 4 +- website/docs/guides/automate-with-cron.md | 2 +- 6 files changed, 59 insertions(+), 65 deletions(-) diff --git a/agent/agent_init.py b/agent/agent_init.py index 88985abbdc..db92f18487 100644 --- a/agent/agent_init.py +++ b/agent/agent_init.py @@ -1816,9 +1816,10 @@ def init_agent( # skip_memory=True skips the external memory *provider*. Flush/background # agents can still pass enabled_toolsets=["memory"] so the built-in file # store exists and the memory tool does not fail with store=None (#65429). - # A toolset on disabled_toolsets is not a request. Cron always denylists - # memory, but the default cron toolset still names it, so an enabled-only - # check would load MEMORY.md into an auto-approve job. + # A toolset on disabled_toolsets is not a request: a caller that denylists + # memory while its default toolset still names it must not get MEMORY.md + # loaded by an enabled-only check. (Cron agents now run with + # skip_memory=False and take the normal path here.) _enabled_toolsets = agent.enabled_toolsets or [] _disabled_toolsets = agent.disabled_toolsets or [] _memory_toolset_requested = ( diff --git a/cron/scheduler.py b/cron/scheduler.py index 59ce8fafa8..dd82623ea1 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -358,13 +358,9 @@ class CronPromptInjectionBlocked(Exception): def _resolve_cron_disabled_toolsets(cfg: dict) -> list[str]: """Toolsets a cron-spawned agent must never receive. - Three toolsets are always disabled in cron context regardless of config: + Two toolsets are always disabled in cron context regardless of config: - ``messaging`` — interactive, needs a live gateway session - ``clarify`` — interactive, blocks waiting for user input - - ``memory`` — cron agents run with ``skip_memory=True``. The tool is - hidden, and ``memory`` is stripped from enabled_toolsets so the - built-in store is not created either (MEMORY.md would otherwise - land in the cron system prompt). ``cronjob`` is policy-denied by default (loop prevention, not a security boundary) and config-gated: setting ``cron.allow_agent_scheduling: true`` @@ -379,9 +375,9 @@ def _resolve_cron_disabled_toolsets(cfg: dict) -> list[str]: """ cron_cfg = (cfg or {}).get("cron") or {} if cron_cfg.get("allow_agent_scheduling"): - disabled = ["messaging", "clarify", "memory"] + disabled = ["messaging", "clarify"] else: - disabled = ["cronjob", "messaging", "clarify", "memory"] + disabled = ["cronjob", "messaging", "clarify"] agent_cfg = (cfg or {}).get("agent") or {} from agent.skill_utils import parse_config_string_list @@ -438,10 +434,6 @@ def _resolve_cron_enabled_toolsets(job: dict, cfg: dict) -> list[str] | None: 3. ``None`` on any lookup failure — AIAgent loads the full default set (legacy behavior before this change, preserved as the safety net). - ``memory`` is always stripped. Cron denylists that toolset and passes - ``skip_memory=True``. Leaving it in enabled_toolsets still constructs - the built-in MemoryStore and injects MEMORY.md into the job prompt. - _DEFAULT_OFF_TOOLSETS ({moa, homeassistant, rl}) are removed by ``_get_platform_tools`` for unconfigured platforms, so fresh installs get cron WITHOUT ``moa`` by default (issue reported by Norbert — @@ -449,12 +441,10 @@ def _resolve_cron_enabled_toolsets(job: dict, cfg: dict) -> list[str] | None: """ per_job = job.get("enabled_toolsets") if per_job: - return _strip_cron_memory_toolset( - _merge_mcp_into_per_job_toolsets(list(per_job), cfg or {}) - ) + return _merge_mcp_into_per_job_toolsets(list(per_job), cfg or {}) try: from hermes_cli.tools_config import _get_platform_tools # lazy: avoid heavy import at cron module load - return _strip_cron_memory_toolset(sorted(_get_platform_tools(cfg or {}, "cron"))) + return sorted(_get_platform_tools(cfg or {}, "cron")) except Exception as exc: logger.warning( "Cron toolset resolution failed, falling back to full default toolset: %s", @@ -463,17 +453,6 @@ def _resolve_cron_enabled_toolsets(job: dict, cfg: dict) -> list[str] | None: return None -def _strip_cron_memory_toolset(enabled: list[str] | None) -> list[str] | None: - """Drop ``memory`` from a cron enabled-toolset list. - - ``None`` means "full default set" and is left alone. skip_memory=True plus - the memory denylist still keep the store off on that fallback path. - """ - if enabled is None: - return None - return [name for name in enabled if name != "memory"] - - def _resolve_job_reasoning_config(job: dict, cfg: dict, model: str) -> dict | None: """Resolve the effective reasoning config for a cron run. @@ -5833,7 +5812,11 @@ def run_job( # Without a workdir, keep cwd context discovery disabled. skip_context_files=not bool(_job_workdir), load_soul_identity=True, - skip_memory=True, # Cron system prompts would corrupt user representations + # Memory is enabled for cron agents like any other agent run: + # MEMORY.md / USER.md load into the system prompt and the memory + # tool follows normal toolset resolution, so jobs benefit from + # (and can update) the user's persistent memory. + skip_memory=False, skip_background_review=True, # Cron has no human-in-the-loop need for skill/memory review forks (~30K tok/event) platform="cron", session_id=_cron_session_id, diff --git a/tests/cron/test_agent_scheduling_gate.py b/tests/cron/test_agent_scheduling_gate.py index abbe6a9fb4..b8dc88de2a 100644 --- a/tests/cron/test_agent_scheduling_gate.py +++ b/tests/cron/test_agent_scheduling_gate.py @@ -7,8 +7,8 @@ default off) makes that denial opt-out-able: - gate off / absent: byte-exact current behavior — ``cronjob`` denied. - gate on: ``cronjob`` dropped from the base denylist; ``messaging`` and - ``clarify`` (interactivity constraints) and ``memory`` (cron agents run - with skip_memory=True) are ALWAYS denied regardless of the gate. + ``clarify`` (interactivity constraints) are ALWAYS denied regardless of + the gate. - user-level ``agent.disabled_toolsets`` still layers on top, so a user who denies ``cronjob`` globally keeps it denied even with the gate on (per-job enabled_toolsets can never widen past the config denylist). @@ -20,26 +20,27 @@ from cron.scheduler import _resolve_cron_disabled_toolsets # The toolsets that must be denied in cron context no matter what the -# agent-scheduling gate says: messaging/clarify are interactive-only, -# memory stays off in cron runs (skip_memory=True, toolset denylisted). -ALWAYS_DISABLED = ["messaging", "clarify", "memory"] +# agent-scheduling gate says: messaging/clarify are interactive-only. +# ``memory`` is intentionally NOT here — cron agents get memory like any +# other agent run. +ALWAYS_DISABLED = ["messaging", "clarify"] class TestGateOffDefault: def test_empty_config_denies_cronjob(self): assert _resolve_cron_disabled_toolsets({}) == [ - "cronjob", "messaging", "clarify", "memory", + "cronjob", "messaging", "clarify", ] def test_none_config_denies_cronjob(self): assert _resolve_cron_disabled_toolsets(None) == [ - "cronjob", "messaging", "clarify", "memory", + "cronjob", "messaging", "clarify", ] def test_cron_section_present_but_gate_absent(self): cfg = {"cron": {"preflight": True}} assert _resolve_cron_disabled_toolsets(cfg) == [ - "cronjob", "messaging", "clarify", "memory", + "cronjob", "messaging", "clarify", ] def test_explicit_false_matches_default(self): @@ -60,12 +61,18 @@ class TestGateOn: disabled = _resolve_cron_disabled_toolsets(cfg) assert "cronjob" not in disabled - def test_interactivity_and_memory_denials_survive_the_gate(self): + def test_interactivity_denials_survive_the_gate(self): cfg = {"cron": {"allow_agent_scheduling": True}} disabled = _resolve_cron_disabled_toolsets(cfg) for name in ALWAYS_DISABLED: assert name in disabled + def test_memory_not_denied(self): + # Cron agents run with memory enabled like any other agent run + # (skip_memory=False); the toolset must not be policy-denied. + for cfg in ({}, {"cron": {"allow_agent_scheduling": True}}): + assert "memory" not in _resolve_cron_disabled_toolsets(cfg) + def test_user_denylist_wins_over_gate(self): # A user who denies cronjob in agent.disabled_toolsets keeps it # denied even with the gate on — the gate only removes the built-in @@ -94,6 +101,12 @@ class TestUserLayerUnchanged: # No duplicate when the user names an already-denied toolset. assert disabled.count("cronjob") == 1 + def test_user_can_still_deny_memory_for_cron(self): + # Memory is no longer policy-denied, but a user-level denylist + # entry still applies to cron runs. + cfg = {"agent": {"disabled_toolsets": ["memory"]}} + assert "memory" in _resolve_cron_disabled_toolsets(cfg) + def test_blank_and_whitespace_entries_ignored(self): cfg = { "cron": {"allow_agent_scheduling": True}, diff --git a/tests/cron/test_scheduler.py b/tests/cron/test_scheduler.py index 4bfe7b27c0..64033d0b09 100644 --- a/tests/cron/test_scheduler.py +++ b/tests/cron/test_scheduler.py @@ -118,23 +118,22 @@ class TestPerJobToolsetMcpMerge: assert m_platform.call_args[0][1] == "cron" assert set(result) == set(sentinel) - def test_resolver_strips_memory_from_per_job_list(self): + def test_resolver_keeps_memory_in_per_job_list(self): result = _resolve_cron_enabled_toolsets( {"enabled_toolsets": ["memory", "file"]}, {"mcp_servers": {}}, ) - assert "memory" not in result + assert "memory" in result assert "file" in result - def test_resolver_strips_memory_from_platform_fallback(self): + def test_resolver_keeps_memory_from_platform_fallback(self): job = {"enabled_toolsets": None} with patch( "hermes_cli.tools_config._get_platform_tools", return_value={"web", "memory", "file"}, ): result = _resolve_cron_enabled_toolsets(job, {}) - assert result == ["file", "web"] - assert "memory" not in result + assert result == ["file", "memory", "web"] class TestResolveOrigin: @@ -628,15 +627,15 @@ class TestRunJobSessionPersistence: yield fake_db, mock_agent_cls - def test_run_job_memory_toolset_disabled_in_cron(self, tmp_path): - """memory toolset must be disabled in cron sessions — issue #38129. + def test_run_job_memory_enabled_in_cron(self, tmp_path): + """Cron agents get memory like any other agent run. - Cron agents are constructed with skip_memory=True. The memory tool - stays off the schema, and memory is stripped from enabled_toolsets - so MEMORY.md is not loaded into the job prompt. + skip_memory=False and the memory toolset is not policy-denied, so + MEMORY.md/USER.md load and the memory tool follows normal toolset + resolution. """ job = { - "id": "memory-hide-job", + "id": "memory-enabled-job", "name": "test", "prompt": "hello", } @@ -644,17 +643,13 @@ class TestRunJobSessionPersistence: run_job(job) kwargs = mock_agent_cls.call_args.kwargs - assert "memory" in (kwargs["disabled_toolsets"] or []), ( - "memory toolset should be disabled in cron to match skip_memory=True" + assert kwargs["skip_memory"] is False + assert "memory" not in (kwargs["disabled_toolsets"] or []), ( + "memory toolset must not be policy-denied in cron" ) - def test_run_job_disables_memory_even_when_per_job_enables_it(self, tmp_path): - """Cron runs pass skip_memory=True, so memory must not be exposed. - - A cron job can name the memory toolset in enabled_toolsets. The - resolver drops it, and the denylist still lists it, so the model - never gets the tool and init never builds MemoryStore. - """ + def test_run_job_keeps_per_job_memory_toolset(self, tmp_path): + """A per-job enabled_toolsets naming memory keeps it.""" job = { "id": "memory-toolset-job", "name": "test", @@ -665,10 +660,10 @@ class TestRunJobSessionPersistence: run_job(job) kwargs = mock_agent_cls.call_args.kwargs - assert kwargs["skip_memory"] is True - assert kwargs["enabled_toolsets"] == ["file"] - assert "memory" not in (kwargs["enabled_toolsets"] or []) - assert "memory" in kwargs["disabled_toolsets"] + assert kwargs["skip_memory"] is False + assert "memory" in (kwargs["enabled_toolsets"] or []) + assert "file" in (kwargs["enabled_toolsets"] or []) + assert "memory" not in kwargs["disabled_toolsets"] def test_tick_skips_due_jobs_while_dispatch_is_paused(self, tmp_path): """The drain gate runs before advancing a due job's schedule.""" diff --git a/website/docs/developer-guide/cron-internals.md b/website/docs/developer-guide/cron-internals.md index 13a342324c..a18c1a9dd6 100644 --- a/website/docs/developer-guide/cron-internals.md +++ b/website/docs/developer-guide/cron-internals.md @@ -176,7 +176,9 @@ agent↔Nous wire contract lives in `docs/chronos-managed-cron-contract.md`. Each cron job runs in a completely fresh agent session: - No conversation history from previous runs -- No memory of previous cron executions (unless persisted to memory/files) +- No memory of previous cron executions (persistent memory — MEMORY.md / + USER.md — does load, like any other agent run, so durable preferences and + facts carry over; per-run conversation context does not) - The prompt must be self-contained — cron jobs cannot ask clarifying questions - The `cronjob` toolset is disabled (recursion guard) diff --git a/website/docs/guides/automate-with-cron.md b/website/docs/guides/automate-with-cron.md index dec05e43fd..a1a787fe88 100644 --- a/website/docs/guides/automate-with-cron.md +++ b/website/docs/guides/automate-with-cron.md @@ -123,7 +123,7 @@ Otherwise, provide a concise summary of the activity." --name "Repo watcher" --d ``` :::warning Self-Contained Prompts -Notice how the prompt includes the exact `gh` commands. The cron agent has no memory of previous runs or your preferences — spell everything out. +Notice how the prompt includes the exact `gh` commands. The cron agent has no conversation history from previous runs — spell everything out. (Persistent memory does load, so durable preferences saved to MEMORY.md carry over, but don't rely on it for job-critical details.) ::: --- From f29ee96dd3b5bc4cd7a33ff16d69a1b4c911d344 Mon Sep 17 00:00:00 2001 From: PT Date: Tue, 28 Jul 2026 11:24:15 -0700 Subject: [PATCH 16/37] fix(update): restart all macOS launchd gateways on hermes update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The macOS branch of the update's fleet-restart step only restarted the invoking profile's LaunchAgent. Sibling ai.hermes.gateway- services kept pre-update modules cached in sys.modules and died on their next agent turn (ImportError on new lazy imports, or TypeError/ AttributeError with garbled tracebacks on wider version gaps). The systemd branch already iterates every hermes-gateway* unit; this brings launchd to parity: - _restart_macos_launchd_gateways(): the invoking profile keeps the existing launchd_restart() path; every other gateway of this install is drained via SIGUSR1 (same as systemd siblings), then hard- kickstarted unless KeepAlive already respawned it, then verified on a fresh PID. TimeoutExpired is isolated per label (#68523 parity) and counts toward failed_or_stale_units — including timeouts during liveness discovery, which must not read as "unloaded". - Install-scoped fleet enumeration: launchd_gateway_labels_for_install() derives labels from THIS install's profiles (get_default_hermes_root), not by globbing the shared per-user ~/Library/LaunchAgents — a sandboxed HERMES_HOME (tests, capture sandboxes, side-by-side installs) must never enumerate, let alone restart, another install's fleet. This also keeps the hermetic test suite blind to a dev machine's real gateways. - Domain-explicit sibling handling via _locate_launchd_gateway_service(): liveness, kickstart, and fresh-PID verification all use the domain the service was actually located in (gui/ vs user/ probed per label via `launchctl print`). This addresses the #41403 review defect: the process-wide _launchd_domain() cache resolves the current profile's domain and must never be reused for a sibling. _launchd_domain() itself becomes a thin caching wrapper; behavior unchanged. - _get_service_pids(all_profiles=...): the update path's manual-process sweep excludes every gateway service PID (mirror of the systemd hermes-gateway* pattern) so it cannot mistake a freshly respawned sibling service for a stale manual gateway. Default-scope callers (gateway status, cron checks, stop_profile_gateway's orphan reaper — which kills what it is fed) keep the current-profile-only contract. - _warn_incomplete_gateway_fleet_restart() prints launchctl recovery hints for launchd labels alongside the systemctl ones. Supersedes and completes #41403, addressing its review feedback (per-label domain resolution + mocked regression tests). Co-authored-by: David Neyra Co-Authored-By: Claude Fable 5 --- hermes_cli/gateway.py | 292 +++++--- hermes_cli/update_cmd.py | 148 +++- .../test_update_launchd_fleet_restart.py | 642 ++++++++++++++++++ 3 files changed, 983 insertions(+), 99 deletions(-) create mode 100644 tests/hermes_cli/test_update_launchd_fleet_restart.py diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index c64080ab0c..dced8278a5 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -107,11 +107,14 @@ def _get_service_pids(all_profiles: bool = False) -> set: returns (true for both systemd and launchd in practice). ``all_profiles`` widens the launchd branch to every installed - ``ai.hermes.gateway*`` agent — the update path needs the whole fleet - excluded from its sweep so sibling-profile launchd gateways found by the - ps scan aren't misclassified as manual processes (#73626). Default-scope - callers (``gateway status``, cron checks) keep seeing only the current - profile's service. + ``ai.hermes.gateway*`` LaunchAgent — the update path needs the whole + fleet excluded from its sweep (#41403, #73626): sibling-profile launchd + gateways found by the (BSD-fixed) ps scan must not be misclassified as + manual processes and killed. Default-scope callers (``gateway status``, + cron checks) keep seeing only the current profile's service; the orphan + reaper passes all_profiles=True for the same friendly-fire reason. The + systemd branch has always been fleet-wide (``hermes-gateway*``) and is + unaffected. """ pids: set = set() @@ -155,13 +158,29 @@ def _get_service_pids(all_profiles: bool = False) -> set: # --- launchd (macOS) --- if is_macos(): - try: - if all_profiles: - # Enumerate every ai.hermes.gateway* agent across profiles - # so the update sweep's exclude set is complete (#73626). - # Without this, sibling-profile launchd gateways found by the - # (now-working) ps scan would be misclassified as manual and - # killed, racing with KeepAlive → duplicate gateways. + labels = {get_launchd_label()} + if all_profiles: + # Every gateway LaunchAgent, not just the invoking profile's — + # mirrors the systemd branch's ``hermes-gateway*`` pattern above. + # The update path restarts the whole fleet, and its stale-process + # sweep must not mistake a sibling service's fresh PID for a + # manual gateway it should kill (#41403). + labels.update(launchd_gateway_labels_for_install()) + for label in sorted(labels): + try: + _domain, pid = _locate_launchd_gateway_service(label) + except subprocess.TimeoutExpired: + continue + if pid is not None and pid > 0: + pids.add(pid) + if all_profiles: + # Belt-and-suspenders for the EXCLUDE use case (#74075): a bare + # ``launchctl list`` prefix scan also catches ai.hermes.gateway* + # agents the label derivation can't map (renamed profiles, other + # installs sharing this user). Over-inclusion is safe here — + # these PIDs are only ever protected from the kill sweep, never + # targeted. Restart paths use the label-derived set only. + try: result = subprocess.run( ["launchctl", "list"], capture_output=True, @@ -180,33 +199,8 @@ def _get_service_pids(all_profiles: bool = False) -> set: pids.add(pid) except ValueError: pass - else: - label = get_launchd_label() - result = subprocess.run( - ["launchctl", "list", label], - capture_output=True, - text=True, encoding='utf-8', errors='replace', - timeout=5, - ) - if result.returncode == 0: - # Try plist format first (macOS 26+): "PID" = ; - pid = _parse_launchd_pid_from_list_output(result.stdout) - if pid is not None and pid > 0: - pids.add(pid) - else: - # Fall back to legacy tab-separated format: - # "PID\tStatus\tLabel" - for line in result.stdout.strip().splitlines(): - parts = line.split() - if len(parts) >= 3 and parts[2] == label: - try: - pid = int(parts[0]) - if pid > 0: - pids.add(pid) - except ValueError: - pass - except (FileNotFoundError, subprocess.TimeoutExpired): - pass + except (FileNotFoundError, subprocess.TimeoutExpired): + pass return pids @@ -1437,6 +1431,87 @@ def _parse_launchd_pid_from_list_output(output: str) -> int | None: return None +def _parse_launchd_pid_from_print_output(output: str) -> int | None: + """Extract the live PID from ``launchctl print`` output (``pid = ``). + + A bootstrapped-but-not-running service prints no ``pid =`` line; the + first (service-level) occurrence wins over any nested endpoint state. + Returns ``None`` when no PID is found or the PID is non-positive. + """ + for line in output.splitlines(): + stripped = line.strip() + if stripped.startswith("pid = "): + try: + pid = int(stripped[len("pid = "):].strip()) + return pid if pid > 0 else None + except ValueError: + return None + return None + + +def _launchd_print_service_pid(domain: str, label: str) -> tuple[bool, int | None]: + """Return ``(loaded, pid)`` for ``domain/label`` via ``launchctl print``. + + Domain-explicit on purpose: legacy ``launchctl list`` infers its domain + from the caller's execution context, which is exactly the ambiguity that + sank the first fleet-restart attempt (#41403 review). ``TimeoutExpired`` + propagates — fleet-restart callers own per-label failure accounting (a + wedged launchctl call must be reported, not read as "unloaded"). + """ + try: + result = subprocess.run( + ["launchctl", "print", f"{domain}/{label}"], + capture_output=True, + text=True, encoding='utf-8', errors='replace', + timeout=5, + ) + except FileNotFoundError: + return (False, None) + if result.returncode != 0: + return (False, None) + return (True, _parse_launchd_pid_from_print_output(result.stdout)) + + +def _launchd_service_registered(label: str) -> bool: + """True when launchd knows ``label`` (``launchctl list