fix(task): key subagent fallback chains off the pre-expansion model role

A single-model subagent is pinned to a `subagent:<id>` role whose `retry.fallbackChains` entry shadows every configured role chain, so the chain it inherits decides where the child retries. Inheritance resolved the role by re-deriving it from the child's `modelPatterns` — but every spawn path expands the role alias into `modelOverride` before calling `runSubprocess` (`modelPatterns = normalizeModelPatterns(modelOverride ?? agent.model)`), so `@task` never reached the derivation and it returned `undefined` every time. Every task subagent inherited `chains.default`. With `modelRoles.task: anthropic/claude-sonnet-5`, `task` chained to sonnet alone, and `default` chained to sonnet plus a second provider, a transient stall on sonnet routed the child onto the default chain's second model — one the operator had deliberately kept out of the `task` chain — and a quota error there killed a 28-minute run. #7694 fixed only the shape where an unexpanded alias reaches the executor, which no production caller produces; its tests supplied a bare `agent.model: ["@smol"]` with no `modelOverride`. The incident above happened on v17.2.10, which contains that fix. Route inheritance off the role identity the spawn path already computes and passes as `modelRole`. Since that leaves the pattern-derived operand unreachable, drop it and the parameter it was the only user of. The vibe worker path had the same defect independently: `#resolveWorker` expanded `@task`/`@smol` for the bundled `task`/`sonic` workers and kept no role, so vibe children inherited `default` no matter what the executor did. It now carries `modelRole` on `ResolvedVibeWorker` and `VibeRecord` through both the spawn and rehydrate sites. To stop the two halves drifting apart again — the mistake that caused this bug — `resolveAgentModelSelection` returns the expanded `patterns` and the pre-expansion `role` from one call, and both spawn paths take both from it. `resolveAgentModelSource` is removed: its only use was being fed to `resolveExplicitModelRole`, and keeping it invites the same split derivation. `resolveAgentModelPatterns` stays for the UI callers that legitimately want patterns alone. Tests cover the producible shapes: the incident's chain layout (role chain equal to the primary, default chain a superset), role identity surviving expansion for every alias-routed bundled agent, and the patterns/role pairing itself. #7694's two tests are re-anchored to a shape a real caller produces.
2026-08-07 21:14:17 +08:00
parent ab78d3091e
commit 77ee3f2e7e
8 changed files with 184 additions and 58 deletions
@@ -8,6 +8,14 @@
 - Remote MCP transports now enforce header precedence and origin policy: client-generated HTTP/MCP/authorization headers win over configured headers case-insensitively, and Agent Plugins servers never forward configured headers across a redirect to a different origin (method-changing redirects of JSON-RPC POSTs are refused). Agent Plugins stdio `env` values and remote `headers` are likewise exempt from config-value resolution (no ambient env-name lookup, no `!command` execution, empty values preserved).
 - Added `omp share <session>`: share a saved session by id prefix or `.jsonl` path without launching the agent — same encrypted upload, store selection, and `share.redactSecrets` handling as the `/share` slash command.

