diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index a15aa5523..8a4c96a37 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -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 `: 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 diff --git a/packages/coding-agent/src/config/model-resolver.ts b/packages/coding-agent/src/config/model-resolver.ts index 6f77535c4..ca66360bf 100644 --- a/packages/coding-agent/src/config/model-resolver.ts +++ b/packages/coding-agent/src/config/model-resolver.ts @@ -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"; diff --git a/packages/coding-agent/src/task/executor.ts b/packages/coding-agent/src/task/executor.ts index a48069220..3b482c047 100644 --- a/packages/coding-agent/src/task/executor.ts +++ b/packages/coding-agent/src/task/executor.ts @@ -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:` 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 = { @@ -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, diff --git a/packages/coding-agent/test/bundled-agent-parsing.test.ts b/packages/coding-agent/test/bundled-agent-parsing.test.ts index 8d206ca74..bcc1efbbf 100644 --- a/packages/coding-agent/test/bundled-agent-parsing.test.ts +++ b/packages/coding-agent/test/bundled-agent-parsing.test.ts @@ -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, + }); + } + }); }); diff --git a/packages/coding-agent/test/issue-2750-subagent-runtime-fallback.test.ts b/packages/coding-agent/test/issue-2750-subagent-runtime-fallback.test.ts index 93cf7ad3a..3bbe30f7c 100644 --- a/packages/coding-agent/test/issue-2750-subagent-runtime-fallback.test.ts +++ b/packages/coding-agent/test/issue-2750-subagent-runtime-fallback.test.ts @@ -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 | undefined; + vi.spyOn(sdkModule, "createAgentSession").mockImplementation(async options => { + if (!options) throw new Error("Expected createAgentSession options"); + childFallbackChains = options.settings?.get("retry.fallbackChains") as Record | 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"] }, diff --git a/packages/coding-agent/test/model-resolver.test.ts b/packages/coding-agent/test/model-resolver.test.ts index b64a0cc3e..f050cfa44 100644 --- a/packages/coding-agent/test/model-resolver.test.ts +++ b/packages/coding-agent/test/model-resolver.test.ts @@ -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", () => {