From 323ca6394d3b83e0d05b755c4fbe6483dd3eddc1 Mon Sep 17 00:00:00 2001 From: Krishna Penukonda Date: Sat, 25 Jul 2026 09:00:50 +0000 Subject: [PATCH] fix(pi-subagents): inherit models and finalize provider errors --- extensions/pi-subagents/README.md | 3 +- extensions/pi-subagents/package.json | 10 +- .../pi-subagents/skills/pi-subagents/SKILL.md | 2 + .../pi-subagents/src/extension/index.ts | 12 +- .../pi-subagents/src/extension/schemas.ts | 5 +- .../runs/shared/subagent-prompt-runtime.ts | 25 ++ .../test/unit/minimal-subagents.test.ts | 251 ++++++++++++++++++ 7 files changed, 298 insertions(+), 10 deletions(-) diff --git a/extensions/pi-subagents/README.md b/extensions/pi-subagents/README.md index f465c11..85c22b5 100644 --- a/extensions/pi-subagents/README.md +++ b/extensions/pi-subagents/README.md @@ -1,6 +1,6 @@ # pi-subagents -Minimal recursive child-Pi spawner for Pi. +Minimal recursive child-Pi spawner for Pi 0.80.4 or newer. This extension intentionally does **not** define roles, agent types, chains, or parallel task lists. The parent agent supplies the full prompt and instructions for each child. @@ -22,6 +22,7 @@ spawn_subagent({ - No wait-for-completion mode exists; calls always return immediately with an ID and resultPath. - You will be notified when the subagent completes. - The returned ID is also the child Pi session ID and can be used with Pi session lookup/resume behavior. +- `model` is an explicit override; omitting it inherits the active parent provider/model. - `timeout` is optional (default 600s = 10 minutes) and measured in seconds. When reached, the parent is informed that the child is still running; the child is **not killed**. - Give `timeout` a healthy margin above expected runtime because child execution time can be wildly unpredictable. - Subagents always start with fresh session history. Put any desired context explicitly in `task`. diff --git a/extensions/pi-subagents/package.json b/extensions/pi-subagents/package.json index 9c778e1..a0ad98b 100644 --- a/extensions/pi-subagents/package.json +++ b/extensions/pi-subagents/package.json @@ -51,7 +51,7 @@ "peerDependencies": { "@earendil-works/pi-agent-core": "*", "@earendil-works/pi-ai": "*", - "@earendil-works/pi-coding-agent": "*", + "@earendil-works/pi-coding-agent": ">=0.80.4", "@earendil-works/pi-tui": "*" }, "peerDependenciesMeta": { @@ -73,9 +73,9 @@ "typebox": "^1.1.24" }, "devDependencies": { - "@earendil-works/pi-agent-core": "^0.74.0", - "@earendil-works/pi-ai": "^0.74.0", - "@earendil-works/pi-coding-agent": "^0.74.0", - "@earendil-works/pi-tui": "^0.74.0" + "@earendil-works/pi-agent-core": "^0.80.4", + "@earendil-works/pi-ai": "^0.80.4", + "@earendil-works/pi-coding-agent": "^0.80.4", + "@earendil-works/pi-tui": "^0.80.4" } } diff --git a/extensions/pi-subagents/skills/pi-subagents/SKILL.md b/extensions/pi-subagents/skills/pi-subagents/SKILL.md index a57e80a..bd08d18 100644 --- a/extensions/pi-subagents/skills/pi-subagents/SKILL.md +++ b/extensions/pi-subagents/skills/pi-subagents/SKILL.md @@ -11,6 +11,7 @@ Use this tool to launch unrestricted child Pi sessions. The caller must include - `spawn_subagent({ task, timeout?, cwd?, model? })` - Calls return immediately; the parent will be notified when the subagent completes. + - `model` is an explicit override; omitting it inherits the active parent provider/model. - `timeout` is optional; default is `600` seconds (10 minutes). - Timeout is only a notification threshold: the parent is informed that the child is still running; the child is not killed. - The returned subagent id is also the child Pi session id. @@ -37,6 +38,7 @@ Calls return immediately; the parent will be notified when each subagent complet - No wait-for-completion mode exists. - No subagent types exist. - No chain or parallel-list mode exists. +- `model` is an explicit override; omitting it inherits the active parent provider/model. - `timeout` is optional and measured in seconds; omitted timeout defaults to 10 minutes. - When `timeout` expires, the parent is informed that the subagent is still running; the child is not killed. - Do not kill subagents autonomously to enforce `timeout`. diff --git a/extensions/pi-subagents/src/extension/index.ts b/extensions/pi-subagents/src/extension/index.ts index 73d50b9..e79df86 100644 --- a/extensions/pi-subagents/src/extension/index.ts +++ b/extensions/pi-subagents/src/extension/index.ts @@ -37,6 +37,7 @@ interface ToolDetails { timedOut?: boolean; timeoutAt?: number; timeoutMessage?: string; + model?: string; subagents?: Array<{ id: string; running: boolean }>; } @@ -792,6 +793,10 @@ function makeRecord( const id = randomUUID(); const parentId = parentSessionId(ctx); const dir = childDir(parentId, id); + const inheritedModel = ctx.model + ? `${ctx.model.provider}/${ctx.model.id}` + : undefined; + const model = params.model || inheritedModel; fs.mkdirSync(dir, { recursive: true }); return { id, @@ -799,7 +804,7 @@ function makeRecord( cwd: params.cwd ? path.resolve(ctx.cwd, params.cwd) : ctx.cwd, taskPreview: params.task.slice(0, 500), timeout: params.timeout ?? DEFAULT_TIMEOUT_SECONDS, - ...(params.model ? { model: params.model } : {}), + ...(model ? { model } : {}), running: false, sessionDir: dir, outputFile: path.join(dir, "result.log"), @@ -1281,7 +1286,7 @@ export default function registerSubagentExtension(pi: ExtensionAPI): void { name: "spawn_subagent", label: "Spawn subagent", description: - "Spawn a child Pi subagent for one task. timeout is optional and measured in seconds (default 600 = 10 minutes). This returns immediately, allowing the parent to spawn multiple concurrent subagents by calling spawn_subagent multiple times. Do not kill subagents autonomously to enforce timeout; the parent will be informed when timeout expires. Give a healthy timeout margin above expected runtime because subagent execution may be wildly unpredictable.", + "Spawn a child Pi subagent for one task. model is an explicit override; omitting it inherits the active parent provider/model. timeout is optional and measured in seconds (default 600 = 10 minutes). This returns immediately, allowing the parent to spawn multiple concurrent subagents by calling spawn_subagent multiple times. Do not kill subagents autonomously to enforce timeout; the parent will be informed when timeout expires. Give a healthy timeout margin above expected runtime because subagent execution may be wildly unpredictable.", parameters: SpawnSubagentParams, async execute( id, @@ -1336,13 +1341,14 @@ export default function registerSubagentExtension(pi: ExtensionAPI): void { content: [ { type: "text", - text: `Spawned subagent ${started.record.id}. Result will be at: ${started.record.outputFile}. You will be notified when this subagent completes. Do not poll for result. Do not sleep for result. Continue with whatever other work you may have.`, + text: `Spawned subagent ${started.record.id} using ${started.record.model ?? "Pi's selected model"}. Result will be at: ${started.record.outputFile}. You will be notified when this subagent completes. Do not poll for result. Do not sleep for result. Continue with whatever other work you may have.`, }, ], details: { id: started.record.id, running: started.record.running, resultPath: started.record.outputFile, + ...(started.record.model ? { model: started.record.model } : {}), }, }; }, diff --git a/extensions/pi-subagents/src/extension/schemas.ts b/extensions/pi-subagents/src/extension/schemas.ts index d21d750..8a56811 100644 --- a/extensions/pi-subagents/src/extension/schemas.ts +++ b/extensions/pi-subagents/src/extension/schemas.ts @@ -19,7 +19,10 @@ export const SpawnSubagentParams = Type.Object( }), ), model: Type.Optional( - Type.String({ description: "Optional model override for the child Pi." }), + Type.String({ + description: + "Explicit model override for the child Pi. Omitting it inherits the active parent provider/model.", + }), ), }, { additionalProperties: false }, diff --git a/extensions/pi-subagents/src/runs/shared/subagent-prompt-runtime.ts b/extensions/pi-subagents/src/runs/shared/subagent-prompt-runtime.ts index bd76c17..bb49397 100644 --- a/extensions/pi-subagents/src/runs/shared/subagent-prompt-runtime.ts +++ b/extensions/pi-subagents/src/runs/shared/subagent-prompt-runtime.ts @@ -57,6 +57,31 @@ function setupLifelineWatcher(): void { setupLifelineWatcher(); export default function registerSubagentPromptRuntime(pi: ExtensionAPI): void { + let settledProviderError: string | undefined; + let exitingForProviderError = false; + + pi.on("message_end", (event) => { + if (event.message.role !== "assistant") return; + if (event.message.stopReason !== "error") { + settledProviderError = undefined; + return; + } + settledProviderError = + event.message.errorMessage?.trim() || + `Provider/model request failed for ${event.message.provider}/${event.message.model}.`; + }); + + pi.on("agent_settled", () => { + if (!settledProviderError || exitingForProviderError) return; + exitingForProviderError = true; + process.exitCode = 1; + process.stderr.write(`${settledProviderError}\n`); + // message_end has been persisted and agent_settled guarantees no retry, + // compaction, or queued continuation remains. Force exit because the + // dedicated parent lifeline intentionally keeps the event loop alive. + setImmediate(() => process.exit(1)); + }); + pi.on("tool_call", (event) => { if (!FILE_TOOL_NAMES.has(event.toolName)) return; const resultPath = process.env[SUBAGENT_RESULT_PATH_ENV]?.trim(); diff --git a/extensions/pi-subagents/test/unit/minimal-subagents.test.ts b/extensions/pi-subagents/test/unit/minimal-subagents.test.ts index a26c843..20704e4 100644 --- a/extensions/pi-subagents/test/unit/minimal-subagents.test.ts +++ b/extensions/pi-subagents/test/unit/minimal-subagents.test.ts @@ -175,6 +175,30 @@ test("spawn schema accepts task only and rejects removed properties", () => { ); }); +test("model override contract is synchronized across schema, tool, README, and skill", () => { + const schemaDescription = + (SpawnSubagentParams.properties.model as { description?: string }).description ?? ""; + const extensionSource = fs.readFileSync( + path.join(projectRoot, "src", "extension", "index.ts"), + "utf-8", + ); + const readme = fs.readFileSync(path.join(projectRoot, "README.md"), "utf-8"); + const skill = fs.readFileSync( + path.join(projectRoot, "skills", "pi-subagents", "SKILL.md"), + "utf-8", + ); + + for (const [surface, text] of [ + ["schema", schemaDescription], + ["tool", extensionSource], + ["README", readme], + ["skill", skill], + ] as const) { + assert.match(text, /(?:explicit model override|model[^\n]*explicit override)/i, `${surface} must call model an explicit override`); + assert.match(text, /omit(?:ted|ting)[^\n]*inherit[^\n]*parent[^\n]*provider\/model/i, `${surface} must document canonical parent model inheritance`); + } +}); + test("user-facing packaged docs do not expose removed API concepts", () => { const packageJsonPath = path.join(projectRoot, "package.json"); const packageJsonText = fs.readFileSync(packageJsonPath, "utf-8"); @@ -243,6 +267,83 @@ function makeFakeCtx(sessionId: string, cwd: string, hasUI: boolean) { }; } +function runPromptRuntimeTerminalMessage( + stopReason: "stop" | "error" | "aborted", + errorMessage?: string, +) { + const runtimePath = path.join( + projectRoot, + "src", + "runs", + "shared", + "subagent-prompt-runtime.ts", + ); + const script = ` + import registerRuntime from ${JSON.stringify(runtimePath)}; + let messageEnd; + let agentSettled; + registerRuntime({ + on(event, handler) { + if (event === "message_end") messageEnd = handler; + if (event === "agent_settled") agentSettled = handler; + }, + }); + if (!messageEnd) throw new Error("message_end handler was not registered"); + if (!agentSettled) throw new Error("agent_settled handler was not registered"); + await messageEnd({ + type: "message_end", + message: { + role: "assistant", + content: [], + provider: "test-provider", + model: "test-model", + stopReason: ${JSON.stringify(stopReason)}, + errorMessage: ${JSON.stringify(errorMessage)}, + }, + }); + await agentSettled({ type: "agent_settled" }); + if (${JSON.stringify(stopReason)} === "error") { + setInterval(() => {}, 60_000); + } else { + setTimeout(() => process.exit(0), 30); + } + `; + return spawnSync( + process.execPath, + ["--experimental-strip-types", "--input-type=module", "-e", script], + { + cwd: projectRoot, + encoding: "utf-8", + timeout: 1500, + env: { ...process.env, PI_NO_COLOR: "1" }, + }, + ); +} + +test("prompt runtime exits nonzero promptly on canonical assistant provider failure", () => { + const startedAt = Date.now(); + const result = runPromptRuntimeTerminalMessage( + "error", + "HTTP 402: provider credits exhausted", + ); + + assert.equal(result.status, 1, result.stderr || result.stdout); + assert.ok(Date.now() - startedAt < 1000, "provider failure must not wait on the child lifeline"); + assert.match(result.stderr, /HTTP 402: provider credits exhausted/); +}); + +test("prompt runtime does not misclassify abort as provider failure", () => { + const result = runPromptRuntimeTerminalMessage("aborted", "Request was aborted"); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.doesNotMatch(result.stderr, /Request was aborted/); +}); + +test("prompt runtime leaves successful terminal messages successful", () => { + const result = runPromptRuntimeTerminalMessage("stop"); + assert.equal(result.status, 0, result.stderr || result.stdout); + assert.equal(result.stderr, ""); +}); + test("prompt runtime prepends exactly one line and preserves content", () => { const prompt = "SYSTEM\n\n# Project Context\nkeep this\n\nThe following skills provide specialized instructions for specific tasks.\nkeep skills"; @@ -442,6 +543,156 @@ test("async completion persists success and pending metadata when stale notifica } }); +test("omitted model inherits and reports the active parent canonical provider/model", async () => { + const mockPi = createMockPi(); + mockPi.install(); + mockPi.onCall({ output: "inherited model done", exitCode: 0 }); + + const { sessionId, ctx } = makeTestCtx("pi-subagents-model-inherit"); + const parentModel = { provider: "openai-codex", id: "gpt-5.3-codex" }; + const { spawnTool } = registerTestTools(() => {}); + + try { + const result = await spawnTool.execute( + "model-inherit-child", + { task: "inherit parent model" }, + new AbortController().signal, + undefined, + { ...ctx, model: parentModel }, + ); + const record = await waitForPersistedRecord(sessionId, result.details.id); + const args = readLatestMockPiArgs(mockPi).args; + const modelFlag = args.indexOf("--model"); + + assert.notEqual(modelFlag, -1, "child args must always select the effective model"); + assert.equal(args[modelFlag + 1], "openai-codex/gpt-5.3-codex"); + assert.equal(record.model, "openai-codex/gpt-5.3-codex"); + assert.equal(result.details.model, "openai-codex/gpt-5.3-codex"); + assert.match(result.content[0].text, /openai-codex\/gpt-5\.3-codex/); + } finally { + mockPi.uninstall(); + cleanupTestCtx(ctx, sessionId); + } +}); + +test("explicit model override wins over the active parent model", async () => { + const mockPi = createMockPi(); + mockPi.install(); + mockPi.onCall({ output: "override model done", exitCode: 0 }); + + const { sessionId, ctx } = makeTestCtx("pi-subagents-model-override"); + const { spawnTool } = registerTestTools(() => {}); + const override = "anthropic/claude-opus-4-6"; + + try { + const result = await spawnTool.execute( + "model-override-child", + { task: "override parent model", model: override }, + new AbortController().signal, + undefined, + { ...ctx, model: { provider: "openai", id: "gpt-5.4" } }, + ); + const record = await waitForPersistedRecord(sessionId, result.details.id); + const args = readLatestMockPiArgs(mockPi).args; + const modelFlag = args.indexOf("--model"); + + assert.equal(args[modelFlag + 1], override); + assert.equal(record.model, override); + assert.equal(result.details.model, override); + } finally { + mockPi.uninstall(); + cleanupTestCtx(ctx, sessionId); + } +}); + +test("provider failure finalizes once with error text and result fallback", async () => { + const mockPi = createMockPi(); + mockPi.install(); + mockPi.onCall({ + jsonl: [{ + type: "message_end", + message: { + role: "assistant", + content: [], + provider: "test-provider", + model: "test-model", + stopReason: "error", + errorMessage: "HTTP 402: provider credits exhausted", + }, + }], + stderr: "HTTP 402: provider credits exhausted\n", + exitCode: 1, + }); + + const { sessionId, ctx } = makeTestCtx("pi-subagents-provider-failure"); + const notifications: string[] = []; + const { spawnTool } = registerTestTools((message) => { + const content = (message as { content?: unknown }).content; + if (typeof content === "string") notifications.push(content); + }); + + try { + const result = await spawnTool.execute( + "provider-failure-child", + { task: "trigger provider failure" }, + new AbortController().signal, + undefined, + ctx, + ); + const record = await waitForPersistedRecord(sessionId, result.details.id); + + assert.equal(record.running, false); + assert.equal(typeof record.completedAt, "number"); + assert.match(record.error, /HTTP 402: provider credits exhausted/); + assert.equal(fs.readFileSync(record.outputFile, "utf-8"), "(error)\n"); + assert.equal(notifications.length, 1, "failure must emit one completion notification"); + assert.equal(record.notifiedCompletion, true); + } finally { + mockPi.uninstall(); + cleanupTestCtx(ctx, sessionId); + } +}); + +test("timeout remains notification-only and child later completes", async () => { + const mockPi = createMockPi(); + mockPi.install(); + mockPi.onCall({ output: "late success", exitCode: 0, delay: 180 }); + + const { sessionId, ctx } = makeTestCtx("pi-subagents-timeout-notify-only"); + const notifications: string[] = []; + const { spawnTool } = registerTestTools((message) => { + const content = (message as { content?: unknown }).content; + if (typeof content === "string") notifications.push(content); + }); + + try { + const result = await spawnTool.execute( + "timeout-notify-child", + { task: "finish after threshold", timeout: 0.04 }, + new AbortController().signal, + undefined, + ctx, + ); + const timedOut = await waitForPersistedRecord( + sessionId, + result.details.id, + (record) => record.running === true && typeof record.timeoutAt === "number", + ); + assert.equal(timedOut.running, true, "timeout must not kill the child"); + assert.equal(timedOut.timeoutNotified, true); + assert.equal(notifications.filter((text) => text.includes("still running; not killed")).length, 1); + + const completed = await waitForPersistedRecord(sessionId, result.details.id); + assert.equal(completed.running, false); + assert.equal(completed.error, undefined); + assert.equal(fs.readFileSync(completed.outputFile, "utf-8").trim(), "late success"); + assert.equal(notifications.filter((text) => text.includes("completed")).length, 1); + } finally { + mockPi.uninstall(); + cleanupTestCtx(ctx, sessionId); + } +}); + test("spawn persists unified id session file result.log and fresh args", async () => { const mockPi = createMockPi(); mockPi.install();