From 576b50f7e4c4256a57acf029fea5716cc2bcd221 Mon Sep 17 00:00:00 2001 From: brynne8 Date: Wed, 21 Oct 2026 23:19:08 +0800 Subject: [PATCH] fix(subagents): carry the delegation policy in the run's prompt A subagent policy only works where the model reads it. Upstream ships the delegation advice as the `Agent` tool's `promptGuidelines`, and pi flattens those into the one flat `` list that every tool and pi's own "be concise" share, so the policy arrives as generic advice next to advice about file paths. Nothing told the model that `explore` is for questions about existing code, that `plan` is for a multi-file change, or that delegating is cheaper than doing the work inline: that lived only in the tool's description and in the profile list. - The policy now ends every run's prompt as a `` block, built from the enabled profiles: `explore` and `plan` get a rule of their own, the other enabled subagents are listed by name. - The block rides on the text of the prompt, through a `before_agent_start` handler that returns `systemPrompt` (the shape `createExactSystemPromptExtension()` already uses), so a prompt another extension forced earlier is carried along rather than overwritten. - The `Agent` tool's prompt snippet says to delegate whenever possible and points at the delegation section. - The three `promptGuidelines` are gone: the block says the same things in the place the model reads, and keeping both would say each of them twice. --- docs/agents/subagents.md | 3 + lib/subagent-extension.integration.test.mjs | 70 +++++++++++++ lib/subagent-extension.test.mjs | 106 +++++++++++++++++--- lib/subagent-extension.ts | 74 ++++++++++++-- 4 files changed, 233 insertions(+), 20 deletions(-) diff --git a/docs/agents/subagents.md b/docs/agents/subagents.md index bf3f1d4902..9a60db040d 100644 --- a/docs/agents/subagents.md +++ b/docs/agents/subagents.md @@ -15,6 +15,9 @@ - An `extensions:` list (`[codegraph, "@team/adapter"]` or `codegraph, @team/adapter`) limits which extensions a child keeps, as pi-subagents does: `scopeSubagentExtensions()` (`lib/subagents.ts`) is an `extensionsOverride` that drops the rest after the SDK loaded them, together with the providers and models their factories queued. A dropped extension binds no handlers, tools or commands, but its factory has already run once, so this scopes the child; it is not a sandbox. Selecting paths before import would mean reaching into SDK internals on every reload (#1035). Names resolve like `ext:` selectors (directory, file or package name, case-insensitive), and a name two sources claim keeps neither. Path entries load nothing: a project profile must not load code that project trust gates. pi-web's own `` extensions always stay. `*` keeps every extension, `none` / `false` switch extensions off, and `load_extensions: false` wins over a list; the list parses as leniently as `skills:`. The SDK reapplies the override on every reload, and the snapshot keeps the names, so resume and RPC reopen get the same scope through `subagentExtensionLoaderOptions()`. `tools: ext:` still picks tools among the extensions that remain. The Agents panel shows the list read-only under the extensions switch, and a save keeps it as authored. `lib/subagent-extension-scope.integration.test.mjs` pins spawn, reload, resume and reopen against a real `AgentSession`. - A child that loads extensions without `ext:` selectors (the Agents panel's extensions switch) gets no `tools` allowlist: the SDK fixes one when the session is created, so a tool an extension registers at `session_start` or later (pi's `dynamic-tools` example) would never be callable (#883). `subagentToolOptions()` (`lib/subagents.ts`) passes `noTools: "builtin"` with the profile's built-ins as `+name` entries and excludes the reserved tools and the SDK's other built-ins (`SDK_BUILTIN_TOOLS` mirrors pi's unexported list), so no extension can switch on one the profile leaves out. The snapshot's `allExtensionTools` makes reopen do the same; an older snapshot keeps its allowlist. `ext:` selectors still pass the names resolved at spawn: the SDK admits tools by name, with no source-aware hook, so a selected extension's late tools stay out. Such an allowlist also excludes every MCP tool it does not name (`mcp__*` and the resource tools): pi keeps those registered for codemode and `tool_search` unless the list names one, so a script could otherwise call an unselected extension's MCP tool; `ext:` selectors that `disallowed_tools` cancels out admit no extension tool. `lib/subagent-codemode-scope.integration.test.mjs` pins this. `lib/subagent-late-tools.integration.test.mjs` pins spawn, reload and reopen. +## Delegation policy +- The delegation policy reaches the model as a `` block at the end of every run's system prompt (`subagentDelegationBlock()`, `withSubagentDelegationPrompt()`, appended by a `before_agent_start` handler that returns `systemPrompt`), not as the `Agent` tool's `promptGuidelines`: pi flattens those into the one flat `` list that every tool and pi's own "be concise" share, where a subagent policy reads as generic advice. The block is built from the enabled profiles: `explore` and `plan` get a rule of their own, the other enabled subagents are listed by name. + ## What the model may choose when spawning a subagent - `Agent` offers `subagent_type`, `prompt`, `description`, `run_in_background`, `resume`, `input_files`, `model`, and `isolation`. Thinking level, turn limit, and parent-context inheritance belong to the profile (`thinking`, `max_turns`, `inherit_context` in its frontmatter, editable in the Agents panel); the one exception is a level a call writes into its own `model` (below). A per-call value for any of them used to win over the profile while never appearing in the tool description, so a model guessing `max_turns: 8` or `thinking: "high"` silently discarded what the profile had decided. The runtime reads `thinking` (falling back to the parent's level), `max_turns`, and `inherit_context` from the profile, never from a parameter of that name. pi's validator allows keys the schema does not name, so a call that still sends one is accepted and the key ignored; transcripts holding such calls render and resume as before. - `model` resolves as `request ?? profile`: the profile decides by default, and one run may go to a different model, which the tool description informs by listing each profile's own model and thinking level. A model reference, from the call or the profile, reads as pi's `--model` does: `provider/modelId` or a bare id, optionally ending in `:` (#1188). The whole reference is tried first, so an id that contains a colon (`qwen3:8b`) still matches, and a suffix that is no level is refused, never dropped. The level travels with its model: a call's `model: "x:high"` sets that run's thinking over the profile's, which is how one call picks a model and its level together without a separate `thinking` parameter for a model to guess; a call naming a model without a level keeps the profile's `thinking`. Within a profile, `thinking:` wins over a level on its own `model:`, as pi's `--thinking` does over `--model x:level`. pi clamps the level to what the model supports. The Agents panel shows such a profile model as its base model (not unavailable) with the reference as written; choosing another model there drops the level, which the Thinking field sets. `input_files` stays a per-call parameter: the host inlines those UTF-8 files from the session cwd into the delegated task (`lib/subagent-input.ts`, bounded count and size), which is input preparation, not a profile setting. The size bound is the child model's: about a tenth of its context window by pi's chars/4, counted in bytes, never more than 512 KiB, and 512 KiB when the window is unknown (0). A copy at the flat limit (about 131k tokens) arrives in the child's first message and is over half of what a 272k-token model holds before pi compacts, so a child could compact before its first tool call (#1199). A larger set is refused before any worktree or child session exists, with an error telling the model to pass fewer or smaller files, or name the paths in `prompt` if the subagent can read them. That early check only looks the model up (the call's, else the profile's, else the parent's) and never fails on it: the child's model is still resolved, and a bad reference refused, after the child's `createAgentSessionServices()`, which reloads models.json into the shared runtime and registers the child's own extension providers. A model added in Settings › Models since the parent opened, or one only the child's extensions provide, therefore still runs, with the flat limit. `isolation` stays a string, not a `"worktree"` literal, so a model that fills every optional field with `""` can still start a subagent; only `"worktree"` takes effect. diff --git a/lib/subagent-extension.integration.test.mjs b/lib/subagent-extension.integration.test.mjs index e9f2b01985..69aaba6548 100644 --- a/lib/subagent-extension.integration.test.mjs +++ b/lib/subagent-extension.integration.test.mjs @@ -131,6 +131,15 @@ function textOf(message) { return message.content.map((block) => block.text ?? "").join(""); } +// The system message reaches the provider as text: a forced prompt as `content`, the structured +// sections as `sections`, already rendered. +function systemText(message) { + if (!message) return ""; + const content = typeof message.content === "string" ? message.content : textOf(message); + const sections = Object.values(message.sections ?? {}).join("\n\n"); + return `${content}\n\n${sections}`; +} + // A codemode script must not start, collect, or steer subagents: a run started from a script // would carry a nested parentToolCallId that no transcript entry has, so the chat could not // link to its session. The model itself keeps all three tools and can still call them. @@ -278,3 +287,64 @@ test("a session with the subagent feature switched off registers no control tool await rm(dir, { recursive: true, force: true }); } }); + +// The system prompt the provider receives, as the model reads it. +async function sentSystemPrompt(session, faux) { + let system = null; + faux.setResponses([ + (context) => { + system = context.messages.find((message) => message.role === "system"); + return fauxAssistantMessage([fauxText("done")]); + }, + ]); + await session.prompt("Say done."); + return systemText(system); +} + +// The delegation policy has to reach the model where it reads it. As the tool's +// `promptGuidelines` it lands in the one flat `` list, beside pi's own rules, where a +// subagent policy reads as generic advice; the block rides on the text of the prompt instead. +test("the prompt the model receives carries the delegation block, not the flat rules list", async () => { + const dir = await mkdtemp(join(tmpdir(), "pi-web-subagent-policy-")); + let session; + try { + const faux = fauxProvider({ models: [{ id: "faux-model" }] }); + const { runtime } = recordingRuntime(); + session = await createSession(dir, faux, [createSubagentExtension(runtime, () => [])]); + const text = await sentSystemPrompt(session, faux); + + assert.match(text, /\n- `Agent` runs a whole task in a fresh session/); + assert.match(text, /- Do not duplicate work already delegated to a running subagent\.\n<\/delegation>/); + // The rules list keeps pi's own rules, and no longer carries the policy. + assert.match(text, /- Be concise in your responses/); + assert.doesNotMatch(text, /Use Agent for a focused task/); + } finally { + session?.dispose(); + await rm(dir, { recursive: true, force: true }); + } +}); + +// A prompt another extension forced earlier is carried along, not replaced: the handler reads +// the rendered prompt, which already holds what the other extension forced. +test("a prompt another extension forced earlier is kept, and the block still rides on it", async () => { + const dir = await mkdtemp(join(tmpdir(), "pi-web-subagent-forced-")); + let session; + try { + const faux = fauxProvider({ models: [{ id: "faux-model" }] }); + const { runtime } = recordingRuntime(); + const voice = (pi) => { + pi.on("before_agent_start", (event) => ({ systemPrompt: `${event.systemPrompt}\n\nTalk like a caveman.` })); + }; + session = await createSession(dir, faux, [voice, createSubagentExtension(runtime, () => [])]); + const text = await sentSystemPrompt(session, faux); + + assert.match(text, /Talk like a caveman\./); + assert.match(text, /\n- `Agent` runs a whole task in a fresh session/); + // The tool's snippet names the block, so only the closing tag counts the block itself. + assert.equal(text.split("").length - 1, 1, "the block is appended once"); + assert.ok(text.indexOf("Talk like a caveman.") < text.indexOf("\n"), "the other prompt comes first"); + } finally { + session?.dispose(); + await rm(dir, { recursive: true, force: true }); + } +}); diff --git a/lib/subagent-extension.test.mjs b/lib/subagent-extension.test.mjs index 94e62f8b51..cdca61d4b0 100644 --- a/lib/subagent-extension.test.mjs +++ b/lib/subagent-extension.test.mjs @@ -5,20 +5,29 @@ import { createJiti } from "jiti"; const { createSubagentExtension, preferPiWebSubagentExtension, + SUBAGENT_DELEGATION_BLOCK, + withSubagentDelegationPrompt, subagentFinalText, subagentNotificationText, SUBAGENT_NOTIFICATION_PREFIX, } = await createJiti(import.meta.url).import("./subagent-extension.ts"); +// The extension registers tools and one event handler; the fake collects both. +function fakePi({ tools = new Map(), sent = [], handlers = new Map() } = {}) { + return { + registerTool(tool) { tools.set(tool.name, tool); }, + sendMessage(message, options) { sent.push({ message, options }); }, + on(type, handler) { handlers.set(type, handler); }, + }; +} + async function loadTools(runtime, getProfiles = () => [], isEnabled = () => true) { const tools = new Map(); const sent = []; + const handlers = new Map(); const extension = createSubagentExtension(runtime, getProfiles, isEnabled); - await extension.factory({ - registerTool(tool) { tools.set(tool.name, tool); }, - sendMessage(message, options) { sent.push({ message, options }); }, - }); - return { tools, sent }; + await extension.factory(fakePi({ tools, sent, handlers })); + return { tools, sent, handlers }; } function run(overrides = {}) { @@ -67,10 +76,7 @@ test("integrated extension exposes the legacy-compatible tool names", async () = async steer() {}, async notifyParent() {}, }, () => []); - await extension.factory({ - registerTool(tool) { tools.set(tool.name, tool); }, - sendMessage() {}, - }); + await extension.factory(fakePi({ tools })); assert.deepEqual([...tools.keys()], ["Agent", "get_subagent_result", "steer_subagent"]); const result = await tools.get("Agent").execute( @@ -119,15 +125,89 @@ test("integrated extension registers no tools while its feature is disabled", as }; const extension = createSubagentExtension(runtime, () => [], () => enabled); const disabledTools = new Map(); - await extension.factory({ registerTool(tool) { disabledTools.set(tool.name, tool); } }); + await extension.factory(fakePi({ tools: disabledTools })); assert.deepEqual([...disabledTools], []); enabled = true; const enabledTools = new Map(); - await extension.factory({ registerTool(tool) { enabledTools.set(tool.name, tool); } }); + await extension.factory(fakePi({ tools: enabledTools })); assert.deepEqual([...enabledTools.keys()], ["Agent", "get_subagent_result", "steer_subagent"]); }); +// The delegation policy has to reach the model where it reads it. pi flattens a tool's +// `promptGuidelines` into the one flat `` list that every tool and pi's own "be concise" +// share, so the policy rides on the text of the run's prompt instead, in a block of its own. +test("every run's prompt carries the delegation block, and the tool's rules do not", async () => { + const runtime = { + async start() { throw new Error("unused"); }, + async get() { return null; }, + async steer() {}, + async notifyParent() {}, + }; + const { tools, handlers } = await loadTools(runtime); + const agent = tools.get("Agent"); + + assert.equal(agent.promptGuidelines, undefined, "the policy is no longer a rule beside \"be concise\""); + assert.match(agent.promptSnippet, /delegate work to subagents instead of doing it yourself/); + assert.match(agent.promptSnippet, /`` section of the system prompt/); + + const handler = handlers.get("before_agent_start"); + assert.ok(handler, "the extension forces the prompt with the policy on it"); + const forced = await handler({ systemPrompt: "Base prompt." }); + assert.equal(forced.systemPrompt, `Base prompt.\n\n${SUBAGENT_DELEGATION_BLOCK}`); + + // The block is the policy the rules used to carry, in a form the model reads as a section. + assert.match(SUBAGENT_DELEGATION_BLOCK, /^- `Agent` runs a whole task in a fresh session and returns only the result, so this conversation stays small\./m); + assert.match(SUBAGENT_DELEGATION_BLOCK, /^- Use several background `Agent` calls in one response for independent parallel work;/m); + assert.match(SUBAGENT_DELEGATION_BLOCK, /^- Do not duplicate work already delegated to a running subagent\.$/m); + assert.deepEqual(SUBAGENT_DELEGATION_BLOCK.split("\n").map((line) => line.split(" ")[0]), [ + "", "-", "-", "-", "", + ]); + // No subagent named means no rule for one: the empty provider must not invent an agent. + assert.doesNotMatch(SUBAGENT_DELEGATION_BLOCK, /`explore`|`plan`/); +}); + +const profile = (overrides = {}) => ({ + name: "explore", + displayName: "Explore", + description: "Inspect the codebase", + systemPrompt: "Explore.", + tools: ["read", "grep"], + inheritContext: false, + runInBackground: false, + enabled: true, + scope: "builtin", + ...overrides, +}); + +test("the delegation block names the enabled profiles and leaves the disabled ones out", async () => { + const runtime = { + async start() { throw new Error("unused"); }, + async get() { return null; }, + async steer() {}, + async notifyParent() {}, + }; + const profiles = [ + profile(), + profile({ name: "plan", displayName: "Plan", description: "Plan a change", tools: ["read"] }), + profile({ name: "general-purpose", displayName: "General purpose", description: "Do the work", tools: ["read", "bash", "edit"] }), + profile({ name: "hidden", displayName: "Hidden", description: "Must stay hidden", enabled: false }), + ]; + const { handlers } = await loadTools(runtime, () => profiles); + const forced = await handlers.get("before_agent_start")({ systemPrompt: "Base." }); + const block = forced.systemPrompt.split("")[1]; + + // The two agents with a rule of their own are not repeated in the list, and the list is names + // only: a description read once in the rules needs no second copy. + assert.match(block, /`explore` for any question about existing code: where a symbol lives/); + assert.match(block, /`plan` before an edit that spans several files or steps: it reads the repository/); + assert.match(block, /^- Other subagents: `general-purpose`\.$/m); + assert.doesNotMatch(block, /Inspect the codebase/); + assert.doesNotMatch(block, /`hidden`/); + assert.equal(forced.systemPrompt.split("").length - 1, 1, "the block is appended once"); + assert.equal(forced.systemPrompt, withSubagentDelegationPrompt("Base.", profiles)); +}); + test("Agent tool description lists enabled effective profiles and refreshes when its factory reloads", async () => { const runtime = { async start() { throw new Error("unused"); }, @@ -163,7 +243,7 @@ test("Agent tool description lists enabled effective profiles and refreshes when const extension = createSubagentExtension(runtime, () => profiles); const firstTools = new Map(); - await extension.factory({ registerTool(tool) { firstTools.set(tool.name, tool); }, sendMessage() {} }); + await extension.factory(fakePi({ tools: firstTools })); const first = firstTools.get("Agent"); assert.match(first.description, /- explore: Inspect the codebase \(Tools: read, grep\)/); assert.doesNotMatch(first.description, /disabled-agent|Must stay hidden/); @@ -183,7 +263,7 @@ test("Agent tool description lists enabled effective profiles and refreshes when scope: "project", }]; const reloadedTools = new Map(); - await extension.factory({ registerTool(tool) { reloadedTools.set(tool.name, tool); }, sendMessage() {} }); + await extension.factory(fakePi({ tools: reloadedTools })); const reloaded = reloadedTools.get("Agent"); assert.match(reloaded.description, /- reviewer: Review a change \(Tools: none; Model: provider\/review-model; Thinking: high\)/); assert.doesNotMatch(reloaded.description, /explore/); diff --git a/lib/subagent-extension.ts b/lib/subagent-extension.ts index a699300321..5f2e7827d4 100644 --- a/lib/subagent-extension.ts +++ b/lib/subagent-extension.ts @@ -27,6 +27,64 @@ const TERMINAL_SUBAGENT_STATUSES = new Set(["complete * entry carries, so the chat would lose the link to the child session. */ const SUBAGENT_TOOL_EXPOSURE = "model-only" satisfies ToolExposure; +const AGENT_TOOL_NAME = "Agent"; + +/** + * The delegation policy, in a block of its own at the end of the system prompt. + * + * It is not the tool's `promptGuidelines`: pi appends those to the one flat `` list that + * every tool and pi's own "be concise" share, where a subagent policy reads as generic advice + * next to advice about file paths. Nor is it a `sections` entry: only a `before_agent_start` + * handler can name a section, and pi sends a forced prompt instead of the structured sections as + * soon as any extension forces one, so a section added by a handler that runs later reached + * neither the model nor the System panel. Appending the block to the text the run sends works + * whatever the shape of that text is. + * + * The block carries its own tag because a forced prompt is plain text: pi wraps nothing around + * it, so the label the policy is known by has to be in the text. + * + * `explore` and `plan` get a rule of their own, not just a line in the tool description: the + * policy says what each is for and why reaching for it is cheaper, because a generic "prefer a + * read-only profile" line named no agent and left the model to connect a word to a subagent it had + * never seen. The other profiles are listed by name, so a custom one is still discoverable. + */ +const DELEGATION_INTRO = `\`${AGENT_TOOL_NAME}\` runs a whole task in a fresh session and returns only the result, so this conversation stays small. Delegate as soon as the goal is clear; a goal and known paths are enough.`; + +const DELEGATION_RULES = [ + "Use several background `Agent` calls in one response for independent parallel work; their scopes may overlap, and an isolated run can use a worktree.", + "Do not duplicate work already delegated to a running subagent.", +]; + +/** The policy block, with the enabled subagents the model can choose from (`[]` lists none). */ +export function subagentDelegationBlock(profiles: readonly SubagentProfile[]): string { + const available = profiles.filter((profile) => profile.enabled); + const named = new Set(available.map((profile) => profile.name)); + const rules = [DELEGATION_INTRO]; + if (named.has("explore")) { + rules.push("`explore` for any question about existing code: where a symbol lives, how a flow works, which files touch a feature. It only reads and reports paths, so ask it first instead of reading the neighbourhood yourself."); + } + if (named.has("plan")) { + rules.push("`plan` before an edit that spans several files or steps: it reads the repository and returns steps, files, risks and verification, without changing anything."); + } + rules.push(...DELEGATION_RULES); + const others = available.filter((profile) => profile.name !== "explore" && profile.name !== "plan"); + if (others.length > 0) { + rules.push(`Other subagents: ${others.map((profile) => `\`${profile.name}\``).join(", ")}.`); + } + return ["", ...rules.map((rule) => `- ${rule}`), ""].join("\n"); +} + +export const SUBAGENT_DELEGATION_BLOCK = subagentDelegationBlock([]); + +/** + * The run's prompt with the delegation block appended. + * + * `event.systemPrompt` renders the options as they stand when it is read, so a prompt another + * extension forced earlier is already in that text and is carried along rather than overwritten. + */ +export function withSubagentDelegationPrompt(prompt: string, profiles: readonly SubagentProfile[] = []): string { + return `${prompt}\n\n${subagentDelegationBlock(profiles)}`; +} export interface SubagentToolDetails { kind: "pi-web-subagent"; @@ -183,14 +241,9 @@ export function createSubagentExtension( pi.registerTool(defineTool({ name: "Agent", label: "Agent", - description: `Delegate a focused task to a configured subagent. Each subagent runs as a full, inspectable Pi session. Use background mode for independent work and foreground mode when the result is needed immediately.\n\nAvailable agent types:\n${agentTypeDescription(profiles)}`, - promptSnippet: "Delegate a focused task to an inspectable subagent session", + description: `Delegate a task to a configured subagent. Each subagent runs as a full, inspectable Pi session. Use background mode for independent work and foreground mode when the result is needed immediately.\n\nAvailable agent types:\n${agentTypeDescription(profiles)}`, + promptSnippet: "Delegate a task to an inspectable subagent session. Whenever possible, delegate work to subagents instead of doing it yourself. Read the `` section of the system prompt for which subagent to pick and when.", exposure: SUBAGENT_TOOL_EXPOSURE, - promptGuidelines: [ - "Use Agent for a focused task that benefits from an isolated context.", - "Use multiple background Agent calls in the same response for independent parallel work.", - "Do not duplicate work already delegated to a running subagent.", - ], executionMode: "parallel", parameters: Type.Object({ subagent_type: Type.Optional(Type.String({ description: `Configured agent profile. Available types: ${availableTypes}. Default: general-purpose.` })), @@ -337,6 +390,13 @@ export function createSubagentExtension( } }, })); + + // The delegation policy, and the subagents it can pick, ride on every run's prompt; see + // subagentDelegationBlock(). The handler reads the rendered prompt and returns it with the + // block appended, so whatever pi or an earlier extension produced as text comes first. + pi.on("before_agent_start", (event) => { + return { systemPrompt: withSubagentDelegationPrompt(event.systemPrompt, profiles) }; + }); }, }; }