fix(config): make live settings reload safe

2026-08-17 21:56:21 -04:00
parent ced78801ba
commit e3475e6353
3 changed files with 219 additions and 66 deletions
@@ -2,6 +2,10 @@

 ## [Unreleased]

+### Fixed
+
+- Fixed task and eval subagents discovering newly added agent definitions while resolving their role aliases from stale startup settings. Subagent preflight now atomically reloads persisted settings before agent discovery while preserving live runtime overrides.
+
 ## [17.3.7] - 2026-08-17

 ### Changed
@@ -89,9 +93,6 @@
 - Fixed `hub jobs` and empty `hub wait` snapshots hiding running subagents that have no live turn, which removed the only way to discover and `hub cancel` a stale registration; such agents are listed again and flagged as having no turn in flight.
 - Fixed external thinking being offered on xAI reasoning-only Responses models (grok-4 family) that reject `reasoning.effort`, where the private scratchpad ran alongside native reasoning instead of replacing it.
 - Fixed the extension tool-call handler timeout rendering outside a titled section in `/settings` by registering its Extensions group on the Tools tab.
-### Fixed
-
- Fixed task and eval subagents discovering newly added agent definitions while resolving their role aliases from stale startup settings. Subagent preflight now atomically reloads persisted settings before agent discovery while preserving live runtime overrides.

 ## [17.3.4] - 2026-08-14

@@ -71,6 +71,22 @@ type YamlLoadResult =
 	| { kind: "invalid"; error: unknown; backupPath?: string }
 	| { kind: "unreadable"; error: unknown };