+### Fixed
+
+- Subagents spawned through a model-role alias now inherit that role's `retry.fallbackChains` entry instead of the `default` chain. Both spawn paths (`task` and vibe workers) expand the alias — the bundled `task` agent's `@task`, `sonic`/`scout`'s `@smol` — before it reaches the executor, so the role identity was lost and every child was pinned to the `default` chain, routing retries onto models the operator had deliberately kept out of the role's chain. Completes [#7694](https://github.com/can1357/oh-my-pi/pull/7694), which only covered agents whose unexpanded alias reached the executor ([#7910](https://github.com/can1357/oh-my-pi/pull/7910) by [@enieuwy](https://github.com/enieuwy)).
+
+### Removed
+
+- Removed the `resolveAgentModelSource` model-resolver export, whose only use was being fed to `resolveExplicitModelRole`. Replaced by `resolveAgentModelSelection`, which returns the expanded `patterns` and the pre-expansion `role` together so a spawn path cannot derive one without the other ([#7910](https://github.com/can1357/oh-my-pi/pull/7910) by [@enieuwy](https://github.com/enieuwy)).
+
 ## [17.2.10] - 2026-08-06

 ### Breaking Changes
@@ -1138,14 +1138,30 @@ function resolveEffectiveAgentModelSelection(
 	return { patterns: resolveConfiguredModelPatterns(fallback, settings) };
 }

-/** Return the raw selector source that supplies the effective agent patterns. */
-export function resolveAgentModelSource(options: AgentModelPatternResolutionOptions): string | string[] | undefined {
-	return resolveEffectiveAgentModelSelection(options).source;
+/** Effective agent model patterns paired with the pre-expansion role alias behind them. */
+export interface AgentModelSelection {
+	/** Expanded model patterns to spawn with. */
+	patterns: string[];
+	/** Role alias the patterns came from (`@task` -> `task`), when the source named one. */
+	role: string | undefined;
 }

+/**
+ * Resolve an agent's model patterns together with the role identity they were
+ * expanded from. Spawn paths MUST take both from this single call: the child's
+ * inherited retry-fallback chain is keyed off the role, which the expansion
+ * discards, and deriving the two halves separately is how they drift apart.
+ */
+export function resolveAgentModelSelection(options: AgentModelPatternResolutionOptions): AgentModelSelection {
+	const { source, patterns } = resolveEffectiveAgentModelSelection(options);
+	return { patterns, role: resolveExplicitModelRole(source, options.settings) };
+}
+
+/** Effective agent model patterns alone, for callers with no interest in role identity. */
 export function resolveAgentModelPatterns(options: AgentModelPatternResolutionOptions): string[] {
 	return resolveEffectiveAgentModelSelection(options).patterns;
 }
+
 /** Default prewalk hand-off target when no explicit target is configured. */
 export const DEFAULT_PREWALK_TARGET = "@smol";

@@ -189,19 +189,25 @@ function resolveSubagentRetryFallbackCandidates(
 * Chain a single-model subagent inherits when its own model patterns supply no
 * fallbacks of their own. The child is pinned to a `subagent:<id>` role whose
 * chain shadows every configured role chain (see
- * {@link installSubagentRetryFallbackChain}), so a role-alias request (`@smol`)
- * MUST inherit that role's chain — otherwise the pin silently re-routes the
- * child onto the `default` role's chain. Explicit model selectors keep
- * inheriting `default`: they carry no role identity, and a role that happens to
- * be assigned the same model must not capture the child's fallback routing.
+ * {@link installSubagentRetryFallbackChain}), so a role-alias request (`@smol`,
+ * the bundled `task` agent's `@task`) MUST inherit that role's chain —
+ * otherwise the pin silently re-routes the child onto the `default` role's
+ * chain. Explicit model selectors keep inheriting `default`: they carry no role
+ * identity, and a role that happens to be assigned the same model must not
+ * capture the child's fallback routing.
+ *
+ * `modelRole` is the sole witness of that identity. Callers hand
+ * `runSubprocess` the model patterns already expanded by
+ * `resolveAgentModelSelection`, so `@task` never survives into them — the role
+ * has to travel beside the patterns, and re-deriving it from them yields
+ * `undefined` every time.
 */
 function resolveSubagentInheritedRetryFallbackChain(
 	settings: Settings,
 	modelRegistry: ModelRegistry,
-	modelPatterns: string[],
+	role: string | undefined,
 ): string[] | undefined {
 	const configuredChains = settings.get("retry.fallbackChains");
-	const role = resolveExplicitModelRole(modelPatterns, settings);
 	// An explicitly emptied role chain means "no fallbacks", not "inherit
 	// default" — mirrors expandDefaultRetryFallbackChains.
 	const fallbackChain = (role !== undefined ? configuredChains?.[role] : undefined) ?? configuredChains?.default;
@@ -2835,7 +2841,7 @@ export async function runSubprocess(options: ExecutorOptions): Promise<SingleRes
 			const configuredModelPatterns = resolveConfiguredModelPatterns(modelPatterns, settings);
 			const inheritedRetryFallbackChain =
 				configuredModelPatterns.length === 1
-					? resolveSubagentInheritedRetryFallbackChain(subagentSettings, modelRegistry, modelPatterns)
+					? resolveSubagentInheritedRetryFallbackChain(subagentSettings, modelRegistry, modelRole)
 					: undefined;
 			const {
 				model,
@@ -8,7 +8,7 @@ import * as fs from "node:fs/promises";
 import * as os from "node:os";
 import path from "node:path";
 import { $env, prompt, Snowflake } from "@oh-my-pi/pi-utils";
-import { resolveAgentModelPatterns, resolveAgentModelSource, resolveExplicitModelRole } from "../config/model-resolver";
+import { resolveAgentModelSelection } from "../config/model-resolver";
 import type { LocalProtocolOptions } from "../internal-urls";
 import { registerArtifactsDir } from "../internal-urls/registry-helpers";
 import { MCPManager } from "../mcp/manager";
@@ -288,10 +288,10 @@ export async function resolveEffectiveSubagentPolicy(
 		activeModelPattern: parentActiveModelPattern,
 		fallbackModelPattern: request.session.getModelString?.(),
 	};
-	// Keep role identity from the same effective non-empty source that supplies
-	// model selection: caller request, settings override, then agent definition.
-	const modelRole = resolveExplicitModelRole(resolveAgentModelSource(modelResolution), request.session.settings);
-	const modelOverride = resolveAgentModelPatterns(modelResolution);
+	// Role identity and patterns come from one call so they cannot be derived
+	// from different sources: the expansion below discards the alias, and the
+	// child's inherited retry-fallback chain is keyed off the role.
+	const { patterns: modelOverride, role: modelRole } = resolveAgentModelSelection(modelResolution);
 	const isolationMode = request.session.settings.get("task.isolation.mode");
 	const isIsolated = request.isolation?.requested === true;
 	if (isIsolated && isolationMode === "none") {
@@ -18,7 +18,7 @@ import * as os from "node:os";
 import * as path from "node:path";
 import { logger, prompt, Snowflake } from "@oh-my-pi/pi-utils";
 import type { AsyncJob, AsyncJobManager } from "../async/job-manager";
-import { resolveAgentModelPatterns } from "../config/model-resolver";
+import { resolveAgentModelSelection } from "../config/model-resolver";
 import type { LocalProtocolOptions } from "../internal-urls";
 import { registerArtifactsDir } from "../internal-urls/registry-helpers";
 import { MCPManager } from "../mcp/manager";
@@ -43,7 +43,7 @@ export type VibeCli = "fast" | "good";
 * CLI flavor → bundled agent type. This IS the model-tier mapping: `sonic`
 * carries `model: "@smol"` (the configured fast/low-latency role) and `task`
 * carries `model: "@task"` (inherits the session's strong model).
- * Resolution goes through {@link resolveAgentModelPatterns} exactly like a
+ * Resolution goes through {@link resolveAgentModelSelection} exactly like a
 * `task` spawn, so `task.agentModelOverrides` and model-role settings apply.
 */
 export const VIBE_CLI_AGENT: Record<VibeCli, string> = {
@@ -144,6 +144,8 @@ interface VibeRestoreCandidate {
 interface ResolvedVibeWorker {
 	agent: AgentDefinition;
 	modelOverride?: string | string[];
+	/** Pre-expansion role alias behind {@link modelOverride}, when the worker agent named one. */
+	modelRole?: string;
 }

 interface VibeTurn {
@@ -165,6 +167,8 @@ interface VibeRecord {
 	childSessionFile?: string;
 	agent: AgentDefinition;
 	modelOverride?: string | string[];
+	/** Pre-expansion role alias behind {@link modelOverride}, when the worker agent named one. */
+	modelRole?: string;
 	state: VibeSessionState;
 	createdAt: number;
 	lastActivityAt: number;
@@ -490,16 +494,17 @@ export class VibeSessionRegistry {
 			throw new ToolError(`Bundled agent "${agentName}" for vibe cli "${cli}" is unavailable.`);
 		}
 		const agentModelOverrides = session.settings.get("task.agentModelOverrides");
-		return {
-			agent,
-			modelOverride: resolveAgentModelPatterns({
-				settingsOverride: agentModelOverrides[agentName],
-				agentModel: agent.model,
-				settings: session.settings,
-				activeModelPattern: session.getActiveModelString?.(),
-				fallbackModelPattern: session.getModelString?.(),
-			}),
-		};
+		// Same contract as the task spawn path: the expansion discards the role
+		// alias (`@task`, `@smol`), so patterns and role identity come from one
+		// call — the child's inherited retry-fallback chain is keyed off the role.
+		const { patterns, role } = resolveAgentModelSelection({
+			settingsOverride: agentModelOverrides[agentName],
+			agentModel: agent.model,
+			settings: session.settings,
+			activeModelPattern: session.getActiveModelString?.(),
+			fallbackModelPattern: session.getModelString?.(),
+		});
+		return { agent, modelOverride: patterns, modelRole: role };
 	}

 	async #appendLifecycleEvent(
@@ -890,7 +895,7 @@ export class VibeSessionRegistry {
 				existing.sessionFile === childSessionFile &&
 				(existing.status === "idle" || existing.status === "parked");
 			const blockedByCollision = Boolean(existing && !existingIsResumable);
-			const { agent, modelOverride } = this.#resolveWorker(session, spawn.cli);
+			const { agent, modelOverride, modelRole } = this.#resolveWorker(session, spawn.cli);
 			if (!existing) {
 				AgentRegistry.global().register({
 					id: spawn.id,
@@ -911,6 +916,7 @@ export class VibeSessionRegistry {
 				childSessionFile,
 				agent,
 				modelOverride,
+				modelRole,
 				state: "idle",
 				createdAt: spawn.createdAt,
 				lastActivityAt: candidate.lastActivityAt,
@@ -945,7 +951,7 @@ export class VibeSessionRegistry {
 			throw new ToolError("Vibe mode has exited; enter Vibe mode again before spawning a worker.");
 		}
 		const manager = this.#manager(session);
-		const { agent, modelOverride } = this.#resolveWorker(session, args.cli);
+		const { agent, modelOverride, modelRole } = this.#resolveWorker(session, args.cli);
 		if (!session.agentOutputManager) {
 			session.agentOutputManager = new AgentOutputManager(session.getArtifactsDir ?? (() => null));
 		}
@@ -969,6 +975,7 @@ export class VibeSessionRegistry {
 			childSessionFile,
 			agent,
 			modelOverride,
+			modelRole,
 			state: "starting",
 			createdAt,
 			lastActivityAt: createdAt,
@@ -1418,6 +1425,7 @@ export class VibeSessionRegistry {
 			taskDepth: session.taskDepth ?? 0,
 			detached: true,
 			modelOverride: record.modelOverride,
+			modelRole: record.modelRole,
 			parentActiveModelPattern: session.getActiveModelString?.(),
 			thinkingLevel: record.agent.thinkingLevel,
 			sessionFile,
@@ -1,7 +1,11 @@
 import { describe, expect, it } from "bun:test";
 import { Effort } from "@oh-my-pi/pi-ai";
 import { buildModel } from "@oh-my-pi/pi-catalog/build";
-import { resolveAgentModelPatterns, resolveModelOverride } from "@oh-my-pi/pi-coding-agent/config/model-resolver";
+import {
+	resolveAgentModelPatterns,
+	resolveAgentModelSelection,
+	resolveModelOverride,
+} from "@oh-my-pi/pi-coding-agent/config/model-resolver";
 import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings";
 import { getBundledAgent } from "@oh-my-pi/pi-coding-agent/task/agents";
 import { AUTO_THINKING } from "@oh-my-pi/pi-coding-agent/thinking";
@@ -57,4 +61,34 @@ describe("bundled agent parsing", () => {
 		expect(resolved.thinkingLevel).toBe(Effort.XHigh);
 		expect(resolved.explicitThinkingLevel).toBe(true);
 	});
+
+	// The alias is expanded before it reaches the executor, so the role identity
+	// only survives as the `role` half of the selection. A subagent's inherited
+	// `retry.fallbackChains` entry is keyed off it — lose it and every bundled
+	// agent silently retries on the `default` role's chain.
+	it("keeps the role identity of every alias-routed bundled agent through expansion", () => {
+		const settings = Settings.isolated({
+			modelRoles: {
+				default: "anthropic/opus",
+				task: "anthropic/sonnet",
+				smol: "fast/hy3",
+				slow: "codex/sol",
+				designer: "anthropic/opus",
+			},
+		});
+
+		for (const [name, role, model] of [
+			["task", "task", "anthropic/sonnet"],
+			["sonic", "smol", "fast/hy3"],
+			["scout", "smol", "fast/hy3"],
+			["reviewer", "slow", "codex/sol"],
+			["designer", "designer", "anthropic/opus"],
+		] as const) {
+			const agent = getBundledAgent(name);
+			expect(resolveAgentModelSelection({ agentModel: agent?.model, settings })).toEqual({
+				patterns: [model],
+				role,
+			});
+		}
+	});
 });
@@ -178,7 +178,8 @@ describe("subagent runtime model resolution", () => {
 			return { session: createYieldingSession(), extensionsResult: {}, setToolUIContext: () => {} } as never;
 		});

-		// Mirrors the bundled scout agent (`model: "@smol"`).
+		// Mirrors what the spawn path hands the executor for the bundled scout
+		// (`model: "@smol"`): patterns already expanded, role carried beside them.
 		const agent: AgentDefinition = {
 			name: "scout",
 			description: "test",
@@ -192,6 +193,8 @@ describe("subagent runtime model resolution", () => {
 			task: "work",
 			index: 0,
 			id: "role-alias-chain",
+			modelOverride: ["fast/hy3"],
+			modelRole: "smol",
 			settings: Settings.isolated({
 				modelRoles: { default: "slow/opus", smol: "fast/hy3" },
 				"retry.fallbackChains": {
@@ -212,6 +215,53 @@ describe("subagent runtime model resolution", () => {
 		expect(childFallbackChains?.default).toEqual(["slow/opus-backup"]);
 	});

+	it("inherits the aliased role's chain when the spawn path pre-expands the alias", async () => {
+		// The real task flow (structured-subagent) resolves `@task` to a concrete
+		// selector before calling the executor and carries the role identity in
+		// `modelRole`. Re-deriving the role from the expanded patterns yields
+		// nothing, so the child must route off `modelRole`, not `default`.
+		const roleModel = model("task-provider", "sonnet");
+		const defaultModel = model("default-provider", "opus");
+		let childFallbackChains: Record<string, string[]> | undefined;
+		vi.spyOn(sdkModule, "createAgentSession").mockImplementation(async options => {
+			if (!options) throw new Error("Expected createAgentSession options");
+			childFallbackChains = options.settings?.get("retry.fallbackChains") as Record<string, string[]> | undefined;
+			return { session: createYieldingSession(), extensionsResult: {}, setToolUIContext: () => {} } as never;
+		});
+
+		const agent: AgentDefinition = {
+			name: "task",
+			description: "test",
+			systemPrompt: "test",
+			source: "bundled",
+			model: ["@task"],
+		};
+		await runSubprocess({
+			cwd: "/tmp",
+			agent,
+			task: "work",
+			index: 0,
+			id: "pre-expanded-role",
+			modelOverride: ["task-provider/sonnet"],
+			modelRole: "task",
+			settings: Settings.isolated({
+				modelRoles: { default: "default-provider/opus", task: "task-provider/sonnet" },
+				"retry.fallbackChains": {
+					default: ["task-provider/sonnet", "default-provider/sol"],
+					task: ["task-provider/sonnet"],
+				},
+			}),
+			modelRegistry: {
+				refresh: async () => {},
+				getAvailable: () => [roleModel, defaultModel],
+				getApiKey: async () => "test-key",
+			} as never,
+			enableLsp: false,
+		});
+
+		expect(childFallbackChains?.["subagent:pre-expanded-role"]).toEqual(["task-provider/sonnet"]);
+	});
+
 	it("inherits the default chain for a role alias whose role configures no chain", async () => {
 		const fast = model("fast", "hy3");
 		const slow = model("slow", "opus");
@@ -235,6 +285,8 @@ describe("subagent runtime model resolution", () => {
 			task: "work",
 			index: 0,
 			id: "role-alias-default-chain",
+			modelOverride: ["fast/hy3"],
+			modelRole: "smol",
 			settings: Settings.isolated({
 				modelRoles: { default: "slow/opus", smol: "fast/hy3" },
 				"retry.fallbackChains": { default: ["slow/opus-backup"] },
@@ -10,7 +10,7 @@ import {
 	parseModelString,
 	pickDefaultAvailableModel,
 	resolveAgentModelPatterns,
-	resolveAgentModelSource,
+	resolveAgentModelSelection,
 	resolveAgentPrewalkPattern,
 	resolveAllowedModels,
 	resolveCliModel,
@@ -846,7 +846,7 @@ describe("resolveAgentPrewalkPattern", () => {
 	});
 });
 describe("resolveAgentModelPatterns", () => {
-	test("selects the first non-empty source and skips aliases with no patterns", () => {
+	test("pairs the first non-empty source's role with its patterns, skipping aliases with no patterns", () => {
 		const settings = Settings.isolated({
 			modelRoles: {
 				empty: "",
@@ -855,32 +855,34 @@ describe("resolveAgentModelPatterns", () => {
 			},
 		});

-		const emptyRequest = {
-			requestModel: "",
-			settingsOverride: "@override",
-			agentModel: ["@definition"],
-			settings,
-		};
-		expect(resolveAgentModelPatterns(emptyRequest)).toEqual(["openai/gpt-4o"]);
-		expect(resolveAgentModelSource(emptyRequest)).toBe("@override");
+		expect(
+			resolveAgentModelSelection({
+				requestModel: "",
+				settingsOverride: "@override",
+				agentModel: ["@definition"],
+				settings,
+			}),
+		).toEqual({ patterns: ["openai/gpt-4o"], role: "override" });

-		const emptyAlias = {
-			requestModel: "@empty",
-			settingsOverride: ",,",
-			agentModel: ["@definition"],
-			settings,
-		};
-		expect(resolveAgentModelPatterns(emptyAlias)).toEqual(["anthropic/claude-sonnet-4-5"]);
-		expect(resolveAgentModelSource(emptyAlias)).toEqual(["@definition"]);
+		expect(
+			resolveAgentModelSelection({
+				requestModel: "@empty",
+				settingsOverride: ",,",
+				agentModel: ["@definition"],
+				settings,
+			}),
+		).toEqual({ patterns: ["anthropic/claude-sonnet-4-5"], role: "definition" });

-		const concreteRequest = {
-			requestModel: "openai/gpt-4o",
-			settingsOverride: "@override",
-			agentModel: ["@definition"],
-			settings,
-		};
-		expect(resolveAgentModelSource(concreteRequest)).toBe("openai/gpt-4o");
-		expect(resolveExplicitModelRole(resolveAgentModelSource(concreteRequest), settings)).toBeUndefined();
+		// An explicit selector carries no role identity, so the child must not
+		// capture the routing of a role that happens to name the same model.
+		expect(
+			resolveAgentModelSelection({
+				requestModel: "openai/gpt-4o",
+				settingsOverride: "@override",
+				agentModel: ["@definition"],
+				settings,
+			}),
+		).toEqual({ patterns: ["openai/gpt-4o"], role: undefined });
 	});

 	test("falls back to the active session model when @task is unset", () => {