+type MainYamlReadResult = {
+	settings: RawSettings | null;
+	configPath: string | null;
+};
+
+type ProjectSettingsReadResult = {
+	settings: RawSettings;
+	fileSettings: RawSettings;
+	shellPathSource: string | undefined;
+};
+
+type ConfigOverlayReadResult = {
+	settings: RawSettings;
+	shellPathSource: string | undefined;
+};
+
 export interface SettingsOptions {
 	/** Current working directory for project settings discovery */
 	cwd?: string;
@@ -353,6 +369,8 @@ export class Settings {
 	#modifiedProjectModelRoles = new Set<string>();
 	/** Individual global model roles modified during this session (for partial save) */
 	#modifiedGlobalModelRoles = new Set<string>();
+	/** Changes whenever a live API mutates a persisted layer. */
+	#persistedMutationGeneration = 0;
 	/**
 	 * Original process-wide model-role overrides captured before a project edit
 	 * temporarily replaced them via `#updateRuntimeModelRoleOverride`. Restored
@@ -505,6 +523,7 @@ export class Settings {
 		const prev = this.get(path);
 		const segments = path.split(".");
 		setByPath(this.#global, segments, value);
+		this.#persistedMutationGeneration++;
 		this.#modified.add(path);
 		this.#rebuildMerged();
 		const next = this.get(path);
@@ -650,40 +669,56 @@ export class Settings {
 	}

 	async #reloadPersistedLayers(): Promise<void> {
-		await this.flush();
-		const previousSignaledValues = {
-			modelRoles: this.get("modelRoles"),
-			sessionAccent: this.get("statusLine.sessionAccent"),
-		};
-		const previousHookValues = new Map<SettingPath, unknown>();
-		for (const key of Object.keys(SETTING_HOOKS) as SettingPath[]) {
-			previousHookValues.set(key, this.get(key));
-		}
-
-		const [globalResult, projectResult, overlayResult] = await Promise.allSettled([
-			this.#loadExistingMainYaml(),
-			this.#loadProjectSettings(),
-			this.#loadConfigOverlays(),
-		]);
-		if (globalResult.status === "rejected") throw globalResult.reason;
-		if (projectResult.status === "rejected") throw projectResult.reason;
-		if (overlayResult.status === "rejected") throw overlayResult.reason;
-
-		this.#global = globalResult.value ?? {};
-		this.#project = projectResult.value;
-		this.#configOverlay = overlayResult.value;
-		this.#rebuildMerged();
-		this.#fireEffectiveSettingChanged("modelRoles", this.get("modelRoles"), previousSignaledValues.modelRoles);
-		this.#fireEffectiveSettingChanged(
-			"statusLine.sessionAccent",
-			this.get("statusLine.sessionAccent"),
-			previousSignaledValues.sessionAccent,
-		);
-		for (const [key, previous] of previousHookValues) {
-			const next = this.get(key);
-			if (!Object.is(next, previous)) {
-				SETTING_HOOKS[key]?.(next, previous);
+		for (;;) {
+			await this.flush();
+			const mutationGeneration = this.#persistedMutationGeneration;
+			const previousSignaledValues = {
+				modelRoles: this.get("modelRoles"),
+				sessionAccent: this.get("statusLine.sessionAccent"),
+			};
+			const previousHookValues = new Map<SettingPath, unknown>();
+			for (const key of Object.keys(SETTING_HOOKS) as SettingPath[]) {
+				previousHookValues.set(key, this.get(key));
 			}
+
+			const [globalResult, projectResult, overlayResult] = await Promise.allSettled([
+				this.#readExistingMainYaml(false),
+				this.#readProjectSettings(false),
+				this.#readConfigOverlays(false),
+			]);
+			if (mutationGeneration !== this.#persistedMutationGeneration) continue;
+			if (globalResult.status === "rejected") throw globalResult.reason;
+			if (projectResult.status === "rejected") throw projectResult.reason;
+			if (overlayResult.status === "rejected") throw overlayResult.reason;
+
+			this.#configPath = globalResult.value.configPath;
+			this.#global = globalResult.value.settings ?? {};
+			this.#project = projectResult.value.settings;
+			this.#projectFileSettings = projectResult.value.fileSettings;
+			this.#projectShellPathSource = projectResult.value.shellPathSource;
+			this.#configOverlay = overlayResult.value.settings;
+			this.#overlayShellPathSource = overlayResult.value.shellPathSource;
+			this.#rebuildMerged();
+
+			const nextModelRoles = this.get("modelRoles");
+			if (!Bun.deepEquals(nextModelRoles, previousSignaledValues.modelRoles)) {
+				this.#fireEffectiveSettingChanged("modelRoles", nextModelRoles, previousSignaledValues.modelRoles);
+			}
+			const nextSessionAccent = this.get("statusLine.sessionAccent");
+			if (!Bun.deepEquals(nextSessionAccent, previousSignaledValues.sessionAccent)) {
+				this.#fireEffectiveSettingChanged(
+					"statusLine.sessionAccent",
+					nextSessionAccent,
+					previousSignaledValues.sessionAccent,
+				);
+			}
+			for (const [key, previous] of previousHookValues) {
+				const next = this.get(key);
+				if (!Bun.deepEquals(next, previous)) {
+					SETTING_HOOKS[key]?.(next, previous);
+				}
+			}
+			return;
 		}
 	}

@@ -933,6 +968,7 @@ export class Settings {
 		current[role] = modelId;
 		setByPath(this.#project, ["modelRoles"], current);
 		this.#modifiedProjectModelRoles.add(role);
+		this.#persistedMutationGeneration++;
 		this.#rebuildMerged();
 		this.#fireEffectiveSettingChanged("modelRoles", this.get("modelRoles"), prev);
 		this.#queueProjectSave();
@@ -966,6 +1002,7 @@ export class Settings {
 		// clobbered by this process's stale in-memory snapshot.
 		setByPath(this.#global, ["modelRoles"], current);
 		this.#modifiedGlobalModelRoles.add(role);
+		this.#persistedMutationGeneration++;
 		this.#rebuildMerged();
 		this.#queueSave();
 		this.#fireEffectiveSettingChanged("modelRoles", this.get("modelRoles"), prev);
@@ -1154,7 +1191,7 @@ export class Settings {
 		return loaded ?? {};
 	}

-	async #loadYamlIfPresent(filePath: string): Promise<YamlLoadResult> {
+	async #loadYamlIfPresent(filePath: string, captureLegacyChangelogVersion = true): Promise<YamlLoadResult> {
 		let content: string;
 		try {
 			content = await fs.promises.readFile(filePath, "utf8");
@@ -1178,7 +1215,10 @@ export class Settings {
 				error: new Error("Settings YAML must contain a mapping at the document root"),
 			};
 		}
-		return { kind: "loaded", settings: this.#migrateRawSettings(parsed as RawSettings) };
+		return {
+			kind: "loaded",
+			settings: this.#migrateRawSettings(parsed as RawSettings, captureLegacyChangelogVersion),
+		};
 	}

 	async #resolveYamlWritePath(filePath: string): Promise<string> {
@@ -1278,55 +1318,81 @@ export class Settings {
 		}
 	}

-	async #loadExistingMainYaml(): Promise<RawSettings | null> {
-		if (!this.#configPath) return null;
+	async #readExistingMainYaml(quarantineInvalid: boolean): Promise<MainYamlReadResult> {
+		if (!this.#configPath) return { settings: null, configPath: null };
 		for (const filename of MAIN_CONFIG_FILENAMES) {
 			const configPath = path.join(this.#agentDir, filename);
-			const loaded = await this.#loadYamlIfPresentForStartup(configPath);
-			if (loaded) {
-				this.#configPath = configPath;
-				return loaded;
-			}
+			const loaded = quarantineInvalid
+				? await this.#loadYamlIfPresentForStartup(configPath)
+				: this.#unwrapYamlLoadResult(configPath, await this.#loadYamlIfPresent(configPath, false));
+			if (loaded) return { settings: loaded, configPath };
 		}
-		this.#configPath = path.join(this.#agentDir, MAIN_CONFIG_FILENAMES[0]);
-		return null;
+		return {
+			settings: null,
+			configPath: path.join(this.#agentDir, MAIN_CONFIG_FILENAMES[0]),
+		};
 	}

-	async #loadProjectSettings(): Promise<RawSettings> {
-		this.#projectShellPathSource = undefined;
+	async #loadExistingMainYaml(): Promise<RawSettings | null> {
+		const result = await this.#readExistingMainYaml(true);
+		this.#configPath = result.configPath;
+		return result.settings;
+	}
+
+	async #readProjectSettings(quarantineInvalid: boolean): Promise<ProjectSettingsReadResult> {
+		let shellPathSource: string | undefined;
 		let merged: RawSettings = {};
 		try {
 			const result = await loadCapability(settingsCapability.id, { cwd: this.#cwd });
 			for (const item of result.items as SettingsCapabilityItem[]) {
 				if (item.level === "project") {
 					merged = this.#deepMerge(merged, item.data as RawSettings);
-					if (Object.hasOwn(item.data, "shellPath")) this.#projectShellPathSource = item.path;
+					if (Object.hasOwn(item.data, "shellPath")) shellPathSource = item.path;
 				}
 			}
 		} catch {
-			this.#projectShellPathSource = undefined;
+			shellPathSource = undefined;
 			// Capability discovery is best-effort; the native project config below
 			// remains authoritative for its model-role layer and must not be hidden.
 		}
 		const projectConfigPath = path.join(this.#cwd, ".omp", "config.yml");
-		const nativeProject = await this.#loadYaml(projectConfigPath);
-		this.#projectFileSettings = structuredClone(nativeProject);
+		const nativeProject = quarantineInvalid
+			? await this.#loadYaml(projectConfigPath)
+			: (this.#unwrapYamlLoadResult(projectConfigPath, await this.#loadYamlIfPresent(projectConfigPath, false)) ??
+				{});
 		const nativeModelRoles = getByPath(nativeProject, ["modelRoles"]);
 		if (nativeModelRoles !== undefined) {
 			merged = this.#deepMerge(merged, { modelRoles: nativeModelRoles });
 		}
-		return this.#migrateRawSettings(merged);
+		return {
+			settings: this.#migrateRawSettings(merged, quarantineInvalid),
+			fileSettings: structuredClone(nativeProject),
+			shellPathSource,
+		};
+	}
+
+	async #loadProjectSettings(): Promise<RawSettings> {
+		const result = await this.#readProjectSettings(true);
+		this.#projectFileSettings = result.fileSettings;
+		this.#projectShellPathSource = result.shellPathSource;
+		return result.settings;
+	}
+
+	async #readConfigOverlays(captureLegacyChangelogVersion = true): Promise<ConfigOverlayReadResult> {
+		let shellPathSource: string | undefined;
+		let settings: RawSettings = {};
+		for (const filePath of this.#configFiles) {
+			const overlay = await this.#loadOverlayYaml(filePath, captureLegacyChangelogVersion);
+			settings = this.#deepMerge(settings, overlay);
+			if (Object.hasOwn(overlay, "shellPath")) shellPathSource = filePath;
+		}
+		return { settings, shellPathSource };
 	}

 	async #loadConfigOverlays(): Promise<RawSettings> {
-		this.#overlayShellPathSource = undefined;
-		let merged: RawSettings = {};
-		for (const filePath of this.#configFiles) {
-			const overlay = await this.#loadOverlayYaml(filePath);
-			merged = this.#deepMerge(merged, overlay);
-			if (Object.hasOwn(overlay, "shellPath")) this.#overlayShellPathSource = filePath;
-		}
-		return merged;
+		const result = await this.#readConfigOverlays();
+		this.#overlayShellPathSource = result.shellPathSource;
+		return result.settings;
 	}

 	/**
@@ -1334,7 +1400,7 @@ export class Settings {
 	 * missing or malformed files are hard errors so a typo'd path cannot
 	 * silently fall back to the persistent settings.
 	 */
-	async #loadOverlayYaml(filePath: string): Promise<RawSettings> {
+	async #loadOverlayYaml(filePath: string, captureLegacyChangelogVersion = true): Promise<RawSettings> {
 		let content: string;
 		try {
 			content = await Bun.file(filePath).text();
@@ -1355,7 +1421,7 @@ export class Settings {
 		if (typeof parsed !== "object" || Array.isArray(parsed)) {
 			throw new Error(`Config overlay must be a YAML mapping: ${filePath}`);
 		}
-		return this.#migrateRawSettings(parsed as RawSettings);
+		return this.#migrateRawSettings(parsed as RawSettings, captureLegacyChangelogVersion);
 	}

 	async #migrateFromLegacy(): Promise<void> {
@@ -1396,7 +1462,7 @@ export class Settings {
 	}

 	/** Apply schema migrations to raw settings */
-	#migrateRawSettings(raw: RawSettings): RawSettings {
+	#migrateRawSettings(raw: RawSettings, captureLegacyChangelogVersion = true): RawSettings {
 		// queueMode -> steeringMode
 		if ("queueMode" in raw && !("steeringMode" in raw)) {
 			raw.steeringMode = raw.queueMode;
@@ -1408,7 +1474,7 @@ export class Settings {
 		// longer dirty user-tracked configs. Capture for marker seeding (see
 		// #seedLastChangelogVersionMarker), then strip the key — the next
 		// config save drops it from disk.
-		if (typeof raw.lastChangelogVersion === "string") {
+		if (captureLegacyChangelogVersion && typeof raw.lastChangelogVersion === "string") {
 			this.#legacyLastChangelogVersion ??= raw.lastChangelogVersion;
 		}
 		delete raw.lastChangelogVersion;
@@ -8,11 +8,13 @@ import { __providerInFlightForTesting, streamSimple } from "@oh-my-pi/pi-ai/stre
 import type { Context } from "@oh-my-pi/pi-ai/types";
 import {
 	onAppendOnlyModeChanged,
+	onModelRolesChanged,
 	onStatusLineSessionAccentChanged,
 	resetSettingsForTest,
 	type SettingPath,
 	Settings,
 } from "@oh-my-pi/pi-coding-agent/config/settings";
+import * as discovery from "@oh-my-pi/pi-coding-agent/discovery";
 import { AgentStorage } from "@oh-my-pi/pi-coding-agent/session/agent-storage";
 import { AUTO_IMAGE_PROVIDER_ORDER } from "@oh-my-pi/pi-coding-agent/tools/image-providers";
 import { SEARCH_PROVIDER_ORDER } from "@oh-my-pi/pi-coding-agent/web/search/types";
@@ -389,6 +391,90 @@ describe("Settings", () => {
 		});
 	});

+	describe("live persisted reload", () => {
+		it("rejects malformed live configs without moving them aside or replacing effective settings", async () => {
+			const projectConfigPath = path.join(projectDir, ".omp", "config.yml");
+			await writeSettings({
+				setupVersion: 1,
+				modelRoles: { global_role: "openai/global" },
+			});
+			await Bun.write(
+				projectConfigPath,
+				YAML.stringify({ modelRoles: { project_role: "openai/project" } }, null, 2),
+			);
+			const settings = await Settings.init({ cwd: projectDir, agentDir });
+			const malformedGlobal = 'setupVersion: 2\nmodelRoles:\n  global_role: "unterminated\n';
+			const malformedProject = 'modelRoles:\n  project_role: "unterminated\n';
+			await Promise.all([
+				Bun.write(getConfigPath(), malformedGlobal),
+				Bun.write(projectConfigPath, malformedProject),
+			]);
+
+			await expect(settings.reloadFromDisk()).rejects.toThrow("Settings config is invalid");
+
+			expect(await Bun.file(getConfigPath()).text()).toBe(malformedGlobal);
+			expect(await Bun.file(projectConfigPath).text()).toBe(malformedProject);
+			expect(fs.readdirSync(agentDir).some(name => name.startsWith("config.yml.broken-"))).toBe(false);
+			expect(
+				fs.readdirSync(path.dirname(projectConfigPath)).some(name => name.startsWith("config.yml.broken-")),
+			).toBe(false);
+			expect(settings.get("setupVersion")).toBe(1);
+			expect(settings.getModelRole("global_role")).toBe("openai/global");
+			expect(settings.getModelRole("project_role")).toBe("openai/project");
+		});
+		it("retries when a persisted setting changes while files are being read", async () => {
+			await writeSettings({ setupVersion: 1 });
+			const settings = await Settings.init({ cwd: projectDir, agentDir });
+			const loadCapability = discovery.loadCapability;
+			const projectLoadStarted = Promise.withResolvers<void>();
+			const releaseProjectLoad = Promise.withResolvers<void>();
+			let pauseProjectLoad = true;
+			vi.spyOn(discovery, "loadCapability").mockImplementation(async (id, options) => {
+				if (pauseProjectLoad) {
+					pauseProjectLoad = false;
+					projectLoadStarted.resolve();
+					await releaseProjectLoad.promise;
+				}
+				return await loadCapability(id, options);
+			});
+
+			const reload = settings.reloadFromDisk();
+			await projectLoadStarted.promise;
+			settings.set("setupVersion", 2);
+			releaseProjectLoad.resolve();
+			await reload;
+			await settings.flush();
+
+			expect(settings.get("setupVersion")).toBe(2);
+			expect((await readSettings()).setupVersion).toBe(2);
+		});
+
+		it("preserves runtime overrides and only signals semantic model-role changes", async () => {
+			await writeSettings({ modelRoles: { default: "openai/original" } });
+			const settings = await Settings.init({ cwd: projectDir, agentDir });
+			settings.overrideModelRoles({ runtime: "openai/runtime" });
+			let signalCount = 0;
+			const unsubscribe = onModelRolesChanged(() => {
+				signalCount++;
+			});
+
+			try {
+				await settings.reloadFromDisk();
+				expect(signalCount).toBe(0);
+				expect(settings.getModelRole("runtime")).toBe("openai/runtime");
+
+				await writeSettings({ modelRoles: { default: "openai/updated" } });
+				await settings.reloadFromDisk();
+
+				expect(signalCount).toBe(1);
+				expect(settings.getModelRole("default")).toBe("openai/updated");
+				expect(settings.getModelRole("runtime")).toBe("openai/runtime");
+			} finally {
+				unsubscribe();
+			}
+		});
+	});
+
 	describe("get()", () => {
 		it("resolves overrides, schema defaults, and falsey values", () => {
 			const isolated = Settings.isolated({